Make WordPress Core

Opened 5 months ago

Last modified 2 months ago

#60645 assigned enhancement

Add pre-fire hook for cron

Reported by: kkmuffme's profile kkmuffme Owned by: audrasjb's profile audrasjb
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch needs-testing needs-docs dev-feedback
Focuses: Cc:

Description

Followup to https://core.trac.wordpress.org/ticket/56048 https://core.trac.wordpress.org/changeset/54258

This adds an additional hook before every cron event to allow for custom code to run before a cron hook

Use cases:

  • custom error checking (e.g. if a cron even has no callbacks,...)
  • load libraries/functions/classes that are only needed for a specific number of cron events and would otherwise slow down the site/cron - this hook allows you to handle multiple at once which is useful for e.g. dynamic hook names or for cases where you need to run it for a number of hooks (instead of adding 100 add_action with same callback)

Change History (13)

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


5 months ago
#1

  • Keywords has-patch added

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


4 months ago

#3 @jorbin
4 months ago

  • Version 6.5 deleted

#4 @audrasjb
4 months ago

  • Keywords needs-testing added
  • Owner set to audrasjb
  • Status changed from new to assigned

Self assigning for review and tests.

#5 @audrasjb
4 months ago

  • Milestone changed from Awaiting Review to 6.6

#6 @rajinsharwar
4 months ago

  • Keywords needs-docs added

Added a small space change suggestion, also we will be needing docs for this hook I believe.

#7 @kkmuffme
3 months ago

will be needing docs for this hook I believe.

Can you clarify which docs you mean? There's already a docblock there?

#8 @oglekler
3 months ago

  • Keywords dev-feedback added

I wonder if this hook makes sense. The hooked function itself can load any libraries, objects, classes and whatever, the code needs to be organized this way reducing unnecessary load and moving this action from one hook to another is not making the code itself better. If you have two actions registered in the same big class, it will not make any difference.

#9 @kkmuffme
3 months ago

That will create tons of redundant code with dynamic hook names, since you'd have to call the same function to load it in each of them.

Additionally, we use this hook for identification of cron hooks that do not have any callbacks registered. This happens a lot with plugins that rename their hooks in updates but forgot to unschedule the old hooks as well as with plugins that are deactivated/uninstalled but don't unschedule their hooks.
There's no other way to do this atm.

This matters, since every hook (even if unused) will write (update) the cron option, which is rather expensive to update as it requires to be updated atomically and have a grace period to not get major race conditions in a multi-server DB multi-tiered object cache set up.

#10 @peterwilsoncc
3 months ago

I don't really see the need for this filter.

Detecting cron jobs without any callbacks registered can be done via the pre_reschedule_event filter. If no callbacks are registerd then the hook can be removed and rescheduling overridden.

I think the hooked function for the cron job loading the files is the correct approach as each callback should be self-sufficient in the event it is fired directly. To avoid duplicate code a plugin an create a helper function for the purpose.

There are also some backward/forward compatibility issues with WP-CLI to consider. WP-CLI fires the action directly (view source code) so there is a risk the new hook would be missed on systems running real cron using the CLI.

#11 @kkmuffme
3 months ago

Detecting cron jobs without any callbacks registered can be done via the pre_reschedule_event filter. If no callbacks are registerd then the hook can be removed and rescheduling overridden.

Thanks for the suggestion, but it can't be used because:

  • the function can be called from arbitrary locations
  • there are tons of hooks that will run after that (all of which cannot be used for this, since those hooks are also called in various other cases, not only in processing) that could add an add_action

I think the hooked function for the cron job loading the files is the correct approach as each callback should be self-sufficient in the event it is fired directly.

I think that's a misunderstanding from my original text (it sounds differently in my last comment, which is probably why): the hook is used to LOAD the callback in the first place. (the add action which will run on the cron event isn't loaded at all but only loaded by this new action)
Without the new hook, there's no callback.

This avoids having to instantiate 1000s of classes that may potentially have an add action in cron on every cron request - instead the class will only be instantied if the event using the class/file is actually executed.
Leading to
a) faster cron execution (since that is an issue already, since cron starts to time lag extremely quickly) b) lower PHP memory consumption and
c) avoids executing old code (which has been an issue in CD for us in some cases, since we have a cron timeout of 15 minutes, and some plugins/classes were updated in the meantime, leading to the execution of old code)

Another thing we started to use this hook is to continuously re-check environment conditions - e.g. wp_die if the site suddenly switched to maintenance mode, the DB version changed,... to avoid corrupting data or producing tons of notices as those are regular issues that happen when using an actual PHP-CLI cron.

There are also some backward/forward compatibility issues with WP-CLI to consider

The existing hooks cron_reschedule_event_error and cron_unschedule_event_error are missing in CLI already too, so that wouldn't be an issue.

---

Are there any downsides to adding this action?

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


2 months ago

#13 @oglekler
2 months ago

  • Milestone changed from 6.6 to 6.7

We have 2 days before Beta 1 and no concrete understanding, so, I am moving this ticket to the next milestone for further discussion.

Note: See TracTickets for help on using tickets.