[FrameworkBundle] Convert null prefix to an empty string in translation:update #20184
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabpot
reviewed
Oct 9, 2016
| @@ -119,7 +119,7 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $extractedCatalogue = new MessageCatalogue($input->getArgument('locale')); | |||
| $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.
You should be explicit here: null === $input->getOption('prefix') ? '' : $input->getOption('prefix'). If not, a prefix with a falsy value like 0 is converted to an empty string, which is not what we want.
…on:update command
141e679 to
f02b687
Compare
|
Thank you @chalasr. |
fabpot
added a commit
that referenced
this pull request
Oct 9, 2016
…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/a This 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
This was referenced Oct 27, 2016
Merged
Merged
Merged
navitronic
reviewed
Oct 27, 2016
| @@ -118,8 +118,9 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| // load any messages from templates | |||
| $extractedCatalogue = new MessageCatalogue($input->getArgument('locale')); | |||
| $output->text('Parsing templates'); | |||
| $prefix = $input->getOption('prefix'); | |||
There was a problem hiding this comment.
Maybe I am misunderstanding this, but $prefix can never be assigned with a null 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.
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's always parsed as null, so it's impossible get an empty value for an option with VALUE_OPTIONAL and a default value, because if it is null, 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).
fabpot
added a commit
that referenced
this pull request
Nov 7, 2016
… 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/a This reverts commit 3f650f8 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)"
fabpot
added a commit
that referenced
this pull request
Nov 7, 2016
* 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
fabpot
added a commit
that referenced
this pull request
Nov 7, 2016
* 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
fabpot
added a commit
that referenced
this pull request
Nov 7, 2016
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.