-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature purge on cron #504
base: master
Are you sure you want to change the base?
Conversation
All commented out code should probably be removed before ready for review? |
Will remove, sorry about that. In regard to a separate pull request that addresses the 1200 urls per 5 minutes: The method purgeCacheByRelevantURLs is problematic as written because:
What do you think about splitting that method into two - one method that gets related urls, and another method that actually does the zonePurgeFiles? This would allow us to implement adherence to the API limit in the purging function |
Which PR is this? I haven't paid attention maybe, but I didn't even know there was some kind of 1200 / 5min limit. |
PR doesn't yet exist, mainly asking for guidance in regard to direction before actually doing the work and testing. Here is the general API rate limit documentation: https://developers.cloudflare.com/fundamentals/api/reference/limits/ I'll see if I can get verification from support about a rate limit regarding paid purge by url. |
I think this is a good use case for leveraging Cloudflare Queues to handle purging of the cache. WordPress would send the URLs to a queue which would be responsible for purging the cache via a Worker. Since it's a queue it would reduce the risk of hitting any API limits. This would introduce additional setup steps for users, so having it as optional would be great. It would be really useful for enterprise sites to offload the heavy lifting to Queues and Workers. Thoughts? |
I like the idea. I'll look more at the queue documentation as I'm not familiar. I see that it requires a worker addition to the plan which is fine for most cases, but maybe the cron rate limited version is still valuable for free plan users. The documentation has since changed and I can't seem to find the rate limit I had seen previously. Can you confirm there is a rate limit for purge by URL requests? |
@midweste On this page - https://developers.cloudflare.com/cache/how-to/purge-cache/ I see the following mentioned -
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Any update on this? Saving any post takes 2-3 seconds due to CF\WordPress\Hooks::purgeCacheOnPostStatusChange function. This is very frustrating. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Has this been addressed somewhere else or should it stay open? |
It should stay open, it's not addressed anywhere. |
The PR as is works as expected, however, to improve on it I was looking for guidance in regard to the existing method purgeCacheByRelevantURLs Cloudflare-WordPress/src/WordPress/Hooks.php Line 140 in dd13e15
Thanks! |
Maybe cronPurgeQueue() function can take care of this, so it purges max 1000 URLs of an array and leave the rest for the next cron job? Current limit: |
The cloudflare_related_urls in cronPurgeQueue are just post_ids that have yet to determine their related urls, therefore you unfortunately cannot determine how many related urls are going to be added for every post_id you feed the purge method because they compute at run time. That's the main issue with the method both computing the related urls AND purging in one step. Also, there is no proper return so you can't determine even how many were purged in the previous request In the current state of this method, you could feed a single post_id that has 2000 related urls and you would never know that the remaining 1000 urls were not purged |
So actually there is no other way but to split purgeCacheByRelevantURLs? |
Just some thoughts.
|
We can at least agree it seems that purgeCacheByRelevantURLs should break out the part that calculated related urls |
All this said though, I don't see any reason why this PR can't be merged in. All the things we are talking about are different PRs |
@aseure please advise on this. |
Yes.
Yes, and ultimately this should be discussed in an issue and from there a PR should be created. So I'll stop posting here. |
Would be glad to put more pull requests in, but not if maintainers do not respond or are not interested in these improvements |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Version 4.12.8 still lacks this feature. |
This is why I'm not bothering to put any work in here... year and a half later, still nothing. Maybe it's time to fork the repo to a community version. It's really disappointing a company as big as Cloudflare won't maintain their plugin. |
@aseure @jacobbednarz could you please look into this issue? Our editors are quite frustrated with the slowness. Thanks! |
I stop using this plugin completely, I think this plugin was just a checkbox on a todo list more than an actual project internally in Cloudflare. |
With the limitations and long run time of clearing related urls, and given the delay of clearing the edge caches, I've added the ability to purge aggregated entries on cron
By default, the existing behavior is maintained if the 'cloudflare_purge_on_cron' is not returning true