Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][Translation] Move translation compiler pass#22619
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
conflict rules are missing to ensure it does not get installed with an older version of symfony/translation (which would not allow registering it)
UPGRADE-3.3.md Outdated
| `Symfony\Component\Workflow\DependencyInjection\ValidateWorkflowsPass` class instead. | ||
| * The`Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationDumperPass` | ||
| class has been deprecated and will be removed in 4.0. Use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this will have to go in a new UPGRADE-3.4.md file actually. It is too late to include this in 3.3 as we already released the beta
| useSymfony\Component\DependencyInjection\Reference; | ||
| useSymfony\Component\DependencyInjection\ContainerBuilder; | ||
| useSymfony\Component\DependencyInjection\Compiler\CompilerPassInterface; | ||
| @trigger_error(sprintf('The %s class is deprecated since version 3.3 and will be removed in 4.0. UseSymfony\Component\Translation\DependencyInjection\TranslationDumperPass instead.', TranslationDumperPass::class),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
3.4
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The Translator component needssymfony/dependency-injection:~3.3 as dev requirement and a conflict forsymfony/dependency-injection:<3.3. Also adding a test case for passes which miss one would be great.
| { | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| if (!$container->hasDefinition('translation.extractor')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the "collector" service id should be configurable through a constructor argument, same for the collected tag e.g.__construct($extractorServiceId = 'translation.extractor', $extractorTag = 'translation.extractor')
| { | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| if (!$container->hasDefinition('translator.default')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same here (constructor)
| */ | ||
| namespaceSymfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler; | ||
| namespaceSymfony\Component\Translation\Tests\DependencyInjection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The original test case should be kept inFrameworkBundle with@group legacy and copied to the component, not moved.
| { | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| if (!$container->hasDefinition('translation.writer')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same here (constructor)
chalasr commentedMay 3, 2017
The fixed ticket would not be fixed actually, HttpKernel passes are still missing. |
lepiaf commentedMay 3, 2017
@chalasr I'm currently working in another pr to move HttpKernel passes |
| * Deprecated`TranslationExtractorPass`, use | ||
| `Symfony\Component\Translation\DependencyInjection\TranslationExtractorPass` instead | ||
| * Deprecated`TranslatorPass`, use | ||
| `Symfony\Component\Translation\DependencyInjection\TranslatorPass` instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
2 extra spaces beforeSymfony
| * Deprecated`TranslationDumperPass`, use | ||
| `Symfony\Component\Translation\DependencyInjection\TranslationDumperPass` instead | ||
| * Deprecated`TranslationExtractorPass`, use | ||
| `Symfony\Component\Translation\DependencyInjection\TranslationExtractorPass` instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
1 extra space beforeSymfony
chalasr commentedMay 3, 2017
@lepiaf thanks for working on this. |
lepiaf commentedMay 5, 2017
I did all requested changes. I will add new test for moved class. |
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
~3.4 can't be used as a constraint yet, that breaks tests. I would let it as is and wait for 3.3 to be stable/ 3.4 branch to be created
| }, | ||
| "require-dev": { | ||
| "symfony/config":"~2.8|~3.0", | ||
| "symfony/dependency-injection":"~3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you need to add a conflict rule in order to prevent errors when using 3.3 features (these passes use ones)
| $translatorServiceId ='translator.default', | ||
| $loaderServiceId ='translation.loader', | ||
| $loaderTag ='translation.loader' | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
one-line signatures are preferred
| * @param string $translatorServiceId | ||
| * @param string $loaderServiceId | ||
| * @param string $loaderTag | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
phpdoc should be removed (same for properties, applies on the 3 passes), it doesn't give anything that an IDE couldn't guess
xabbuh commentedMay 27, 2017
@lepiaf |
lepiaf commentedMay 27, 2017
tests are failing but I don't know why. Is |
chalasr left a comment
There was a problem hiding this 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 is normal AFAIK. 👍
chalasr commentedJun 21, 2017
ping deciders |
fabpot commentedJul 6, 2017
Thank you@lepiaf. |
…er pass (lepiaf)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle][Translation] Move translation compiler 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/amove TranslationDumperPass, TranslationExtractorPass, TranslatorPass to Translation component.Commits-------74c951f [FrameworkBundle][Translation] Move translation compiler pass
Uh oh!
There was an error while loading.Please reload this page.
move TranslationDumperPass, TranslationExtractorPass, TranslatorPass to Translation component.