schmittjoh / JMSI18nRoutingBundle

Allows you to internationalize your routing
http://jmsyst.com/bundles/JMSI18nRoutingBundle
358 stars 159 forks source link

Replace GetResponseForExceptionEvent with ExceptionEvent (Symfony 5.x compatibility) #246

Closed gjuric closed 3 years ago

gjuric commented 3 years ago

This PR replaces GetResponseForExceptionEvent::getException() with ExceptionEvent::getThrowable() since GetResponseForExceptionEvent has been removed from Symfony starting with Symfony 5.0.

I've also bumped requirements for version of Symfony to 4.3 since this is when ExceptionEvent class was introduced to Symfony. This shouldn't be an issue since CookieSettingListeneris already using ResponseEvent that has been added to Symfony with v4.3 as well, this change was introduced in https://github.com/schmittjoh/JMSI18nRoutingBundle/pull/243

toooni commented 3 years ago

@gjuric I think I've fixed all the issues in https://github.com/schmittjoh/JMSI18nRoutingBundle/pull/247. Can you also take a look?

gjuric commented 3 years ago

Hi @toooni I've taken a look at your PR and IMHO you are changing many different things in a single PR. Since this project is AFAIK not actively maintained but only a few PRs merged here and there I am not sure this is the best approach if you want your PR to be merged.

I've updated my PR with bumped Symfony constraints and I believe this is a minimal change that needs to be done to make this bundle work on Symfony 5.x

toooni commented 3 years ago

@gjuric The problem is that to have PHP8 Support, almost all these changes are needed.

For example:

Fixing the PHPUnit deprecations was not needed. But there is no change in logic anywhere. I just replaced $this by self:: and moved the expectException stuff from the annotations, which isn't supported in PHPUnit 9+ inside the method.

Your PR does indeed work if you have Symfony 5, but the tests aren't. It isn't tested with Symfony 5. That's the reason why it does pass CI.

acasademont commented 3 years ago

Hi guys! First of all, thanks for submitting these 2 PRs, much appreciated. As you have perfectly realized, this bundle is not actively maintained anymore (I believe Symfony provides a powerful enough solution for 18n routes right no). Still, I'm happy to merge one of this PRs and tag a new release.