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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Symfony 5 to the long list of supported versions #211

Open
wants to merge 21 commits into
base: master
from

Conversation

@damienalexandre
Copy link

damienalexandre commented Nov 25, 2019

This PR supersede #209 as the logic is different, here we keep support for all versions by using class_exists a lot.

Thanks to @lyrixx I discovered that the setUp() : void from PHPUnit issue is normally managed by the PHPUnit Bridge! 馃槏

@damienalexandre damienalexandre force-pushed the damienalexandre:symfony5-simple branch from 24448bd to 0e83eff Nov 25, 2019
@damienalexandre

This comment has been minimized.

Copy link
Author

damienalexandre commented Nov 25, 2019

Tests are ok on PHP 7.1+, still need to figure out why SYMFONY_PHPUNIT_REMOVE_RETURN_TYPEHINT=1 does not work as expected.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 26, 2019

Thanks for the work, let me know once you figure out the build failure and I happily merge..

@damienalexandre

This comment has been minimized.

Copy link
Author

damienalexandre commented Nov 26, 2019

Would it be okay to drop PHP 5.4?

I cannot make it work with it:

  • the phpunit bridge > 4.2 is PHP 5.5 only
  • the methods replacing "expectedException" annotations are not poly-filled in phpunit bridge <= 4.2

This is the last and only blocker for this PR.

image

@damienalexandre

This comment has been minimized.

Copy link
Author

damienalexandre commented Nov 28, 2019

Tests are now green but PHP 5.4 has been removed. It does not mean the bundle does not work anymore, it's just we can't run the tests on this version.

@@ -36,7 +38,12 @@ public function testItWorksDynamically()
$collectedShas['style-src'][] = $shaComputer->computeForStyle($style);
}));
$twig = new \Twig_Environment(new \Twig_Loader_Filesystem(__DIR__.'/templates'));
if (class_exists('Twig\Environment')) {

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2019

Contributor

this can be simplified. Namespaced class names exist in Twig 1.x and 2.x too (I don't remember which minor version, but Symfony requires such minor version in all maintained branches anyway as it uses namespaced classes everywhere)

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Dec 3, 2019

Author

This bundle is tested against un-maintained versions of Symfony, when installing with composer update --prefer-lowest we get Twig version 1.24.0 where there is no class Twig\Environment:

https://github.com/twigphp/Twig/blob/v1.24.0/lib/Twig/Environment.php

Maybe we should bump Twig minimal version but that will be a BC Break.

This condition is only here to avoid a direct deprecation notices:

1x: Using the "Twig_Environment" class is deprecated since Twig version 2.7, use "Twig\Environment" instead.
1x in IntegrationTest::testItWorksDynamically from Nelmio\SecurityBundle\Tests\Twig

1x: Using the "Twig_Loader_Filesystem" class is deprecated since Twig version 2.7, use "Twig\Loader\FilesystemLoader" instead.
1x in IntegrationTest::testItWorksDynamically from Nelmio\SecurityBundle\Tests\Twig

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2019

Contributor

Maybe we should bump Twig minimal version but that will be a BC Break.

I would not consider it as a BC break, given that composer will take care of preventing to install these old unmaintained Twig releases alongside the new version of this bundle.

{
// Compatibility with Symfony < 5 and Symfony >=5
if (!$e instanceof FilterResponseEvent && !$e instanceof ResponseEvent) {
return;

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2019

Contributor

this should be a throw new TypeError (or throw new InvalidArgumentException if PHP 5.x support is needed) rather than a silent failure. Otherwise, switching back to a typehint when dropping support for Symfony < 4.4 would be a BC break.

Btw, I suggest tagging all listener classes as @final, otherwise this typehint removal is a BC break.

### To release

* Symfony 5 compatibility added
* Drop tests for PHP 5.4

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2019

Contributor

Dropping tests for PHP 5.4 without dropping support for it in composer.json is a bad idea IMO. Projects running on PHP 5.4 should rather get an old test release than an uptodate release for which no testing has been done (as it might be incompatible).

@damienalexandre

This comment has been minimized.

Copy link
Author

damienalexandre commented Dec 3, 2019

Thanks for the review, I just switched all the return to a clear Exception message, added a test, added the final annotation and bumped PHP minimal version in composer.

@pyrech pyrech mentioned this pull request Dec 3, 2019
@damienalexandre

This comment has been minimized.

Copy link
Author

damienalexandre commented Dec 4, 2019

Twig is now bumped to 1.38, which is the minimal version we can use with support for the namespaced classes.

I also changed all Twig related code to use the namespaced classes to avoid some errors and make it "clean".

PHP Fatal error: Declaration of Nelmio\SecurityBundle\Twig\TokenParser\AbstractCSPParser::parse(Twig_Token $token) must be compatible with Twig\TokenParser\TokenParserInterface::parse(Twig\Token $token) in /**/NelmioSecurityBundle/Twig/TokenParser/AbstractCSPParser.php on line 57

This was referenced Dec 6, 2019
"twig/twig": "^1.24",
"symfony/yaml": "~2.3|~3.0|~4.0",
"symfony/phpunit-bridge": "^3.4.24|~4.0"
"twig/twig": "^1.38|^2.10",

This comment has been minimized.

Copy link
@spackmat

spackmat Dec 6, 2019

Please also include Twig ^3.0.

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Dec 6, 2019

Author

Yes great idea, just did that 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.