-
Notifications
You must be signed in to change notification settings - Fork 5
Add symfony 5.0 compatibility #18
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 symfony 5.0 compatibility #18
Conversation
@Toflar do you want to keep support the old symfony versions? Or do you want to set min requirement to 4.4? |
7b9dfca
to
7606188
Compare
7606188
to
41fd7d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Toflar do you want to keep support the old symfony versions? Or do you want to set min requirement to 4.4?
I would like to keep them because 3.4 is LTS for another two years and I'm not sure for Sulu and for eZ Publish (cc @andrerom) but for Contao we have LTS for another 2 years as well :)
We may raise them to 3.4 conistently though.
composer.json
Outdated
"require-dev": { | ||
"friendsofphp/php-cs-fixer": "^2.12", | ||
"phpunit/phpunit": "^5.7.27", | ||
"symfony/phpunit-bridge": "dev-master", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no stable release yet that works for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SetUpTearDownTrait
is needed to support this wide range on php versions with phpunit. This was added symfony/symfony#32922 which is only available in 4.4.
I can also workaround this problem by refractoring the tests that no tearDown
and setUp
is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok could fix it by set the SYMFONY_PHPUNIT_VERSION
see https://github.com/Toflar/psr6-symfony-http-cache-store/pull/18/files#diff-180cc0a648d9fd0a0ca3ede96ea0aeef
tests/Psr6StoreTest.php
Outdated
$entries = $cacheItem->get(); | ||
|
||
$this->assertInternalType('array', $entries, 'Entries are stored in cache.'); | ||
$this->assertTrue(is_array($entries), 'Entries are stored in cache.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be \is_array()
. Normally, I have my CS fixer set up so that it automatically fixes those. Did I miss that here maybe or did you just not run the CS fixer? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the ci didn't tell me todo this 😄 maybe you want to activate https://prettyci.com/ for this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also just add the cs fixer to the travis job, but don't worry, I'll do that :)
In eZ Platform we don't yet use this in stable version, we use this in 3.x which will be on Symfony 4.4. So should rather check with FosHttpCache 2.x users if they need this on Symfony 3.4LTS or not, there probably are some. |
I see. Well, as we're using it in Contao it's out of scope anyway :) So this will remain PHP 5.6 and Symfony 3.4 compatible for another 2 years. Also, it's just one class so it's rather easy for us to remain compatible with all the different combinations. @alexander-schranz so I'm waiting with this for symfony/symfony#34036, right? |
5eef667
to
74de0cd
Compare
@Toflar yes would wait for symfony/symfony#34036 getting merged. Sadly with phpunit bridge its currently not possible to test agains PHP 8 (would need: symfony/symfony#33642). |
c4c2856
to
58c6d94
Compare
58c6d94
to
a3a7c22
Compare
I'm totally fine using the phpunit bridge :) I would just like to use stable versions if possible but we could've used |
@Toflar yes stable version is used now by set phpunit version to 7 for newer php version in the newly created |
Squashed, merged and released as 2.1.1. Thank you so much @alexander-schranz! |
@Toflar Thank you! |
Fix compatibility to symfony 5.0.
Needs:
TODO:
symfony/phpunit-bridge
to use correct PhpUnit version for PHP version to run not into prefer lowest errors caused by phpunit1x: The default value of the "$secure" and "$samesite" arguments of "Symfony\Component\HttpFoundation\Cookie::__construct"'s constructor will respectively change from "false" to "null" and from "null" to "lax" in Symfony 5.0, you should define their values explicitly or use "Cookie::create()" instead. 1x in Psr6StoreTest::testLookupWithVaryOnCookies from Toflar\Psr6HttpCacheStore
Element 'phpunit', attribute 'syntaxCheck': The attribute 'syntaxCheck' is not allowed