[Translation] Fix test, null is valid according to the tests #32386
Conversation
3038112 to
de7747b
Compare
de7747b to
ed56c53
Compare
| * | ||
| * @throws InvalidArgumentException If the locale contains invalid characters | ||
| */ | ||
| public function addResource(string $format, $resource, string $locale, string $domain = null) | ||
| public function addResource(string $format, $resource, ?string $locale, string $domain = null) |
There was a problem hiding this comment.
I don't see how this could work. The assertValidLocale() call below will throw an exception when null is pass for the $locale argument, won't it?
There was a problem hiding this comment.
If we look at the test null is a valid locale, maybe the tests are wrong ?
There was a problem hiding this comment.
assertValidLocale does not seems to throw on null
There was a problem hiding this comment.
hm indeed: https://3v4l.org/h7sGr
There was a problem hiding this comment.
But I think we should deprecate the possibility to pass null here. Looking at how it is used it doesn't seem like it was intended to be supported.
There was a problem hiding this comment.
addResource and similar methods changed here require a $locale argument which does not have a default null value and does not implement the fallback to the defaut locale like for example
symfony/src/Symfony/Component/Translation/Translator.php
Lines 223 to 226 in ed56c53
for this reason, I agree that the $locale should not be nullable and calling
$this->resources[$locale] with null is also strange. so I'm in favor of deprecating null in those places and not making it nullable.
There was a problem hiding this comment.
Also the locale is nullable in
but the getter is not nullable.It seems usages of getLocale would also not expect null. So the nullable in the MessageCatalogue constructor should also be deprecated
There was a problem hiding this comment.
I'm going to do it
|
Closing in favor of #32415 |
This PR was merged into the 4.4 branch. Discussion ---------- [Translation] deprecate passing a null locale | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | not needed <!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch. --> According to the discussion in #32386 (comment) it seems that allowing null here was not the right thing to do, so we are deprecating this behaviour. Commits ------- 088615d [Translation] deprecate passing a null locale
The tests passes now.