[Kernel] Move the cache compiler passes into the Kernel component #20250

Taluu · 2016-10-19T16:15:18Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	--
License	MIT
Doc PR	--

Looking at the passes, for one who doesn't want to use the Framework Bundle (like me, in a minimal app) but has still need some accesses to part of it (such as cache clearers / cache warmers), and as these two passes are not tied to the framwork bundle (but are to the kernel component), I thought it would be better to move these two passes in the Kernel.

Not sure what branch I should target, so I targetted master... And I am not sure if this is a bc break, as these two classes are not final but not tagged @api

ro0NL · 2016-10-19T17:51:57Z

@Taluu the passes depend on definitions, defined in the frameworkbundle, hence these are tight to the frameworkbundle as well.

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml#L39

ie. the passes are still unuseable (as is) with just the HttpKernel+DI component.

edit: point is; imo. these passes are designed in scope of the frameworkbundle. Although they could be used standalone (and i understand in that case you dont want to depend on the frameworkbundle, but rather the httpkernel component) they are not designed from a component point of view (eg. the tag name, definitions id's are hardcoded). Ie. imo the added value is minimal then to put it in a component.

stof · 2016-10-19T17:59:21Z

@ro0NL any time we moved passes to the components, we made the id and tag names configurable in the constructor (defaulting to the names used in the fullstack).

stof · 2016-10-19T18:00:29Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheClearerPass.php

@@ -9,7 +9,7 @@
 * file that was distributed with this source code.
 */

-namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;
+namespace Symfony\Component\HttpKernel\DependencyInjection;


yo cannot just move a class. It is a BC break (as it deletes the old class). You need to keep the class in FrameworkBundle (making it extend the new one to avoid duplication) but deprecating it

Yes, I actually wasn't sure if this was really needed, but I'll do that then

ro0NL · 2016-10-19T18:01:21Z

@stof dont get me wrong, 👍 for moving logic to components.. but not by just renaming :)

ro0NL · 2016-10-19T18:46:00Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheWarmerPass.php

+    private $tag;
+    private $definition;
+
+    public function __construct($definition = null, $tag = null)


What about just __construct($definition = 'cache_warmer', $tag = 'kernel.cache_warmer'), that would be fine for 3.2 as well right.. (ref)

Yes, but I wanted to add a deprecation on the fact that one should add arguments

(So that on 4.0 we have required args)

What about making them required now, and call parent::__construct from the legacy class?

fixed, see @nicolas-grekas' comments. :}

nicolas-grekas · 2016-10-19T18:46:25Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheClearerPass.php

-use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
-use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\HttpKernel\DependencyInjection\AddCacheClearerPass as RealAddCacheClearerPass;


Base instead of Real?

I hesitated, but both are fine, so I'll change it

nicolas-grekas · 2016-10-19T18:46:56Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheClearerPass.php

