Add kill switch for cron. #76
Conversation
When initially deploying this plugin, hundreds or thousands of cron event can be created to generate site maps over time. While this may be desirable for some, others may want to generate the sitemaps manually with WP CLI. In order to do this without the interference of cron, there needs to be a way to shut down the cron events temporarily. This PR does just that. `wp msm-sitemaps cron --disable` is introduced to set a option variable. When this option variable is set, the cron file is not loaded and thus the actions tied to the cron events won't execute. Additionally, the command removes all of the `msm_cron_generate_sitemap_for_year_month_day` events from the cron event list. This is more or less for cleaning up that list for sanity. Once a developer wants to enable cron events again, she can do so with `wp msm-sitemaps cron --enable`. This removes the option that blocks the cron file from loading. While I've explained this PR in the context of a specific use case, this cron disabling feature is also useful if a developer deploys this plugin and needs a quick way to kill the cron actions (perhaps the sitemap generation has caused too much load). Note that this does not affect the `msm_cron_update_sitemap` which is part of the main plugin file. This is because it is not part of the cron file. Additionally, this cron action is harmless without the accompanying cron file.
I don't know if this is desirable for anyone, actually, other than small sites (which are probably not a good target for this plugin). Maybe we should have cron disabled by default? (We don't even actually use the cron generator on wp.com; we funnel all the sitemap generation to async jobs.) |
Probably true. My biggest concern is back compat. Ideally, the plugin wouldn't do anything initially. You would have to take an action to start building the sitemaps. Additionally, there would be easy ways to stop it. In my testing, the
I think that this might be an interesting idea. Perhaps it is disabled initially and as soon as the full sitemap is generated, crons can be registered. This assumes that people will have an alternative way to generating the initial site map. For larger sites, I don't think this would be an issue. For smaller sites, it definitely would be. |
Mo alluded to this, and I agree, that it should be fine to target big sites. The background for this plugin is to provide compressive sitemaps for (large) sites where other plugins otherwise can't because of the size. |
WFM |
|
+1 for removing the initial cron by default. When this was initially developed, the cli wasn't present but can't really think of any reason to keep that option given the context of targeting large sites. We'd just need to make sure it was clear that to generate the initial sitemap requires additional actions, whether in activation or readme docs (prob both). |
| $hook = 'msm_cron_generate_sitemap_for_year_month_day'; | ||
|
|
||
| foreach ( $crons as $timestamp => $cron ) { | ||
| foreach ( $cron[ $hook ] as $key => $data ) { |
There was a problem hiding this comment.
This can cause an undefined index notice, yet it still seems to work. Just adding this note here so I remember to look into it and fix.
trim() will trim both sides of a string, when the intention is to only trim the right side. Thus, rtrim() is the correct function to use. This caused a misspelled word in the WP CLI output.
Before using the cron hook value, ensure that the index is available in the array. This avoids an undefined index error.
The function is called statically and throws a strict standards error because it is not static. This commit makes it static and avoids the error.
When initially deploying this plugin, hundreds or thousands of cron
event can be created to generate site maps over time. While this may be
desirable for some, others may want to generate the sitemaps manually
with WP CLI.
In order to do this without the interference of cron, there needs to be
a way to shut down the cron events temporarily. This PR does just that.
wp msm-sitemaps cron --disableis introduced to set a option variable.When this option variable is set, the cron file is not loaded and thus
the actions tied to the cron events won't execute. Additionally, the
command removes all of the
msm_cron_generate_sitemap_for_year_month_dayevents from the cron event list. This is more or less for cleaning up
that list for sanity.
Once a developer wants to enable cron events again, she can do so with
wp msm-sitemaps cron --enable. This removes the option that blocks thecron file from loading.
While I've explained this PR in the context of a specific use case, this
cron disabling feature is also useful if a developer deploys this plugin
and needs a quick way to kill the cron actions (perhaps the sitemap
generation has caused too much load).
Note that this does not affect the
msm_cron_update_sitemapwhich ispart of the main plugin file. This is because it is not part of the cron
file. Additionally, this cron action is harmless without the
accompanying cron file.