Skip to content

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

Follows #52962 and builds on the semantics of #57553.

@@ -84,14 +84,12 @@ public function getRouteCollection(): RouteCollection

public function warmUp(string $cacheDir, ?string $buildDir = null): array
{
if (!$buildDir) {
return [];
if (null === $currentDir = $this->getOption('cache_dir')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the CacheWarmer not registered when the caching is disabled for the router ?

And with this change, the Router cache is generated once at build-time and once again if you run cache:warmup, right ?
If you run cache:warmup right after your deploy (thus, on a read-only buildDir), this will either fail or you regenerate the router cache in the cache_dir, which won't be used at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no it's not disabled if you set the router.cache_dir to null (which is the only way since the deprecation of the config option)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run cache:warmup right after your deploy (thus, on a read-only buildDir), this will either fail or you regenerate the router cache in the cache_dir, which won't be used at all.

then, $buildDir will be null so that RouterCacheWarmer won't call this warmup method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then, $buildDir will be null so that RouterCacheWarmer won't call this warmup method

Ah right, I got caught by the double warming layer of the Router... 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants