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] Convert null prefix to an empty string in translation:update#20184
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
| $output->text('Parsing templates'); | ||
| $extractor =$this->getContainer()->get('translation.extractor'); | ||
| $extractor->setPrefix($input->getOption('prefix')); | ||
| $extractor->setPrefix($input->getOption('prefix') ?:''); |
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 should be explicit here:null === $input->getOption('prefix') ? '' : $input->getOption('prefix'). If not, a prefix with a falsy value like0 is converted to an empty string, which is not what we want.
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.
Fixed
141e679 tof02b687Comparefabpot commentedOct 9, 2016
Thank you@chalasr. |
…n translation:update (chalasr)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] Convert null prefix to an empty string in translation:update| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20044| License | MIT| Doc PR | n/aThis command needs the ability to use an empty string as prefix, which is not possible using `bin/console translation:update --prefix=""` because `$argv` doesn't parse empty strings thus the value is converted to `null` by `ArgvInput` (only since#19946, before the option was not considered to be set, giving the default `'__'` thus this should be fine from a BC pov).Here I propose to explicitly convert the `prefix` value to an empty string if set to `null`, as it is a very specific need and we can't guess that from `ArgvInput`.An other way to fix it could be to add a `--no-prefix` option to the command but I don't think it is worth it, and it couldn't be treated as a bug fix thus not fixed before `3.2`.Commits-------f02b687 [FrameworkBundle] Convert null prefix to an empty string in translation:update command
| // load any messages from templates | ||
| $extractedCatalogue =newMessageCatalogue($input->getArgument('locale')); | ||
| $output->text('Parsing templates'); | ||
| $prefix =$input->getOption('prefix'); |
navitronicOct 27, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Maybe I am misunderstanding this, but$prefix can never be assigned with anull value here. If it were null, shouldn't it return the default value of__ specified on L40?
Which would mean that the following ternary never returns an empty string.
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.
Unfortunately, you're right.
In fact the issue is actually quite complex, the current implementation makes impossible to detectt if an option has been set with an empty string, it'salways parsed as null, so it's impossible get an empty value for an option withVALUE_OPTIONALand a default value, because if it isnull, the default value is used (the case without default value has been fixed in#19946). We would need to differentiate if an option has been set without any value or with an empty value ("") and I don't think we should make this difference as it would be really hard to do it backward compatible, some use cases would be broken for having this one working (empty strings are converted to null, it is the expected behavior and it is tested).
… to an empty string in translation:update (chalasr)" (chalasr)This PR was merged into the 2.7 branch.Discussion----------Revert "bug#20184 [FrameworkBundle] Convert null prefix to an empty string in translation:update (chalasr)"| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20184 (comment)| License | MIT| Doc PR | n/aThis reverts commit3f650f8 because the fix is not valid.It is actually not possible to have an option with value optional given empty to be `empty` if this one has a default value. The default value will always be the one returned, see#20184 (comment) for details.Commits-------b2fa7c4 Revert "bug#20184 [FrameworkBundle] Convert null prefix to an empty string in translation:update (chalasr)"
* 2.7: prefer getSourceContext() over getSource() [HttpFoundation] Avoid implicit null to array conversion in request matcher Revert "bug#20184 [FrameworkBundle] Convert null prefix to an empty string in translation:update (chalasr)" Update UPGRADE-2.7.md
* 2.8: prefer getSourceContext() over getSource() [HttpFoundation] Avoid implicit null to array conversion in request matcher Revert "bug#20184 [FrameworkBundle] Convert null prefix to an empty string in translation:update (chalasr)" Update UPGRADE-2.7.md
* 3.1: prefer getSourceContext() over getSource() [HttpFoundation] Avoid implicit null to array conversion in request matcher Revert "bug#20184 [FrameworkBundle] Convert null prefix to an empty string in translation:update (chalasr)" Update UPGRADE-2.7.md
This command needs the ability to use an empty string as prefix, which is not possible using
bin/console translation:update --prefix=""because$argvdoesn't parse empty strings thus the value is converted tonullbyArgvInput(only since#19946, before the option was not considered to be set, giving the default'__'thus this should be fine from a BC pov).Here I propose to explicitly convert the
prefixvalue to an empty string if set tonull, as it is a very specific need and we can't guess that fromArgvInput.An other way to fix it could be to add a
--no-prefixoption to the command but I don't think it is worth it, and it couldn't be treated as a bug fix thus not fixed before3.2.