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

Add support for Symfony ^4.0|^5.0 + fix various deprecations + fix travis CI #271

Merged
merged 39 commits into from
Jan 14, 2020

Conversation

vanputten
Copy link
Contributor

@vanputten vanputten commented Dec 6, 2019

First contribution ever 👶

  • Fix Travis CI for 3.4|^4.0|^5.0

  • Add support for Symfony ^4.4 and ^5.0 to bundle

  • Drops support for php <5.6 (impossible to support 5.0 otherwise)

  • Drops support for Symfony <3.4 (impossible to support 5.0 otherwise)

  • (Backwards) compatible with Symfony ^3.4|^4.0|^5.0

  • Compatible with all major twig versions

Fixes #270
Fixes #258
Fixes #261
Fixes #264
FIxes #265
FIxes #269

composer.json Outdated Show resolved Hide resolved
@vanputten vanputten changed the title Add support for Symfony 4.4 Add support for Symfony ~4.4|~5.0 Dec 9, 2019
composer.json Outdated Show resolved Hide resolved
@maxhelias
Copy link

You also need to modify the travis.yml file to fit the new constraint of PHP & Symfony

@vanputten
Copy link
Contributor Author

  • travis.yml has been updated to match the Symfony and PHP versions that this update should support
  • composer conflict on twig 1 and 2 has been hadden
  • support for symfony 4.1 has been restored

.travis.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
Controller/Controller.php Show resolved Hide resolved
@vanputten
Copy link
Contributor Author

vanputten commented Dec 11, 2019

  • PHP <5.6 was removed from travis since they no longer have support for it
  • Symfony <3.3 Support was dropped to allow Symfony 5.0 support (<3.3 is no longer maintained)

I did not see a way to maintain <3.3 support and at the same time add 5.0 support. kernel.root_dir has been removed since it has been replaced by kernel.project_dir since Symfony 3.3

@vanputten
Copy link
Contributor Author

@maxhelias any chance you could review the latest version/my response to your comments? Would be nice if we can get this merged before too many other repos start relying on my fork.

@maxhelias
Copy link

maxhelias commented Jan 6, 2020

@vanputten I did not have time to watch and try it. BTW, I'm not sure if it's a BC
@monteiro may have better vision than me.

@monteiro
Copy link
Collaborator

monteiro commented Jan 9, 2020

I will review this! Sorry for being out. My second son has just born.

@maxhelias
Copy link

Don't worry, I did a first review.
And congratulations 😃

@vanputten
Copy link
Contributor Author

Congratulations @monteiro ! And awesome if you could review this! 🎉

@kl3sk
Copy link
Contributor

kl3sk commented Jan 9, 2020

Congrats 👏 @monteiro I wait for my second daughter ^^, but this bundle require your review

Copy link
Collaborator

@monteiro monteiro left a comment

Choose a reason for hiding this comment

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

I think everything looks fine.
There are some things specific to symfony 3.4 which probably will be cleaned after the deprecation in November 2020.

Any red flags?

Thanks a lot for the review and @vanputten for the PR.

@@ -109,7 +119,7 @@ public function getTranslationsAction(Request $request, $domain, $_format)
$locales = $this->getLocales($request);

if (0 === count($locales)) {
throw new NotFoundHttpException();
return (new Response())->setStatusCode(Response::HTTP_NOT_FOUND);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For now I think it is ok. Symfony 3.4 will be a pain to support always so when it gets deprecated on November 2020 we can change this piece of code.

@vanputten
Copy link
Contributor Author

Thanks for the approval @monteiro !

@kl3sk
Copy link
Contributor

kl3sk commented Jan 14, 2020

Is this PR will be merged ? Not installable yet on SF5 from packagist

@vanputten
Copy link
Contributor Author

@kl3sk no, not merged yet. Only approved. Not sure who needs to do the merge and release

@kl3sk
Copy link
Contributor

kl3sk commented Jan 14, 2020

Bad news, @maxhelias and/or @monteiro are not able to do it ?

@monteiro
Copy link
Collaborator

I can merge. I will merge it today and create a release.

Controller/Controller.php Outdated Show resolved Hide resolved
Tests/Fixtures/app/config/base_config.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@monteiro
Copy link
Collaborator

@stof just applied all your suggestions. Thanks for the review.

@monteiro
Copy link
Collaborator

monteiro commented Jan 14, 2020

Now yes! I think everything is done! 😄 let's wait for tests to go green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants