Skip to content

Conversation

@nstapelbroek
Copy link
Contributor

Heya 👋

This PR should fix the deprecation notices for https://github.com/symfony/translation/blob/4.1/Dumper/FileDumper.php#L52

If you have any additions or changes I'd love to know.

Also adds a compiler pass to assure the service definition remains
unchanged on older Symfony versions
{
public function process(ContainerBuilder $container)
{
if (Kernel::MAJOR_VERSION <= 3) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, consider doing failure first 🤔

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I really like this. Thank you.
If you like to add a test that would be perfect.

Great work!

@nstapelbroek
Copy link
Contributor Author

Hmmm, I'm kind of stuck writing a test for this 🤔. Because I'm unable to mock or overwrite the Kernel::MAJOR_VERSION I'm forced to write an if-statement in my tests with different assertions or I'll mark the test as skipped. Both cases feel not optimal.

If you have any tips on this situation I'd love to hear. Otherwise I'm OK with merging

@Nyholm
Copy link
Member

Nyholm commented Jun 26, 2018

Thank you for working on this.

I wound not care with the sf major version. Let travis handle that. But Im not sure how I would write the tests... Maybe to check if the sf major version is less than 4, then make sure setBackup is called.. But that would be the exact same logic as in the CompilerPass...
Anyhow, Im also happy with this.

Thank you

@Nyholm Nyholm merged commit 689d324 into php-translation:master Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants