Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] Moving Cache-related CompilerPass to Cache component#27770

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

Conversation

@Korbeil
Copy link
Contributor

@KorbeilKorbeil commentedJun 29, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?yes
Deprecations?yes
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

Hi, first PR here 🎉

This is related to#27479 and a first work to move Cache-related CompilerPass out ofFrameworkBundle, it allows to decouple part of the FrameworkBundle configuration classes.

Since we didn't choosed a fixed directory organization to manage theses, I actually did same as in Bundles and used DependencyInjection folder. If we do choose to follow mylast comment directory organization proposal, I'll move theses CompilerPass toFramework/DependencyInjection/Compiler directory (nothing hard here).

Thanks to@DanieleGBX that helped me checking this PR and gave me some good advices !

Here is a list of all CompilerPass I moved, with related component (I also moved related tests when they were present):

  • Cache - CacheCollectorPass
  • Cache - CachePoolClearerPass
  • Cache - CachePoolPass
  • Cache - CachePoolPrunerPass

floranpagliai, webda2l, Taluu, greg0ire, and EtienneDh reacted with thumbs up emojifloranpagliai and EtienneDh reacted with hooray emoji
@KorbeilKorbeil requested a review fromlyrixx as acode ownerJune 29, 2018 11:06
@KorbeilKorbeil changed the titleMoving all component-related CompilerPass to their component[FrameworkBundle] Moving all component-related CompilerPass to their componentJun 29, 2018
@stof
Copy link
Member

This requires addingconflict rules with all older versions of the components (or updating the min version in therequire key in case of mandatory requirements) in the FrameworkBundle composer.json. Otherwise, we might end up installing an older version of the component, and have it half-configured due to not running the compiler pass.

Korbeil reacted with thumbs up emoji

@chalasrchalasr added this to thenext milestoneJun 29, 2018
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 2 times, most recently from0d7635d to75a38fcCompareJune 29, 2018 14:38

$translatorClass =$container->getParameterBag()->resolveValue($container->findDefinition('translator')->getClass());

if (!is_subclass_of($translatorClass,'Symfony\Component\Translation\TranslatorBagInterface')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could take the opportunity to useTranslatorBagInterface::class

chalasr and Korbeil reacted with thumbs up emoji
/**
* @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
*/
class TranslationLoggingPassimplements CompilerPassInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping it namedLoggingTranslatorPass would be more consistent withSymfony\Component\Translation\LoggingTranslator

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I tried to be consistant with other CompilerPass that were already inTranslation/DependencyInjection, I can move back to the old name if preferred .

@lyrixx
Copy link
Member

@Korbeil you also need to remove theses line:https://github.com/symfony/symfony/blob/master/.github/CODEOWNERS#L25-L26

Korbeil reacted with thumbs up emoji

@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 6 times, most recently from3e3cbff to3cf69f8CompareJuly 9, 2018 08:02
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 2 times, most recently from3b4e4d7 to7c57468CompareJuly 11, 2018 12:13
@Korbeil
Copy link
ContributorAuthor

After quick talk with@nicolas-grekas I choosed to split this PR for each component

Now this one contains only moved CompilerPass for Cache component.

@KorbeilKorbeil changed the title[FrameworkBundle] Moving all component-related CompilerPass to their component[FrameworkBundle] Moving Cache-related CompilerPass to their componentJul 11, 2018
@KorbeilKorbeil changed the title[FrameworkBundle] Moving Cache-related CompilerPass to their component[FrameworkBundle] Moving Cache-related CompilerPass to Cache componentJul 11, 2018
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 3 times, most recently frome619abd to1c8c114CompareJuly 11, 2018 12:34
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 2 times, most recently from3dab36d to28703e5CompareAugust 17, 2018 08:44
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch from1bfe564 to977f17dCompareAugust 31, 2018 15:39
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 2 times, most recently from530ed5d toce556ccCompareSeptember 20, 2018 12:02
@Korbeil
Copy link
ContributorAuthor

Hi@nicolas-grekas, anymore work to do here?

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

namespaceSymfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;
Copy link
Member

Choose a reason for hiding this comment

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

The old test must be kept and marked as@legacy until the deprecated class is removed (same for all passes moved here)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Just reverted old tests, for@legacy you want it on all tests ?
Is it also needed on passes since we have the@deprecated tag ?

Copy link
Member

@nicolas-grekasnicolas-grekasSep 25, 2018•
edited
Loading

Choose a reason for hiding this comment

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

it's@group legacy on the test classes

Korbeil and chalasr reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Should be okay, tell me if I forgot something.

@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch 4 times, most recently fromd14b867 to95f87a5CompareSeptember 25, 2018 12:09
/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedExceptionMessage Class "Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\NotFound" used for service "pool.not-found" cannot be found.
* @expectedExceptionMessage Class "Symfony\Component\Cache\Tests\DependencyInjection\NotFound" used for service "pool.not-found" cannot be found.
Copy link
Member

Choose a reason for hiding this comment

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

should be reverted

Copy link
ContributorAuthor

@KorbeilKorbeilSep 25, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Was checking tests and just saw that, sorry :/

@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch from95f87a5 to2a81bdbCompareSeptember 25, 2018 12:17
@KorbeilKorbeilforce-pushed thefeature/moving-compiler-pass-out-of-framework-bundle branch from2a81bdb to53e7040CompareSeptember 26, 2018 15:26
@fabpot
Copy link
Member

Thank you@Korbeil.

@fabpotfabpot merged commit53e7040 intosymfony:masterOct 10, 2018
fabpot added a commit that referenced this pull requestOct 10, 2018
… Cache component (Korbeil)This PR was merged into the 4.2-dev branch.Discussion----------[FrameworkBundle] Moving Cache-related CompilerPass to Cache component| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | yes| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Hi, first PR here 🎉This is related to#27479 and a first work to move Cache-related CompilerPass out of `FrameworkBundle`, it allows to decouple part of the FrameworkBundle configuration classes.Since we didn't choosed a fixed directory organization to manage theses, I actually did same as in Bundles and used DependencyInjection folder. If we do choose to follow my [last comment directory organization proposal](#27479 (comment)), I'll move theses CompilerPass to `Framework/DependencyInjection/Compiler` directory (nothing hard here).Thanks to@DanieleGBX that helped me checking this PR and gave me some good advices !Here is a list of all CompilerPass I moved, with related component (I also moved related tests when they were present):- **Cache** - CacheCollectorPass- **Cache** - CachePoolClearerPass- **Cache** - CachePoolPass- **Cache** - CachePoolPrunerPassCommits-------53e7040 moving Cache-related compiler pass from FrameworkBundle to Cache component
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
@KorbeilKorbeil deleted the feature/moving-compiler-pass-out-of-framework-bundle branchFebruary 4, 2019 20:25
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

@lyrixxlyrixxAwaiting requested review from lyrixx

+1 more reviewer

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

9 participants

@Korbeil@stof@lyrixx@fabpot@nicolas-grekas@ogizanagi@chalasr@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp