Make WordPress Core

#58494 closed enhancement (fixed)

Reduce frequency of https check when https is already working

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.4 Priority: normal
Severity: normal Version: 5.7
Component: Security Keywords: needs-testing has-patch commit
Focuses: sustainability Cc:

Description (last modified by adamsilverstein)

WordPress currently runs a 'twicedaily' job to check if https support is available. This is wasteful when the user already has https enabled. Although it can happen that a site goes from having https support to not having it (eg. when moving hosts or if an SSL certificate expires, this is probably rare, and the website owner is likely to notice issues when they try accessing their site via https as usual.

Therefore, following the example of the Eco-Mode plugin. I propose reducing the frequency in this case to weekly. Tis change will save resources and belongs squarely in the *new* sustainability focus.

Attachments (4)

58494.diff (7.2 KB) - added by adamsilverstein 11 months ago.
patch.diff (1.8 KB) - added by Michi91 11 months ago.
unschedule by using upgrade routines
58494.2.diff (12.1 KB) - added by adamsilverstein 11 months ago.
58494.3.diff (11.1 KB) - added by adamsilverstein 10 months ago.

Download all attachments as: .zip

Change History (37)

#1 @adamsilverstein
14 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#2 @adamsilverstein
14 months ago

  • Description modified (diff)

This ticket was mentioned in PR #4582 on WordPress/wordpress-develop by @adamsilverstein.


14 months ago
#3

  • Keywords has-patch added; needs-patch removed

#4 @adamsilverstein
14 months ago

  • Keywords needs-patch added; has-patch removed

As a back of the envelope estimation, this will result in the following energy savings

Assumptions
~80% of sites use https already reference
~ 810 Million sites run WordPress reference
~ request energy intensity figure is 0.0065 kWh/GB reference
~ median page weight is 2MB reference

Math
~ 648 million sites will reduce requests from ~60/month to 4/month, a reduction of 56 requests or 36288000000 requests
~ at 2mb average that amounts to 72576000 GB of savings

Total estimated savings of 471,744 KwH/month
~

Last edited 13 months ago by adamsilverstein (previous) (diff)

#5 @adamsilverstein
14 months ago

  • Version set to 5.7

#6 @SergeyBiryukov
14 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 6.3

#7 @oglekler
13 months ago

  • Keywords needs-testing added

I am adding needs-testing to highlight that this patch is ready for review.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


13 months ago

#10 @Hareesh Pillai
13 months ago

  • Keywords needs-testing-info added

While I'm 100% aligned with the objective of this ticket and the potential savings, I'm not entirely sure how to test this patch.
Some details on how to test will help drive this forward.

#11 @audrasjb
13 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 6.3 to 6.4

This sounds like a great idea, thanks!

I tried testing this patch on a site on production with https enabled, and it didn't work: the scheduled event never switched to weekly (checking with WP Crontrol).

Also, the PR is still a draft, so let's move this ticket to milestone 6.4.

#12 @adamsilverstein
13 months ago

Some details on how to test will help drive this forward.

Hi @hareesh-pillai -
Here are some steps to test:

Before and after the patch:

  • Set up a site with valid SSL (also, test with invalid SSL if possible)
  • Use wp-cli to check when the next wp_https_detection cron job is scheduled. Use this command: wp cron event list, look for the wp_https_detection row
  • Note the 'recurrence' column, the expected default value will be 12 hours
  • Run the check cron event: wp cron event run wp_https_detection
  • Check the cron job list again (wp cron event list)
  • Before the patch, the recurrence will remain at 12 hours
  • After the patch, if the SSL is valid, the recurrence will change to 1 week

I tried testing this patch on a site on production with https enabled, and it didn't work: the scheduled event never switched to weekly (checking with WP Crontrol).

@audrasjb

Did you leave the patch in place for at least 12 hours? the recurrence will only change to 1 week after the next cron check.

Also, the PR is still a draft, so let's move this ticket to milestone 6.4.

Have to stop making them drafts, I'll update that!

@adamsilverstein commented on PR #4582:


13 months ago
#13

marked as ready for review, only had as a draft out of habit. Appreciate any feedback.

#14 @adamsilverstein
13 months ago

  • Keywords has-patch needs-unit-tests added; needs-testing-info needs-patch removed

This could use some unit tests.

#15 @johnbillion
13 months ago

  • Keywords 2nd-opinion added

@adamsilverstein Alternative: Is this cron event really needed? As far as I can tell, the only place a user is informed about support for HTTPS being available when they're using HTTP is when they view Site Health. Could this cron event be removed entirely and the HTTPS check moved to an async site health check?

This ticket was mentioned in Slack in #core by oglekler. View the logs.


11 months ago

#17 @oglekler
11 months ago

This ticket was discussed during a bug scrub.
@adamsilverstein is it possible to follow what @johnbillion is suggested?
@audrasjb can you provide further insides, please?
I want to ping @Clorith for the opinion as well.

Add props to: @mukesh27

#18 @adamsilverstein
11 months ago

@adamsilverstein Alternative: Is this cron event really needed? As far as I can tell, the only place a user is informed about support for HTTPS being available when they're using HTTP is when they view Site Health. Could this cron event be removed entirely and the HTTPS check moved to an async site health check?

Great suggestion, I will double check that is the case and try moving.

#19 @adamsilverstein
11 months ago

58494.diff removes the cron check entirely, the check is now only executed when visiting Site Health. Also in https://github.com/WordPress/wordpress-develop/pull/4582.

@johnbillion commented on PR #4582:


11 months ago
#20

I've not tested this yet but I think this will need an upgrade routine to remove the existing cron event. I'll review soon.

@adamsilverstein commented on PR #4582:


11 months ago
#21

I've not tested this yet but I think this will need an upgrade routine to remove the existing cron event. I'll review soon.

Yea, probably: I have this removal code here, but that would be much better in an upgrade routine instead. I can look into that or feel free to push a change.

This ticket was mentioned in Slack in #core by michi91. View the logs.


11 months ago

@Michi91
11 months ago

unschedule by using upgrade routines

#23 @Michi91
11 months ago

Hey @adamsilverstein I didnt know how to push it to you using git as i dont have access to you repo, but heres a patch :-)