-use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\HttpKernel\DependencyInjection\AddCacheClearerPass as RealAddCacheClearerPass;
+
+@trigger_error('The '.AddCacheClearerPass::class.' class is deprecated since version 3.2 and will be removed in 4.0. Use the '.RealAddCacheClearerPass::class.' class instead.', E_USER_DEPRECATED);

 /**
 * Registers the cache clearers.
 *
 * @author Dustin Dobervich <ddobervich@gmail.com>


missing @deprecated ... annotation

nicolas-grekas · 2016-10-19T18:49:12Z

src/Symfony/Component/HttpKernel/Tests/DependencyInjection/AddCacheClearerPassTest.php

+{
+    /**
+     * @group legacy
+     * @dataProvider nullProvider


no need for the provider, a single and simple use is enough
the @legacy annotation should be moved to the class level

OH, in fact, is this class required at all?

Hum, thought it would be a way to introduce some tests on that class, but I can remove it

nicolas-grekas · 2016-10-19T18:52:10Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheWarmerPass.php

+
+    public function __construct($definition = null, $tag = null)
+    {
+        if (null === $definition || null === $tag) {


you should use func_num_args instead

nicolas-grekas · 2016-10-19T18:54:56Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheClearerPass.php

+        if (null === $definition || null === $tag) {
+            @trigger_error('The '.static::class.' class\' constructor needs two parameters since version 3.2, and will become required in 4.0.', E_USER_DEPRECATED);
+
+            $definition = 'cache_clearer';


these should be the default values in the method signature

Yes, as I just learned that indeed func_num_args returns the number of args given to a function

nicolas-grekas · 2016-10-19T18:56:04Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheClearerPass.php

+class AddCacheClearerPass implements CompilerPassInterface
+{
+    private $tag;
+    private $definition;


serviceName instead of definition?

nicolas-grekas · 2016-10-19T19:03:49Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheClearerPass.php

+
+    public function __construct($serviceName = 'cache_clearer', $tag = 'kernel.cache_clearer')
+    {
+        if (null === $serviceName || null === $tag) {


if (2 > func_num_args()) {

oops, fixed

ro0NL · 2016-10-19T19:04:40Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheWarmerPass.php

+
+    public function __construct($serviceName = 'cache_warmer', $tag = 'kernel.cache_warmer')
+    {
+        if (2 > func_num_args()) {


@Taluu AFAIK the BC promise allows you to add a constructor without mandatory arguments.., ie you may add to the legacy class and call parent::__construct('cache_warmer', 'kernel.cache_warmer'). That would give us required args for the new class already in 3.2... right?

It does work to call new ... without arguments, You'll just have a deprecated error, which is shown in dev env and in the logs AFAIK

With new class i meant Symfony\Component\HttpKernel\DependencyInjection\AddCacheClearerPass (a new class definition). It can have required args, that can be provided by the legacy class (the old class) by adding a constructor...

// Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheClearerPass public function __construct() { parent::__construct('cache_warmer', 'kernel.cache_warmer'); }

ie. why make them required as of 4.0 instead of 3.2.

Yes, I see where you are getting at (it was even one of my first ideas actually), but I don't think it is really useful, and just let the deprecated error be triggered. Especially as it is silenced (with the @ in front of the trigger_error), it will only be logged and that's pretty much it. So you can still continue to use with new ... and without args, you'll just have a little line on your logs. :}

but I don't think it is really useful,

This is part of a component now.. imo. it shoud make no assumptions on naming, hence required args 👍

So you can still continue to use with new ... and without args

Only until 4.0, as we already decided that. Im not sure why we want to bridge that with optional args now (again; for a new class (definition)). It doesnt make sense to me nor does it solve a DX issue, as the old class still works without args.

@ro0NL is right! We don't need this deprecation on the constructor: let's make the new construcor require 2 args, and the constructor of the legacy class set them as required

Fair enough, i'll revert back to the previous idea i had and you gave. :}

ro0NL · 2016-10-19T19:33:17Z

Technically you still need to document the added constructor in the UPGADE file :) But im not sure it really counts for a deprecated class anyway. @nicolas-grekas ?

linaori · 2016-10-20T06:22:36Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheClearerPass.php

+ *
+ * @author Dustin Dobervich <ddobervich@gmail.com>
+ */
+class AddCacheClearerPass implements CompilerPassInterface


