-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle][HttpKernel] Introduce $buildDir
argument to WarmableInterface::warmup
to warm read-only artefacts in build_dir
#50391
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
Conversation
a575a7e
to
9890af2
Compare
28b878f
to
4b9ed49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on Slack, I'd suggest adding a string $buildDir = null
argument to the existing warmUp
method (using a commented argument for BC). We'd pass null
when only warming up, and the build-dir when building.
92ba4e8
to
d62544e
Compare
build_dir
$buildDir
argument to WarmableInterface::warmup
to warm read-only artefacts in build_dir
03a16b2
to
3b80633
Compare
931cc08
to
dc5d8f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some comment, and my review as a patch :)
Now that we have this, I'm wondering about each warmers: are they doing something for build-time or compilte-time? what's their purpose, etc; E.g CompiledClassMetadataFactory
added in #29117 by @fbourigault is never wired? We should likely investigate this in a follow up PR this and the others too.
diff --git a/src/Symfony/Bridge/Doctrine/CacheWarmer/ProxyCacheWarmer.php b/src/Symfony/Bridge/Doctrine/CacheWarmer/ProxyCacheWarmer.php
index b148b9347b..baedf9c033 100644
--- a/src/Symfony/Bridge/Doctrine/CacheWarmer/ProxyCacheWarmer.php
+++ b/src/Symfony/Bridge/Doctrine/CacheWarmer/ProxyCacheWarmer.php
@@ -40,9 +40,7 @@ class ProxyCacheWarmer implements CacheWarmerInterface
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[] A list of files to preload on PHP 7.4+
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php
index 49fd7f035d..98c281276b 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php
@@ -35,17 +35,16 @@ abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[] A list of classes to preload on PHP 7.4+
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
+ $buildDir = 1 < \func_num_args() ? func_get_arg(1) : null;
$arrayAdapter = new ArrayAdapter();
spl_autoload_register([ClassExistenceResource::class, 'throwOnRequiredClass']);
try {
- if (!$this->doWarmUp($cacheDir, $arrayAdapter)) {
+ if (!$this->doWarmUp($cacheDir, $arrayAdapter, $buildDir)) {
return [];
}
} finally {
@@ -80,7 +79,9 @@ abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface
}
/**
+ * @param string|null $buildDir
+ *
* @return bool false if there is nothing to warm-up
*/
- abstract protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter): bool;
+ abstract protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter /* , string $buildDir = null */): bool;
}
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php
index 279dc4ec9e..20533bb60e 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php
@@ -44,7 +44,10 @@ class AnnotationsCacheWarmer extends AbstractPhpFileCacheWarmer
parent::__construct($phpArrayFile);
}
- protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter): bool
+ /**
+ * @param string|null $buildDir
+ */
+ protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter /* , string $buildDir = null */): bool
{
$annotatedClassPatterns = $cacheDir.'/annotations.map';
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/CachePoolClearerCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/CachePoolClearerCacheWarmer.php
index 7cd02d1cc5..7498a82d1f 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/CachePoolClearerCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/CachePoolClearerCacheWarmer.php
@@ -37,10 +37,7 @@ final class CachePoolClearerCacheWarmer implements CacheWarmerInterface
$this->pools = $pools;
}
- /**
- * @return string[]
- */
- public function warmUp(string $cacheDirectory, string $buildDir = null): array
+ public function warmUp(string $cacheDir, string $buildDir = null): array
{
foreach ($this->pools as $pool) {
if ($this->poolClearer->hasPool($pool)) {
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php
index 12e24bd77c..225fea11b7 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php
@@ -38,11 +38,13 @@ class ConfigBuilderCacheWarmer implements CacheWarmerInterface
}
/**
- * @return string[]
+ * @param string|null $buildDir
*/
- public function warmUp(string $cacheDir, string $buildDir = null): array
+ public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
- if (null === $buildDir) {
+ $buildDir = 1 < \func_num_args() ? func_get_arg(1) : null;
+
+ if (!$buildDir) {
return [];
}
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/RouterCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/RouterCacheWarmer.php
index 82a16c01d3..2af9a2fe80 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/RouterCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/RouterCacheWarmer.php
@@ -34,16 +34,12 @@ class RouterCacheWarmer implements CacheWarmerInterface, ServiceSubscriberInterf
$this->container = $container;
}
- /**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- */
- public function warmUp(string $cacheDir /* , string $buildDir = null */): array
+ public function warmUp(string $cacheDir, string $buildDir = null): array
{
$router = $this->container->get('router');
if ($router instanceof WarmableInterface) {
- return (array) $router->warmUp($cacheDir);
+ return (array) $router->warmUp($cacheDir, $buildDir);
}
throw new \LogicException(sprintf('The router "%s" cannot be warmed up because it does not implement "%s".', get_debug_type($router), WarmableInterface::class));
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php
index 35595474ef..b47a48ce69 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php
@@ -39,7 +39,10 @@ class SerializerCacheWarmer extends AbstractPhpFileCacheWarmer
$this->loaders = $loaders;
}
- protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter): bool
+ /**
+ * @param string|null $buildDir
+ */
+ protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter /* , string $buildDir = null */): bool
{
if (!$this->loaders) {
return true;
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php
index d3abbaca5b..39b1444b0e 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php
@@ -34,16 +34,16 @@ class TranslationsCacheWarmer implements CacheWarmerInterface, ServiceSubscriber
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[]
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
$this->translator ??= $this->container->get('translator');
if ($this->translator instanceof WarmableInterface) {
- return (array) $this->translator->warmUp($cacheDir);
+ $buildDir = 1 < \func_num_args() ? func_get_arg(1) : null;
+
+ return (array) $this->translator->warmUp($cacheDir, $buildDir);
}
return [];
diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php
index 3119c9942a..224e90985e 100644
--- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php
@@ -39,7 +39,10 @@ class ValidatorCacheWarmer extends AbstractPhpFileCacheWarmer
$this->validatorBuilder = $validatorBuilder;
}
- protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter): bool
+ /**
+ * @param string|null $buildDir
+ */
+ protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter /* , string $buildDir = null */): bool
{
$loaders = $this->validatorBuilder->getLoaders();
$metadataFactory = new LazyLoadingMetadataFactory(new LoaderChain($loaders), $arrayAdapter);
diff --git a/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php b/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
index fe89fef6d6..3367ecec2b 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
@@ -81,9 +81,7 @@ class Router extends BaseRouter implements WarmableInterface, ServiceSubscriberI
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[] A list of classes to preload on PHP 7.4+
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
diff --git a/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php b/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php
index 70f5874307..04b56308f3 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php
@@ -95,9 +95,7 @@ class Translator extends BaseTranslator implements WarmableInterface
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[]
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
diff --git a/src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php b/src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php
index c63c973e41..1cbb681c2d 100644
--- a/src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php
+++ b/src/Symfony/Bundle/SecurityBundle/CacheWarmer/ExpressionCacheWarmer.php
@@ -35,9 +35,7 @@ class ExpressionCacheWarmer implements CacheWarmerInterface
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[]
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
diff --git a/src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php b/src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php
index 1adabe6636..2ab801130b 100644
--- a/src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php
+++ b/src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php
@@ -36,9 +36,7 @@ class TemplateCacheWarmer implements CacheWarmerInterface, ServiceSubscriberInte
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[] A list of template files to preload on PHP 7.4+
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
diff --git a/src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php b/src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php
index 638d9ad2c4..a672956e0f 100644
--- a/src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php
+++ b/src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php
@@ -48,8 +48,17 @@ class CacheWarmerAggregate implements CacheWarmerInterface
$this->onlyOptionalsEnabled = $this->optionalsEnabled = true;
}
- public function warmUp(string $cacheDir, string $buildDir = null, SymfonyStyle $io = null): array
+ /**
+ * @param string|null $buildDir
+ */
+ public function warmUp(string $cacheDir, string|SymfonyStyle $buildDir = null, SymfonyStyle $io = null): array
{
+ if ($buildDir instanceof SymfonyStyle) {
+ trigger_deprecation('symfony/http-kernel', '6.4', 'Passing a "%s" as second argument of "%s()" is deprecated, pass it as third argument instead, after the build directory.', SymfonyStyle::class, __METHOD__);
+ $io = $buildDir;
+ $buildDir = null;
+ }
+
if ($collectDeprecations = $this->debug && !\defined('PHPUNIT_COMPOSER_INSTALL')) {
$collectedLogs = [];
$previousHandler = set_error_handler(function ($type, $message, $file, $line) use (&$collectedLogs, &$previousHandler) {
@@ -96,8 +105,8 @@ class CacheWarmerAggregate implements CacheWarmerInterface
}
$start = microtime(true);
- foreach ((array) $warmer->warmUp($cacheDir, null === $buildDir || $warmer->isOptional() ? null : $buildDir) as $item) {
- if (is_dir($item) || (str_starts_with($item, \dirname($cacheDir)) && !is_file($item))) {
+ foreach ((array) $warmer->warmUp($cacheDir, $buildDir) as $item) {
+ if (is_dir($item) || (str_starts_with($item, \dirname($cacheDir)) && !is_file($item)) || ($buildDir && str_starts_with($item, \dirname($buildDir)) && !is_file($item))) {
throw new \LogicException(sprintf('"%s::warmUp()" should return a list of files or classes but "%s" is none of them.', $warmer::class, $item));
}
$preload[] = $item;
diff --git a/src/Symfony/Component/HttpKernel/CacheWarmer/WarmableInterface.php b/src/Symfony/Component/HttpKernel/CacheWarmer/WarmableInterface.php
index 799705e42d..cd051b1add 100644
--- a/src/Symfony/Component/HttpKernel/CacheWarmer/WarmableInterface.php
+++ b/src/Symfony/Component/HttpKernel/CacheWarmer/WarmableInterface.php
@@ -21,8 +21,8 @@ interface WarmableInterface
/**
* Warms up the cache.
*
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
+ * @param string $cacheDir Where warm-up artifacts should be stored
+ * @param string|null $buildDir Where read-only artifacts should go; null when called after compile-time
*
* @return string[] A list of classes or files to preload on PHP 7.4+
*/
diff --git a/src/Symfony/Component/Translation/DataCollectorTranslator.php b/src/Symfony/Component/Translation/DataCollectorTranslator.php
index bdd0e6ce4d..785e5b89ed 100644
--- a/src/Symfony/Component/Translation/DataCollectorTranslator.php
+++ b/src/Symfony/Component/Translation/DataCollectorTranslator.php
@@ -72,14 +72,14 @@ class DataCollectorTranslator implements TranslatorInterface, TranslatorBagInter
}
/**
- * @param string|null $buildDir directory to put generated source code that can be marked as read-only at runtime.
- * The directory is not provided when only cache_dir should be warmed up.
- * @return string[]
+ * @param string|null $buildDir
*/
public function warmUp(string $cacheDir /* , string $buildDir = null */): array
{
+ $buildDir = 1 < \func_num_args() ? func_get_arg(1) : null;
+
if ($this->translator instanceof WarmableInterface) {
- return (array) $this->translator->warmUp($cacheDir);
+ return (array) $this->translator->warmUp($cacheDir, $buildDir);
}
return [];
src/Symfony/Component/HttpKernel/CacheWarmer/CacheWarmerAggregate.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ConfigBuilderCacheWarmer.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rebase
src/Symfony/Bundle/FrameworkBundle/Command/CacheWarmupCommand.php
Outdated
Show resolved
Hide resolved
38fa985
to
d643c9f
Compare
…ableInterface::warmup` to warm read-only artefacts in `build_dir`
d643c9f
to
8b604ff
Compare
Thank you @Okhoshi. |
…bois) This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] Fix CacheWarmerAggregateTest | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT #50391 seems to bring broken tests. Particularly, the `testWarmupOnOptionalWarmerPassBuildDir` test doesn't seem relevant given the actual code of `CacheWarmerAggregate`. Indeed, nothing in the code is setting `buildDir` to null when using an optional warmer. I suggest a changes to fix this test on 6.4. Also for the first one, given `optionalsEnabled` is enabled, `isOptional` is never called. I suggest to remove this expectation. Commits ------- 945d5cd [HttpKernel] Fix CacheWarmerAggregateTest
…bois) This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] Fix CacheWarmerAggregateTest | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT symfony/symfony#50391 seems to bring broken tests. Particularly, the `testWarmupOnOptionalWarmerPassBuildDir` test doesn't seem relevant given the actual code of `CacheWarmerAggregate`. Indeed, nothing in the code is setting `buildDir` to null when using an optional warmer. I suggest a changes to fix this test on 6.4. Also for the first one, given `optionalsEnabled` is enabled, `isOptional` is never called. I suggest to remove this expectation. Commits ------- 945d5cd5e1 [HttpKernel] Fix CacheWarmerAggregateTest
…el.build_dir` (Okhoshi) This PR was squashed before being merged into the 7.1 branch. Discussion ---------- [FrameworkBundle] Move Router cache directory to `kernel.build_dir` | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | none | License | MIT Follow up to #50391, set up Router cache directory to `kernel.build_dir` instead of `kernel.cache_dir` by default, and only warm the cache on read-only resources phase. #SymfonyHackday Commits ------- 1f031f8 [FrameworkBundle] Move Router cache directory to `kernel.build_dir`
…-optional (nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] `ConfigBuilderCacheWarmer` should be non-optional | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #53496 | License | MIT Replaces #53512 This became apparent after #50391, where the warmer is run only when compiling the container. Before, it didn't really matter. Commits ------- 8bd2ff8 [FrameworkBundle] ConfigBuilderCacheWarmer should be non-optional
See #50357 for the details and the reproduction steps. In this PR, I also moved
ConfigBuilderCacheWarmer
to use thebuildDir
argument.