Make WordPress Core

Opened 9 months ago

Closed 7 weeks ago

#60645 closed enhancement (wontfix)

Add pre-fire hook for cron

Reported by: kkmuffme's profile kkmuffme Owned by: audrasjb's profile audrasjb
Milestone: 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 (17)

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


9 months ago
#1

  • Keywords has-patch added

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


7 months ago

#3 @jorbin
7 months ago

  • Version 6.5 deleted

#4 @audrasjb
7 months ago

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

Self assigning for review and tests.

#5 @audrasjb
7 months ago

  • Milestone changed from Awaiting Review to 6.6

#6 @rajinsharwar
7 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
7 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
6 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
6 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
6 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 follow-up: @kkmuffme
6 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.


6 months ago

#13 @oglekler
5 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.

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


2 months ago

#15 @chaion07
2 months ago

Thanks @kkmuffme for reporting this. We reviewed this during a recent bug-scrub session. Here's the summary of the discussion for this Ticket:

  1. Referring to Peter's comment, he is still not convinced if this hook is needed but happy to help if there is other contributors willing to take this forward.
  2. We are also waiting for @audrasjb to Shepard this Ticket closer to a resolution

Props to @peterwilsoncc

Cheers!

#16 in reply to: ↑ 11 @coquardcyr
7 weeks ago

Replying to kkmuffme:

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?

I don’t see the value of this new action.

This was justified by two use cases:
The first one is solved by the filter @peterwilsoncc proposed.
The second use case is not really clear.
From what I understood, it seems to be a way to reduce memory consumption and avoid old code usage.
However, I don't really see why checking for this right before firing the cron hook would change anything about theses points or provide any value on what is existing as it is exactly the same environment and we could instead directly use the cron hook to make theses checks.

Last edited 7 weeks ago by coquardcyr (previous) (diff)

#17 @johnbillion
7 weeks ago

  • Milestone 6.7 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I concur with the above that this action doesn't provide any functionality that isn't already possible via other means. Thanks as always @kkmuffme but I'll close this one off!

Note: See TracTickets for help on using tickets.