Besides of the sort, this pass and the AddCacheWarmerPass are rather identical now (and there's probably more).

A lot of times, a compiler pass for tags is used to collect a bunch of references and that's it. What about a more generic class?

Just a brain fart:

class CollectAndInjectTaggedServicePass implements CompilerPassInterface { const NO_FLAGS = 0; const SORT_BY_PRIORITY = 1; public function __construct($targetService, $serviceArgumentIndex, $tag, $flags = self::NO_FLAGS); }

Hum, then this would need to be in the DependencyInjection component, but I am not sure this should be the goal of this PR (but a later one probably ?)

This PR would introduce 2 new classes in the component while it might be solved with a (re-usable) generic solution, which means those 2 new classes would not be added unless you use them for inheritance templating

@iltar

A lot of times, a compiler pass for tags is used to collect a bunch of references and that's it.

Dont forget the handling of collected references, this is probably the true problem. Currently the passes still make assumptions about what to do with it (pass it as constructor arguments, whereas others add a method call (addSomething(new Ref())), ie. we assume argument index 0 is available on the implementation side, that's why i still think this belongs to the application/framework. Although i can live with it like this in the component as well.

Nevertheless, a generic approach should solve this.

But yeah, i thought about a TagCollectingPass many, many times :)

stof · 2016-10-20T08:45:18Z

src/Symfony/Component/HttpKernel/DependencyInjection/AddCacheWarmerPass.php

+    private $tag;
+    private $serviceName;
+
+    public function __construct($serviceName, $tag)


our other compiler passes in the component don't make these argument required

See the discussion on #20250 (comment)

xabbuh · 2016-10-21T07:14:19Z

You now also need to update the minimum required version of the DependencyInjection component in the HttpKernel composer.json file.

Taluu · 2016-10-24T12:59:00Z

@xabbuh Why ? I am not touching anything relating to DI in the Kernel. Unless you are talking about @iltar's suggestion ?

xabbuh · 2016-10-25T18:30:35Z

@Taluu You are moving compiler passes to the HttpKernel component which make use of a trait that is not available in all versions of the DependencyInjection component (see the failing deps=low build job).

Taluu · 2016-10-26T10:08:56Z

Oh, indeed. I bumped it to 3.2 then, but I'm thinking of applying @iltar's sugggestion maybe (having a generic compiler pass, in DependencyInjection), which would make this bump not necessary,

Status: Needs Work

fabpot · 2017-03-05T20:46:11Z

@Taluu Any news on this one?

Taluu · 2017-03-05T20:48:13Z

I don't know if it was decided or not to have a generic tag compiler pass (if it is the case, then I guess this PR can closed). If not, I just need to rebase I guess, as 3.2 was released some months ago

Taluu · 2017-03-06T13:37:45Z

PR Rebased. I'm going to do the changes suggested by @iltar on another PR, feel free to merge or close it if you don't think it is worth it to have an "inbetween"

fabpot · 2017-03-22T17:37:00Z

@Taluu Closing then, I prefer to merge one PR that solves the issue once and for all instead of having an intermediate solution. Thanks.

carsonbot added the Status: Needs Review label Oct 19, 2016

stof requested changes Oct 19, 2016

View reviewed changes

Taluu force-pushed the move-cache-passes branch 2 times, most recently from 4c56ed0 to 38aa510 Compare October 19, 2016 18:33

ro0NL reviewed Oct 19, 2016

View reviewed changes

nicolas-grekas reviewed Oct 19, 2016

View reviewed changes

Taluu force-pushed the move-cache-passes branch from 38aa510 to 1c4940e Compare October 19, 2016 19:02

nicolas-grekas reviewed Oct 19, 2016

View reviewed changes

Taluu force-pushed the move-cache-passes branch from 1c4940e to edcd138 Compare October 19, 2016 19:04

ro0NL reviewed Oct 19, 2016

View reviewed changes

Taluu force-pushed the move-cache-passes branch 3 times, most recently from 9efdbf6 to b5c3e39 Compare October 19, 2016 19:28

linaori reviewed Oct 20, 2016

View reviewed changes

stof reviewed Oct 20, 2016

View reviewed changes

ro0NL mentioned this pull request Oct 22, 2016

[Console] Move AddConsoleCommandPass from FrameworkBundle to Console. #19443

Merged

carsonbot removed the Status: Needs Review label Oct 26, 2016

carsonbot added the Status: Needs Work label Oct 26, 2016

javiereguiluz added this to the 3.3 milestone Nov 7, 2016

ro0NL mentioned this pull request Jan 14, 2017

[FrameworkBundle] Move compiler passes to their component #21284

Closed

11 tasks

ro0NL mentioned this pull request Feb 16, 2017

[Form][FrameworkBundle] Move FormPass to the Form component #21283

Merged

chalasr mentioned this pull request Feb 28, 2017

[FrameworkBundle][PropertyInfo] Move PropertyInfoPass to the PropertyInfo component #21806

Merged

[Kernel] Move the cache compiler passes into the Kernel component

5ee08d2

Taluu force-pushed the move-cache-passes branch from b29af77 to 7cdfcb4 Compare March 6, 2017 13:31

Taluu added 2 commits March 6, 2017 14:35

[FrameworkBundle] Add BC for cache passes

6330bfa

[HttpKernel] Add arguments for cache passes

06aed12

Taluu force-pushed the move-cache-passes branch from 7cdfcb4 to 06aed12 Compare March 6, 2017 13:36

fabpot closed this Mar 22, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Taluu deleted the move-cache-passes branch August 8, 2019 09:05

[Kernel] Move the cache compiler passes into the Kernel component #20250

[Kernel] Move the cache compiler passes into the Kernel component #20250

Conversation

Taluu commented Oct 19, 2016 • edited Loading

ro0NL commented Oct 19, 2016 • edited Loading

stof commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Oct 21, 2016

Taluu commented Oct 24, 2016

xabbuh commented Oct 25, 2016

Taluu commented Oct 26, 2016

fabpot commented Mar 5, 2017

Taluu commented Mar 5, 2017 • edited Loading

Taluu commented Mar 6, 2017

fabpot commented Mar 22, 2017

Taluu commented Oct 19, 2016 •

edited

Loading

ro0NL commented Oct 19, 2016 •

edited

Loading

ro0NL Oct 19, 2016 •

edited

Loading

ro0NL Oct 19, 2016 •

edited

Loading

ro0NL Oct 20, 2016 •

edited

Loading

Taluu commented Mar 5, 2017 •

edited

Loading