Skip to content

[Translation][FrameworkBundle] Add failing test for BC break regarding translation resources. #25079

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Translation][FrameworkBundle] Add failing test for BC break regarding translation resources. #25079

wants to merge 1 commit into from

Conversation

jenkoian
Copy link
Contributor

@jenkoian jenkoian commented Nov 21, 2017 •
edited
Loading

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? yes (highlights a BC break)
Deprecations? no
Tests pass? no (on purpose in this case)
Fixed tickets N/A (but see #23057 (comment))
License MIT
Doc PR N/A

Adding a failing test case to prove a BC break caused by #23057

@jenkoian jenkoian changed the base branch from master to 2.7 November 21, 2017 11:52
hhamon
hhamon reviewed Nov 21, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/Translation/TranslatorTest.php Outdated
->expects($this->any())
->method('load')
->will($this->returnValue($this->getCatalogue('fr', array(
'foo' => 'foo (fr)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be inlined I think.

hhamon
hhamon reviewed Nov 21, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/Translation/TranslatorTest.php Outdated
->expects($this->any())
->method('load')
->will($this->returnValue($this->getCatalogue('fr', array(
'bar' => 'bar (fr)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be inlined I think.

@jenkoian jenkoian mentioned this pull request Nov 21, 2017
Closed
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Nov 21, 2017
@nicolas-grekas
Copy link
Member

@jenkoian would you like to work on a fix? anyone else who would like to investigate?

@jenkoian
Copy link
Contributor Author

jenkoian commented Jan 11, 2018 •
edited
Loading

@nicolas-grekas yeah I can have a go at a fix, before I waste any time though, is it agreed this is a bug? Expected behaviour is that loaders added after init should work the same as loaders added before init?

@nicolas-grekas
Copy link
Member

any hint from anyone? @yceruto maybe?

@jenkoian
Copy link
Contributor Author

See @stof's comments on #23057 (comment)

Creating a circular reference between the translator and the loader to have the loader reconfiguring the translator regarding what resources should be loaded by the loader is indeed a use case we never intended to support.

So likely, I can just close this PR? Although there are instances this could manifest outside a circular reference I guess, like the test case.

@nicolas-grekas
Copy link
Member

OK, I'm closing then. @stof if you think otherwise, please report here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hhamon hhamon hhamon left review comments

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review Translation
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
4 participants
@jenkoian @nicolas-grekas @hhamon @carsonbot