Skip to content

[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

Merged
merged 1 commit into from
Jul 6, 2017
Merged

[FrameworkBundle][HttpKernel] Move httpkernel pass #22620

merged 1 commit into from
Jul 6, 2017

Conversation

lepiaf
Copy link
Contributor

@lepiaf lepiaf commented May 3, 2017

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

Copy link
Member

@chalasr chalasr left a 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')) {
Copy link
Member

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.

@stof stof added this to the 3.4 milestone May 3, 2017
Copy link
Member

@stof stof left a 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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

same here

@lepiaf lepiaf changed the title [WIP][FrameworkBundle][HttpKernel] Move httpkernel pass [FrameworkBundle][HttpKernel] Move httpkernel pass May 16, 2017
@chalasr
Copy link
Member

chalasr commented May 18, 2017

@lepiaf The 3.4 branch has been created and is the one that this PR should target (changing the based branch and rebase).

@fabpot
Copy link
Member

fabpot commented May 19, 2017

You should also rebase instead of merging.

@lepiaf lepiaf changed the base branch from master to 3.4 May 22, 2017 11:08
@@ -42,7 +42,7 @@
},
"conflict": {
"symfony/config": "<2.8",
"symfony/dependency-injection": "<3.3",
Copy link
Member

@chalasr chalasr May 25, 2017

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

@nicolas-grekas
Copy link
Member

This makes tests fail for now.

@chalasr
Copy link
Member

chalasr commented Jun 21, 2017

@lepiaf Would you mind rebasing so we can look if tests are still failing (fixing them if needed)? Otherwise I can take this over so we can close #21284

@lepiaf
Copy link
Contributor Author

lepiaf commented Jun 21, 2017

@chalasr sure, I will do it now

Copy link
Member

@chalasr chalasr left a 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;
Copy link
Member

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;
Copy link
Member

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);
Copy link
Member

@chalasr chalasr Jun 21, 2017

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)

@fabpot
Copy link
Member

fabpot commented Jul 6, 2017

Thank you @lepiaf.

@fabpot fabpot merged commit 83727c7 into symfony:3.4 Jul 6, 2017
fabpot added a commit that referenced this pull request Jul 6, 2017
…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
This was referenced Oct 18, 2017
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.

6 participants