Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Translation] deprecate passing a null locale #32415

Merged

Conversation

@Simperfit
Copy link
Contributor

commented Jul 7, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets none
License MIT
Doc PR not needed

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.

@Simperfit Simperfit force-pushed the Simperfit:feature/deprecate-passing-null-to-locale branch 2 times, most recently from de68757 to 1adaabc Jul 7, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Status: Needs Review

@@ -527,7 +549,6 @@ public function getValidLocalesTests()
{
return [
[''],
[null],

This comment has been minimized.

Copy link
@Tobion

Tobion Jul 7, 2019

Member

this test provider is used in a lot of tests and in most of them a null locale is meant to be supported. so you can't just remove this here.

@alexislefebvre
Copy link
Contributor

left a comment

Deprecations are missing 2 words.

@Simperfit Simperfit force-pushed the Simperfit:feature/deprecate-passing-null-to-locale branch 2 times, most recently from bca34cf to c25ced6 Jul 7, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/deprecate-passing-null-to-locale branch 3 times, most recently from f093ebe to 815024a Jul 8, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 8, 2019

@@ -184,12 +184,25 @@ public function testAddResourceInvalidLocales($locale)
*/
public function testAddResourceValidLocales($locale)
{
if (null === $locale) {
$this->markTestSkipped('null is not a valid locale');

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 8, 2019

Member

why not remove null from getValidLocalesTests instead? this would look legit to me.

This comment has been minimized.

Copy link
@Simperfit

Simperfit Jul 8, 2019

Author Contributor

because of #32415 (comment) I don't know it seems that it used in places that can be null

EDIT: Just checked it's used

@Simperfit Simperfit force-pushed the Simperfit:feature/deprecate-passing-null-to-locale branch from 815024a to 088615d Jul 8, 2019

@fabpot

fabpot approved these changes Jul 8, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Thank you @Simperfit.

@fabpot fabpot merged commit 088615d into symfony:4.4 Jul 8, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 8, 2019

feature #32415 [Translation] deprecate passing a null locale (Simperfit)
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

@Simperfit Simperfit deleted the Simperfit:feature/deprecate-passing-null-to-locale branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.