[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config #21375

Deamon · 2017-01-22T23:41:31Z

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

This MR is part of the #21284 todo list

chalasr · 2017-01-23T09:22:22Z

Thanks for working on this @Deamon. Tests were broken before this PR, they have been fixed meantime, could you rebase against master so we could see if builds are green?

Also I suggest to wait that at least one of the opened PRs on this subject is merged before moving the remaining passes of the todo list, the goal has been validated by maintainers but the approach might still be challenged, it would be too bad to rework several PRs in this case.

Deamon · 2017-01-23T13:43:19Z

@chalasr, it's a pleasure :)
And as you suggested, I'll wait to continue other changes. I understand that the approach still in discussion.

I pushed again but it fails, I might need to update a constraint in a composer.json file somewhere maybe this one : src/Symfony/Component/DependencyInjection/composer.json line 33 and maybe a conflict in the same file ?

chalasr · 2017-01-23T14:01:51Z

@Deamon It fails because of:

Fatal error: Trait 'Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait' not found in /home/travis/build/symfony/symfony/src/Symfony/Component/Config/DependencyInjection/ConfigCachePass.php

This trait is part of the DI component since 3.2, so what is needed is to add "symfony/dependency-injection": "~3.2" as dev requirement in the Config composer.json, sort as the trait exists even with low deps.

maybe a conflict in the same file

Since it must be ok to use the DI component along with the Config one without using the compiler pass introduced here, this one should not involve any conflict AFAIK.

chalasr · 2017-01-23T15:31:46Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ValidateWorkflowsPass;
+use Symfony\Component\Config\DependencyInjection\Compiler\ConfigCachePass;


Should be Symfony\Component\Config\DependencyInjection\ConfigCachePass, right?

indeed,
thanks for the review!

chalasr · 2017-02-16T18:34:14Z

@daemon The existing PRs regarding the same ticket have been merged, we are now sure about the right approach: add a constructor to the moved compiler pass (the new one) taking the service id in which collected services are injected as argument defaulting to the service id used in the framework (here $factoryServiceId = 'config_cache_factory') and the collected tag(s) as other arguments defaulting to their name in the framework (here $resourceCheckerTag = 'config_cache.resource_checker'), see merged PRs for inspiration.

Would you mind rebasing this and make the change?

Deamon · 2017-02-17T14:24:18Z

@chalasr I'll do that ASAP

Deamon · 2017-02-26T21:27:11Z

@chalasr, PR rebased and updated with a constructor inspired from other PR and your suggestions.

chalasr

LGTM 👍

xabbuh · 2017-02-27T10:30:28Z

src/Symfony/Component/Config/composer.json

@@ -17,7 +17,8 @@
    ],
    "require": {
        "php": ">=5.5.9",
-        "symfony/filesystem": "~2.8|~3.0"
+        "symfony/filesystem": "~2.8|~3.0",
+        "symfony/dependency-injection": "~3.2"


IMO that's not a hard dependency as the Config component itself does not require the DependencyInjection component to be installed.

indeed, only a dev requirement.

I put this in require because we use the "PriorityTaggedServiceTrait" from DependencyInjection here.

I add it to make tests passes on PHP7.1 (travis job).

If it needs to be in require-dev even with this, I change it.

We can skip tests when the trait is not available. And we should probably add a conflict rule for versions of the DependencyInjection component before 3.2.

@xabbuh are you sure for the conflict? People can use DI but not the pass

That's true, but it will then lead to errors when you decide to use the pass and didn't update the DI version before.

Ok I wasn't sure that it's more important than broad compatibility. Fixed it for existing passes in #21779.

We can skip tests when the trait is not available

Adding the conflict avoids the need for skipping tests.

ok I'll push in a minute the require in dev and the conflict section

xabbuh · 2017-02-27T19:57:28Z

👍

Status: Reviewed

chalasr · 2017-02-27T19:59:19Z

src/Symfony/Component/Config/DependencyInjection/ConfigCachePass.php

+
+    public function process(ContainerBuilder $container)
+    {
+        $resourceCheckers = $this->findAndSortTaggedServices('config_cache.resource_checker', $container);


'config_cache.resource_checker' should be $this->resourceCheckerTag

chalasr · 2017-02-27T20:00:01Z

src/Symfony/Component/Config/DependencyInjection/ConfigCachePass.php

+            return;
+        }
+
+        $container->getDefinition('config_cache_factory')->replaceArgument(0, $resourceCheckers);


'config_cache_factory' should be $this->factoryServiceId

chalasr · 2017-02-27T20:03:35Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

 use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ValidateWorkflowsPass;
+use Symfony\Component\Config\DependencyInjection\Compiler\ConfigCachePass;


should be Symfony\Component\Config\DependencyInjection\ConfigCachePass

chalasr · 2017-02-27T20:04:27Z

src/Symfony/Component/Config/Tests/DependencyInjection/ConfigCachePassTest.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Tests\DependencyInjection\Compiler;


should be Symfony\Component\Config\Tests\DependencyInjection;

This PR was merged into the 3.3-dev branch. Discussion ---------- [Form][Serializer] Add missing conflicts for DI | Q | A | ------------- | --- | Branch? | master | Tests pass? | yes | Fixed tickets | #21375 (comment) | License | MIT They make use of `PriorityTaggedServiceTrait` which is available since 3.2 only Commits ------- ddae4ef [Form][Serializer] Add missing conflicts for DI

Deamon · 2017-02-27T23:12:59Z

@chalasr everything has been corrected.
Travis do not pass for HHVM apparently due to timeout, is there a way to restart the failing test ?

chalasr

👍 Build failure is unrelated

fabpot · 2017-02-28T16:10:45Z

Thank you @Deamon.

…ameworkBundle to Config (Deamon) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes/no | Fixed tickets | -  | License | MIT | Doc PR |  This MR is part of the #21284 todo list  Commits ------- bce445f Move ConfigCachePass from FrameworkBundle to Config

carsonbot added Status: Needs Review FrameworkBundle Config Feature Deprecation labels Jan 22, 2017

chalasr mentioned this pull request Jan 23, 2017

[FrameworkBundle] Move compiler passes to their component #21284

Closed

11 tasks

Deamon force-pushed the move-config-cache-pass branch from 64a0100 to 2056aaa Compare January 23, 2017 11:51

Deamon force-pushed the move-config-cache-pass branch 2 times, most recently from e4f886d to e9941f1 Compare January 23, 2017 14:41

Deamon mentioned this pull request Jan 23, 2017

[FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProfiler bundle #21368

Closed

chalasr reviewed Jan 23, 2017

View reviewed changes

Deamon force-pushed the move-config-cache-pass branch from e9941f1 to 0c69d9c Compare January 23, 2017 15:40

nicolas-grekas added this to the 3.3 milestone Jan 24, 2017

chalasr mentioned this pull request Jan 24, 2017

[FrameworkBundle][Serializer] Move SerializerPass to the Serializer #21293

Merged

Deamon force-pushed the move-config-cache-pass branch 3 times, most recently from 728ad3e to 4ce7db9 Compare February 26, 2017 19:10

chalasr approved these changes Feb 27, 2017

View reviewed changes

xabbuh reviewed Feb 27, 2017

View reviewed changes

chalasr mentioned this pull request Feb 27, 2017

[Form][Serializer] Add missing conflicts for DI #21779

Merged

Deamon force-pushed the move-config-cache-pass branch from 4ce7db9 to b0c76b9 Compare February 27, 2017 19:42

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 27, 2017

chalasr requested changes Feb 27, 2017

View reviewed changes

chalasr reviewed Feb 27, 2017

View reviewed changes

Deamon force-pushed the move-config-cache-pass branch 2 times, most recently from 9dc062b to 1c3afb7 Compare February 27, 2017 21:09

Move ConfigCachePass from FrameworkBundle to Config

bce445f

Deamon force-pushed the move-config-cache-pass branch from 1c3afb7 to bce445f Compare February 28, 2017 07:43

chalasr approved these changes Feb 28, 2017

View reviewed changes

fabpot merged commit bce445f into symfony:master Feb 28, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config #21375

[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config #21375

Deamon commented Jan 22, 2017

chalasr commented Jan 23, 2017 •

edited

Loading

Deamon commented Jan 23, 2017 •

edited

Loading

chalasr commented Jan 23, 2017

chalasr Jan 23, 2017

Deamon Jan 23, 2017 •

edited

Loading

chalasr commented Feb 16, 2017

Deamon commented Feb 17, 2017

Deamon commented Feb 26, 2017

chalasr left a comment

xabbuh Feb 27, 2017

chalasr Feb 27, 2017

Deamon Feb 27, 2017

xabbuh Feb 27, 2017

chalasr Feb 27, 2017

xabbuh Feb 27, 2017

chalasr Feb 27, 2017

Deamon Feb 27, 2017

xabbuh commented Feb 27, 2017

chalasr Feb 27, 2017

chalasr Feb 27, 2017 •

edited

Loading

chalasr Feb 27, 2017

chalasr Feb 27, 2017

Deamon commented Feb 27, 2017

chalasr left a comment

fabpot commented Feb 28, 2017

		use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ValidateWorkflowsPass;
		use Symfony\Component\Config\DependencyInjection\Compiler\ConfigCachePass;

[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config #21375

[FrameworkBundle][Config] Move ConfigCachePass from FrameworkBundle to Config #21375

Conversation

Deamon commented Jan 22, 2017

chalasr commented Jan 23, 2017 • edited Loading

Deamon commented Jan 23, 2017 • edited Loading

chalasr commented Jan 23, 2017

Choose a reason for hiding this comment

Deamon Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr commented Feb 16, 2017

Deamon commented Feb 17, 2017

Deamon commented Feb 26, 2017

chalasr left a 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

xabbuh commented Feb 27, 2017

Choose a reason for hiding this comment

chalasr Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deamon commented Feb 27, 2017

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented Feb 28, 2017

chalasr commented Jan 23, 2017 •

edited

Loading

Deamon commented Jan 23, 2017 •

edited

Loading

Deamon Jan 23, 2017 •

edited

Loading

chalasr Feb 27, 2017 •

edited

Loading