NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromDeamon:move-config-cache-pass

Feb 28, 2017

Merged

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

fabpot merged 1 commit intosymfony:masterfromDeamon:move-config-cache-pass

Feb 28, 2017

Conversation

Copy link

Contributor

Deamon commentedJan 22, 2017

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

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

Copy link

Member

chalasr commentedJan 23, 2017•
edited
Loading

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 force-pushed themove-config-cache-pass branch from64a0100 to2056aaaCompare

January 23, 2017 11:51

Copy link

ContributorAuthor

Deamon commentedJan 23, 2017•
edited
Loading

@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 ?

Copy link

Member

chalasr commentedJan 23, 2017

@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.

Deamon force-pushed themove-config-cache-pass branch 2 times, most recently frome4f886d toe9941f1Compare

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

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php Outdated

		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\UnusedTagsPass;
		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass;
		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ValidateWorkflowsPass;
		useSymfony\Component\Config\DependencyInjection\Compiler\ConfigCachePass;

Copy link

Member

chalasrJan 23, 2017

Choose a reason for hiding this comment

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

Should beSymfony\Component\Config\DependencyInjection\ConfigCachePass, right?

Copy link

ContributorAuthor

DeamonJan 23, 2017•
edited
Loading

Choose a reason for hiding this comment

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

indeed,
thanks for the review!

Deamon force-pushed themove-config-cache-pass branch frome9941f1 to0c69d9cCompare

January 23, 2017 15:40

nicolas-grekas added this to the3.3 milestone

Jan 24, 2017

chalasr mentioned this pull request

Jan 24, 2017

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

Merged

Copy link

Member

chalasr commentedFeb 16, 2017

@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?

Copy link

ContributorAuthor

Deamon commentedFeb 17, 2017

@chalasr I'll do that ASAP

Deamon force-pushed themove-config-cache-pass branch 3 times, most recently from728ad3e to4ce7db9Compare

February 26, 2017 19:10

Copy link

ContributorAuthor

Deamon commentedFeb 26, 2017

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

chalasr approved these changes

Feb 27, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

xabbuh reviewed

Feb 27, 2017

View reviewed changes

src/Symfony/Component/Config/composer.json Outdated

		"php":">=5.5.9",
		"symfony/filesystem":"~2.8\|~3.0"
		"symfony/filesystem":"~2.8\|~3.0",
		"symfony/dependency-injection":"~3.2"

Copy link

Member

xabbuhFeb 27, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

indeed, only a dev requirement.

Copy link

ContributorAuthor

DeamonFeb 27, 2017

Choose a reason for hiding this comment

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

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

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.

Copy link

Member

xabbuhFeb 27, 2017

Choose a reason for hiding this comment

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

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.

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

xabbuhFeb 27, 2017

Choose a reason for hiding this comment

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

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.

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

DeamonFeb 27, 2017

Choose a reason for hiding this comment

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

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

chalasr mentioned this pull request

Feb 27, 2017

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

Merged

Deamon force-pushed themove-config-cache-pass branch from4ce7db9 tob0c76b9Compare

February 27, 2017 19:42

Copy link

Member

xabbuh commentedFeb 27, 2017

👍

Status: Reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Feb 27, 2017

chalasr requested changes

Feb 27, 2017

View reviewed changes

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


		publicfunctionprocess(ContainerBuilder$container)
		{
		$resourceCheckers =$this->findAndSortTaggedServices('config_cache.resource_checker',$container);

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

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

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

		return;
		}

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

Copy link

Member

chalasrFeb 27, 2017•
edited
Loading

Choose a reason for hiding this comment

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

'config_cache_factory' should be$this->factoryServiceId

chalasr reviewed

Feb 27, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php Outdated

		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\UnusedTagsPass;
		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ConfigCachePass;
		useSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ValidateWorkflowsPass;
		useSymfony\Component\Config\DependencyInjection\Compiler\ConfigCachePass;

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

should beSymfony\Component\Config\DependencyInjection\ConfigCachePass

chalasr reviewed

Feb 27, 2017

View reviewed changes

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

		* file that was distributed with this source code.
		*/

		namespaceSymfony\Component\Config\Tests\DependencyInjection\Compiler;

Copy link

Member

chalasrFeb 27, 2017

Choose a reason for hiding this comment

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

should beSymfony\Component\Config\Tests\DependencyInjection;

fabpot added a commit that referenced this pull request

Feb 27, 2017

minor#21779[Form][Serializer] Add missing conflicts for DI (chalasr)

29b5a6e

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       | MITThey make use of `PriorityTaggedServiceTrait` which is available since 3.2 onlyCommits-------ddae4ef [Form][Serializer] Add missing conflicts for DI

Deamon force-pushed themove-config-cache-pass branch 2 times, most recently from9dc062b to1c3afb7Compare

February 27, 2017 21:09

Copy link

ContributorAuthor

Deamon commentedFeb 27, 2017

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

Move ConfigCachePass from FrameworkBundle to Config

bce445f

Deamon force-pushed themove-config-cache-pass branch from1c3afb7 tobce445fCompare

February 28, 2017 07:43

chalasr approved these changes

Feb 28, 2017

View reviewed changes

Copy link

Member

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 is unrelated

Copy link

Member

fabpot commentedFeb 28, 2017

Thank you@Deamon.

fabpot merged commitbce445f intosymfony:master

Feb 28, 2017

fabpot added a commit that referenced this pull request

Feb 28, 2017

feature#21375[FrameworkBundle][Config] Move ConfigCachePass from Fr…

41fd5d1

…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<!--see comment below-->| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes/no| Fixed tickets | - <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | <!--highly recommended for new features-->This MR is part of the#21284 todo list<!--- Bug fixes must be submitted against the lowest branch where they apply  (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->Commits-------bce445f Move ConfigCachePass from FrameworkBundle to Config

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

Labels

Config Deprecation Feature FrameworkBundle Status: Reviewed

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

Deamon commentedJan 22, 2017

Uh oh!

chalasr commentedJan 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Deamon commentedJan 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedJan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeamonJan 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commentedFeb 16, 2017

Uh oh!

Deamon commentedFeb 17, 2017

Uh oh!

Deamon commentedFeb 26, 2017

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commentedFeb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasrFeb 27, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deamon commentedFeb 27, 2017

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commentedFeb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chalasr commentedJan 23, 2017•
edited
Loading

Deamon commentedJan 23, 2017•
edited
Loading

DeamonJan 23, 2017•
edited
Loading

chalasrFeb 27, 2017•
edited
Loading