[FrameworkBundle] Rename translation:update to translation:extract #43758
Conversation
fb039e0 to
e7305b3
Compare
|
While at it, let's move the command to the component instead of fwb, wdyt? |
|
When renaming a command, we need to have the old name as an alias of the new name in the command. Otherwise, it is a BC break for people calling the command in the CLI. @nicolas-grekas the command currently relies on getting the Kernel from the Application, which relies on the FrameworkBundle Application, so it does not make sense to move it. |
|
But the kernel can be injected to the constructor instead of relying on getApplication, isn't it? |
|
@stof by alias do you mean something like that in console.php ? @nicolas-grekas I'm not comfortable to move the command in the component right now (at least in this PR). Moreover it relies to the Kernel as @stof said, so I'm not sure we can move it. |
|
Moving means deprecating, we'd better not deprecate twice the same command, that's why I'm wondering. |
|
@welcoMattic no, I'm not talking about aliasing the service. I'm talking about defining aliases for the command name in the Command itself |
|
Note that there is really 2 separate renamings here:
If we are not sure the new location of the class, we might delay that class renaming until Symfony 6.1, doing only the command renaming before the feature freeze. |
|
@stof Ok, I got it. But renaming the CLI command without renaming the file is kind of half done job. I'll try to move it to the Component, I'm just not sure about how to inject the Kernel (like any other service?). |
Moving does not necessarily imply deprecating in this case since the command uses the kernel mainly for extracting translations from a given bundle. So the command could be split in one base command located in the component and one bundle-aware decorator in framework-bundle, as we did for yaml and twig lint commands. |
|
@welcoMattic I would still say more than half, as it is the part of the renaming that solves the DX issue (devs using FrameworkBundle don't care about the class names of commands, as FrameworkBundle is registering them) |
e7305b3 to
b767b03
Compare
|
I've just push the renaming of the CLI command only, with the deprecation trigger in case of usage of |
c1219bb to
ad3faf8
Compare
src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php
Outdated
Show resolved
Hide resolved
ffc18ac to
b98f913
Compare
422cbd4 to
562b2ed
Compare
|
When deprecating something, we try to keep at least one test covering the legacy behavior. Can you please do that? |
562b2ed to
223f18e
Compare
|
Thank you @welcoMattic. |
…anslation:extract (welcoMattic) This PR was merged into the 5.4 branch. Discussion ---------- [FrameworkBundle] Rename translation:update to translation:extract Ref symfony#16023 The code has been merged in symfony/symfony#43758 I'm not 100% sure about the position of the deprecated block, let me know if I have to move it elsewhere. Commits ------- 7f44b33 Rename translation:update to translation:extract
… of using trigger_deprecation call (welcoMattic) This PR was squashed before being merged into the 5.3 branch. Discussion ---------- [FrameworkBundle] Trigger deprecations on stderr instead of using trigger_deprecation call | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | | License | MIT | Doc PR | As discussed in #43758 (comment), I change the way to deprecate options by using warning on stderr instead of `trigger_deprecation` call. Commits ------- 663eb29 [FrameworkBundle] Trigger deprecations on stderr instead of using trigger_deprecation call
… of using trigger_deprecation call (welcoMattic) This PR was squashed before being merged into the 5.3 branch. Discussion ---------- [FrameworkBundle] Trigger deprecations on stderr instead of using trigger_deprecation call | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | | License | MIT | Doc PR | As discussed in symfony/symfony#43758 (comment), I change the way to deprecate options by using warning on stderr instead of `trigger_deprecation` call. Commits ------- 663eb29245 [FrameworkBundle] Trigger deprecations on stderr instead of using trigger_deprecation call
…nslationExtract command to match the command name (welcoMattic) This PR was merged into the 7.3 branch. Discussion ---------- [FrameworkBundle] Rename TranslationUpdateCommand to TranslationExtract command to match the command name | Q | A | ------------- | --- | Branch? | 7.3 <!-- see below --> | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT In #43758, `translation:update` command has been renamed to `translation:extract` command. But the file has never been renamed. This PR deprecate the `TranslationUpdateCommand` class in favor to `TranslationExtractCommand`. So, in 8.0 we could remove `TranslationUpdateCommand` file completely. Commits ------- a61bd89 Rename TranslationUpdateCommand to TranslationExtract command to match the command name
As we discussed in #41411, I've renamed
translation:updatetotranslation:extractwhich is a more accurate name.translation:updatewill trigger a deprecation alert until 6.0 where we could remove it completely. For now all the code of the command lives in TranslationExtractCommand.php file, and TranslationUpdateCommand call it.I think it should be merged after #43676.
It's my first attempt to deprecate something in Symfony, let me know if I forgot something or if I did something wrong.