search

lunetics-org / LocaleBundle

Locale Bundle for symfony
MIT License
187 stars 76 forks source link

Don't use the deprecated interface with Symfony 4.2 #208

Closed hacfi closed 5 years ago

hacfi commented 5 years ago

See https://github.com/symfony/symfony/pull/27476

hacfi commented 5 years ago

@dbu How about we merge this, release it as a patch (2.6.4) and then update the test matrix and composer.json to release a new minor version (2.7.0)?

hacfi commented 5 years ago

Thanks for the feedback @dbu! Will update the PR later today and remove hhvm which isn't supported by PHPUnit itself. I will check if there is a way to expect an exception within a method in newer PHPUnit versions.

On Wed, 2 Jan 2019 at 09:19, David Buchmann notifications@github.com wrote:

@dbu commented on this pull request.

looks good. i disagree with the expected exception and think we should simply drop hhvm testing rather than ignore errors.

In Tests/Controller/LocaleControllerTest.php https://github.com/lunetics/LocaleBundle/pull/208#discussion_r244677758:

@@ -31,7 +35,6 @@ public function testControllerThrowsException() $request->setLocale('en'); $localeController = new LocaleController($this->getRouterMock(), $metaValidatorMock);

  • $this->setExpectedException('\InvalidArgumentException');

can we set a minimum phpunit version that has setExpectedException ? its much better to expect the exception at the place we want it to happen, rather than the annotation on the test.

In .travis.yml https://github.com/lunetics/LocaleBundle/pull/208#discussion_r244677809:

@@ -38,7 +37,13 @@ matrix:

  • SYMFONY_VERSION='3.4.*@BETA'
  • php: 7.1 env:
    • SYMFONY_VERSION='4.0.*@BETA'
    • SYMFONY_VERSION='4.0.*'
    • php: 7.2
  • env:
    • SYMFONY_VERSION='4.1.*'
    • php: hhvm
  • dist: trusty
  • allow_failures:

lets drop hhvm from the build matrix completely, imho it became irrelevant.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lunetics/LocaleBundle/pull/208#pullrequestreview-188604612, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaLKc-YuOu-pp2NdnCy6yAO_bFh3hsrks5u_GuEgaJpZM4ZZRqG .

dbu commented 5 years ago

way to expect an exception within a method in newer PHPUnit versions.

there is, the method has been renamed to expectException (and there is also expectExceptionMessage if needed).

hacfi commented 5 years ago

I think this is good to go now!

@dbu So should we release this as 2.6.4 and then require PHP 7 to release a new minor version (2.7.0)?

dbu commented 5 years ago

thanks! i released 2.6.4.

i don't think we urgently need a new version only for dropping php 7. when we drop versions, we should probably go to 7.1 directly. if you want to work on that, i can review it and we can merge it. but i'd wait with tagging until there are actual features / fixes as well.