#24 @adamsilverstein
11 months ago

Hey @adamsilverstein I didnt know how to push it to you using git as i dont have access to you repo, but heres a patch :-)

Great, thanks that works - I'll take a look! (I think you would need to fork my repo, then push to your fork, then open a pr against my branch)

This ticket was mentioned in Slack in #core by oglekler. View the logs.


11 months ago

#26 @oglekler
11 months ago

Hi @Michi91 this is an enhancement, and we have 12 days before Beta 1 and at this moment all enhancements needs to be on the Trunk. Please write if you did what was suggested by Adam to move things forward. Thank you!

#27 @Michi91
11 months ago

@oglekler thank you for the ping. I have sent the patch as pull request to @adamsilverstein fork and pinged him. Lets see if he can merge :)

#28 @adamsilverstein
11 months ago

Hey @Michi91 - I wasn't able to use the PR, but the patch worked fine! Thanks! I added this to my PR and will now rebase against trunk to resolve conflicts.

#29 @adamsilverstein
11 months ago

@johnbillion appreciate a review if you have a moment, this should be ready to go. The code now removes the cron job as part of an upgrade task.

#30 @johnbillion
10 months ago

  • Keywords commit added; needs-unit-tests 2nd-opinion removed

#31 @adamsilverstein
10 months ago

Thanks for the review, I'll get this committed.

#32 @adamsilverstein
10 months ago

58494.3.diff is a refresh against trunk

#33 @adamsilverstein
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56664:

Security: remove the cron event that checked for https support.

Fix an issue where a cron job ran every 12 hours to check for https support - even when https support was already enabled. The check is now run only when the user visits the Site Health page. Reducing the unneeded requests lowers the impact and load of hosting WordPress sites.

The wp_update_https_detection_errors function is deprecated and the https_detection_errors option that was previously set by the cron job is no longer maintained. The pre_wp_update_https_detection_errors filter is deprecated and replaced by the pre_wp_get_https_detection_errors filter which serves the same function.

Props audrasjb, johnbillion, Michi91.
Fixes #58494.

Note: See TracTickets for help on using tickets.