Pierre-Lannoy / wp-decalog

Capture and log events, metrics and traces on your site. Make WordPress observable - finally!
https://perfops.one/
GNU General Public License v3.0
64 stars 8 forks source link

Corelistener - pre_clear_scheduled_hook: Wrong Log message if external cron system used #20

Closed JanThiel closed 2 years ago

JanThiel commented 3 years ago

Describe the bug If pre_clear_scheduled_hook returns anything other than null the logger logs:

$this->logger->notice( sprintf( 'A plugin prevented the "%s" event to be cleared.', $hook ) );

Yet this message is wrong. In our case we replaced wp-cron with Cavalcade. The WordPress Docs states:

For plugins replacing wp-cron, return the number of events successfully unscheduled (zero if no events were registered with the hook) or false if unscheduling one or more events fails. https://developer.wordpress.org/reference/hooks/pre_clear_scheduled_hook/#description

Thus an INT return value indicates that there is some wp-cron replacement running. Thus the Log message is misleading.

This kind of has to be considered for all schedule related hooks. If an external cron system is running, many filters / actions behave slightly different. This are the relevant hooks: https://developer.wordpress.org/reference/files/wp-includes/cron.php/?post_type%5B%5D=wp-parser-hook

To Reproduce

  1. Install Cavalcade
  2. Call anything unscheduling events like wp_clear_scheduled_hook( 'wp_version_check' );
  3. Check the Logs

Expected behavior One of the following:

  1. Don't log if the return is != null as you cannot be sure that there isn't an external cron replacement
  2. Log according to INT return values that some external cron replacement did something
  3. Check for certain known plugins (like Cavalcade) and log accordingly using an "Alternate WP-Cron Mode"

Environment (please complete the following information):

Best regards,

Jan

Pierre-Lannoy commented 3 years ago

Wow ! Thank you so much for this so well-formed issue :) It must be fixed in the next release. Feel free if you want / have time to submit a PR. If no, I will do it after reviewing all "alternatives" to wp-cron (you're the first one I know who use Cavalcade). Many thanks for all this feedback, it's great help!

JanThiel commented 3 years ago

A well written issue is the least courtesy to make when adding work to someone elses schedule ;-)

To update you on my current knowledge and research:

I cannot give you any promisses on a PR as we are going on vacation next wednesday. I will be (hopefully mostly) offline for two weeks. Yet let's see how far we can come up with a solution till then.

I would propose to add a check that determines, based on coded conditions, whether a wp-cron replacement is running or not. For now, that would only check for Cavalcade. When other systems raise the need to be added, it can be done easily.

I would then add an If block around the add_filter / add_action calls registering the listeners. If a replacement is detected, then seperate functions should be used. They could be identified by a suffix like <hook-name>_external_cron => pre_clear_scheduled_hook_external_cron. Those functions could then implement the alternate mode as described in the docs.

What do you think?

Pierre-Lannoy commented 2 years ago

Hello Jan!

I've made a new listener specifically for Cavalcade as it was too specific to be managed by core listener. Cavalcade events will appear as "library" class events in DecaLog 3.3.0.

I would like to thank you for the invaluable help you give on this issue. Without it, I could not have done this. 🥇

I'm closing this issue, even if 3.0.0 is not yet released... Don't hesitate to re-open if needed.

Thanks again.

JanThiel commented 2 years ago

Thanks @Pierre-Lannoy! One remark: Shouldn't the default listeners be de-registered? Wouldn't both listeners fire now?

Pierre-Lannoy commented 2 years ago

Hello Jan! Yes, the default one is not registered when Cavalcade is detected ;) See https://github.com/Pierre-Lannoy/wp-decalog/blob/6565218ef5dd3bb0f26ad4015a7b432aee916994/includes/listeners/class-corelistener.php#L186

JanThiel commented 2 years ago

Ah, that code was not in the referenced commit 😊 Thanks for pointing it out to me ☺️