Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle][HttpKernel] Introduce$buildDir
argument toWarmableInterface::warmup
to warm read-only artefacts inbuild_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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a575a7e
to9890af2
Compare28b878f
to4b9ed49
Compare
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 astring $buildDir = null
argument to the existingwarmUp
method (using a commented argument for BC). We'd passnull
when only warming up, and the build-dir when building.
92ba4e8
tod62544e
Comparebuild_dir
$buildDir
argument toWarmableInterface::warmup
to warm read-only artefacts inbuild_dir
03a16b2
to3b80633
Compare931cc08
todc5d8f9
CompareThere 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.gCompiledClassMetadataFactory
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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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.phpindex 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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
38fa985
tod643c9f
Compare…ableInterface::warmup` to warm read-only artefacts in `build_dir`
d643c9f
to8b604ff
CompareThank 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 | MITsymfony/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 | MITFollow 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.#SymfonyHackdayCommits-------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 | MITReplaces#53512This 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
…ates known at build time (Okhoshi)This PR was merged into the 7.3 branch.Discussion----------[TwigBundle] Use `kernel.build_dir` to store the templates known at build time| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | yes| Deprecations? | no| Issues | none| License | MITFollow up to#50391, set up the Twig `TemplateCacheWarmer` to use the `kernel.build_dir` instead of `kernel.cache_dir`. A new argument is added to its constructor to specify the directory to use for the cache.Commits-------0f841d2 [TwigBundle] Use `kernel.build_dir` to store the templates known at build time
…ializeCacheWarmer` use `kernel.build_dir` instead of `kernel.cache_dir` (Okhoshi)This PR was squashed before being merged into the 7.3 branch.Discussion----------[FrameworkBundle] Make `ValidatorCacheWarmer` and `SerializeCacheWarmer` use `kernel.build_dir` instead of `kernel.cache_dir`| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | yes| Deprecations? | no| Issues | none| License | MITFollow up to#50391, set up the `ValidatorCacheWarmer` and `SerializerCacheWarmer` to use the `kernel.build_dir` instead of `kernel.cache_dir`. The value conveyed by their constructor parameter has been changed to prevent developers to configure the cache file outside of the build_dir.`AbstractPhpFileCacheWarmer` has been reworked a bit too, to reduce the risk of misconfiguration.#SymfonyHackdayCommits-------dc59817 [FrameworkBundle] Make `ValidatorCacheWarmer` and `SerializeCacheWarmer` use `kernel.build_dir` instead of `kernel.cache_dir`
Uh oh!
There was an error while loading.Please reload this page.
See#50357 for the details and the reproduction steps. In this PR, I also moved
ConfigBuilderCacheWarmer
to use thebuildDir
argument.