-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle][HttpKernel] Move httpkernel pass #22620
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
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.
AddCacheWarmerPassTest should be marked as legacy and copied to the component.
Also deprecations are missing (@trigger_error(...)
*/ | ||
public function process(ContainerBuilder $container) | ||
{ | ||
if (!$container->hasDefinition('cache_clearer')) { |
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.
same as for your other PR, target service id + collected tag name should be configurable through the constructor, defaulting to the ones used in frameworkbundle.
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.
Most comments done on #22619 also apply here
|
||
/** | ||
* Registers the cache clearers. | ||
* | ||
* @author Dustin Dobervich <ddobervich@gmail.com> | ||
*/ | ||
class AddCacheClearerPass implements CompilerPassInterface | ||
class AddCacheClearerPass extends BaseAddCacheClearerPass |
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.
Class should be deprecated
|
||
/** | ||
* Registers the cache warmers. | ||
* | ||
* @author Fabien Potencier <fabien@symfony.com> | ||
*/ | ||
class AddCacheWarmerPass implements CompilerPassInterface | ||
class AddCacheWarmerPass extends BaseAddCacheWarmerPass |
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.
same here
@lepiaf The 3.4 branch has been created and is the one that this PR should target (changing the based branch and rebase). |
You should also rebase instead of merging. |
@@ -42,7 +42,7 @@ | |||
}, | |||
"conflict": { | |||
"symfony/config": "<2.8", | |||
"symfony/dependency-injection": "<3.3", |
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.
the changes on this file should be reverted. You need DI 3.3+ only
This makes tests fail for now. |
@chalasr sure, I will do it now |
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.
Build failure on deps=high seems normal, the bundle currently registers the passes only if they exist (should always be) but as they don't exist on 4.0, they're just not so cache pool clearers aren't injected in the cache_clearer
service.
use PriorityTaggedServiceTrait; | ||
|
||
private $cacheWarmerId; | ||
private $kernelCacheWarmerId; |
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.
cacheWarmerTag
?
class AddCacheClearerPass implements CompilerPassInterface | ||
{ | ||
private $cacheClearerId; | ||
private $kernelCacheClearerId; |
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.
cacheClearerTag
?
@@ -95,8 +95,8 @@ public function build(ContainerBuilder $container) | |||
$this->addCompilerPassIfExists($container, AddConsoleCommandPass::class); | |||
$container->addCompilerPass(new TranslatorPass()); | |||
$container->addCompilerPass(new LoggingTranslatorPass()); | |||
$container->addCompilerPass(new AddCacheWarmerPass()); | |||
$container->addCompilerPass(new AddCacheClearerPass()); | |||
$this->addCompilerPassIfExists($container, AddCacheWarmerPass::class); |
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.
No need for using addCompilerPassIfExists()
because http-kernel
is an hard dependency, addCompilerPass()
should be used directly, that will make the build failure more clear also (fail on unexisting class until merged up to master)
Thank you @lepiaf. |
…piaf) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Move httpkernel pass | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | part of #21284 | License | MIT | Doc PR | n/a Move addcachearmer, addcacheclearer compiler pass to httpkernel Commits ------- 83727c7 [FrameworkBundle][HttpKernel] Move addcachearmer, addcacheclearer compiler pass
Move addcachearmer, addcacheclearer compiler pass to httpkernel