Initial functionality of the static cache warmer addon #1
Conversation
| { | ||
| public function warm(string $url): void | ||
| { | ||
| Http::get($url); |
There was a problem hiding this comment.
Is there any "internal" way to achieve the warming? This way may result in the warmer being served stale cache if e.g. Cloudflare does static caching
There was a problem hiding this comment.
Is there a possibility this happens? Yes. Should it happen? No really.
If using cloudflare caching we recommend using the https://github.com/justbetter/statamic-cloudflare-purge addon wich will also clear the cloudflare cache. It could possibly happen that cloudflare cache is purged after this action is ran, maybe we need something for that.
There was a problem hiding this comment.
I'm not sure about the combination of WarmsUrl and WarmsUrls. Personally I'd have allowed the WarmsUrls to take a string or array of strings to be able to do both singular and multiple URLs---having both of these seems excessive.
The fact that WarmsUrls doesn't take any parameters also doesn't sit right with me. I can tell it warms all URLs, but the name (especially when combined with WarmsUrl) doesn't make sense to me.
I also think the names are too similar like this. I'm already having to double check every time I see one of these two in this PR, and I could easily see a small mistake using the wrong class creeping in.
There was a problem hiding this comment.
First issue we plan to address in the future. But the fact that the names are too similar is just you who needs to focus 😛
| public function handle(WarmsUrl $warmsEntry): void | ||
| { | ||
| $warmsEntry->warm($this->url); |
There was a problem hiding this comment.
| public function handle(WarmsUrl $warmsEntry): void | |
| { | |
| $warmsEntry->warm($this->url); | |
| public function handle(WarmsUrl $warmsUrl): void | |
| { | |
| $warmsUrl->warm($this->url); |
Could also consider instantiating the WarmsUrl class in the constructor, I'm not sure what the best approach for that would be, or whether it even matters.
| { | ||
| $enabled = config('statamic.static_caching.strategy'); | ||
|
|
||
| if ($enabled !== 'full') { |
There was a problem hiding this comment.
I don't think this should be hard-coded here, as the caching strategy could be anything.
There was a problem hiding this comment.
It could be anything, but we only want this to run if it's exactly full
There was a problem hiding this comment.
That sounds strange to me, people could configure a bunch of other strategies that would need cache warming. Explicitly only listening to "full" means this package would be useless for them.
There was a problem hiding this comment.
Note that this event alone is not enough to catch every instance of full cache clears; it is only fired specifically when you click the button in the backend of Statamic. If you have a custom invalidator that calls $this->cacher->flush() (for example, on global set changes, as we do with rapidez/statamic), this event does not get called. It's also possible to instantiate the default statamic invalidator with the all rule, which will always clear everything whenever you do anything.
Have a look at this package which also has to deal with this problem. Might be useful to implement something similar here.
Maybe we could even combine these two packages into one big "static cache extended" package?
There was a problem hiding this comment.
Note that this event alone is not enough to catch every instance of full cache clears; it is only fired specifically when you click the button in the backend of Statamic.
That's the point
But yes i do think we need to have some extensibility here to have it work with the cloudflare package. I don't think we want it to be in a grouped package because not everyone uses cloudflare
Co-authored-by: Jade Geels <[email protected]>
…cache-warmer into feature/init
| foreach ($entries as $entry) { | ||
| WarmUrlJob::dispatch($entry->absoluteUrl()); | ||
| } |
There was a problem hiding this comment.
Maybe nice to add this as a batch, so if one fails the whole batch can be stopped? https://laravel.com/docs/12.x/queues#job-batching
There was a problem hiding this comment.
But if one job fails we do want to warm the other urls right?
There was a problem hiding this comment.
Yeah but if it's a misconfiguration and you notice half way you can simply cancel the batch
Internal reference: JBST-256