[Translator] Default value for 'sort' option in translation:update should be 'asc' #35486
Conversation
|
|
||
| return 1; | ||
| if ($input->hasOption('sort')) { | ||
| if ($sort = $input->getOption('sort')) { |
There was a problem hiding this comment.
getOption() returns null if no value has been defined.
Line 296: if the sort option has been defined, even if it has no value.
Lines 297-304: if a value has been defined, check that it's valid
Line 307: if the option has been explicitly set to 'desc', makes an rsort()
Line 398: we do a sort() in all other cases (including if there's no value defined)
There was a problem hiding this comment.
Why not defining the default value on the option definition instead of dealing with null and only allow asc and desc as strings and no empty value?
There was a problem hiding this comment.
I'm ok with this technicaly, but it would change the default behavior, where --sort=asc would be applied even if the --sort option is not specified. I didn't get into this because the goal is just to fix a bug, and I don't know if the default order responds to any particular logic.
There are actually three possible states: no sorting, asc and desc.
There was a problem hiding this comment.
I would also set ASC as the default value of the option instead of allowing null which does not mean anything.
|
|
||
| return 1; | ||
| if ($input->hasOption('sort')) { | ||
| if ($sort = $input->getOption('sort')) { |
There was a problem hiding this comment.
I would also set ASC as the default value of the option instead of allowing null which does not mean anything.
|
I made the modification: the command now returns the list with "sort=asc" by default. |
0b6a58c to
fdb13c8
Compare
|
Thank you @versgui. |
…n:update should be 'asc' (versgui) This PR was squashed before being merged into the 4.4 branch (closes #35486). Discussion ---------- [Translator] Default value for 'sort' option in translation:update should be 'asc' | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | The value for 'sort' option for `bin/console translation:update --sort` is optional, but no default value is defined. So the list isn't sorted if no value is explicitly defined. This MR brings a default value "asc" if no value is defined, so the list is correctly sorted. Commits ------- fdb13c8 [Translator] Default value for 'sort' option in translation:update should be 'asc'
The value for 'sort' option for
bin/console translation:update --sortis optional, but no default value is defined. So the list isn't sorted if no value is explicitly defined.This MR brings a default value "asc" if no value is defined, so the list is correctly sorted.