Skip to content

Conversation

@emodric
Copy link
Contributor

@emodric emodric commented Aug 17, 2017

PHP 7.2 throws a warning when count is called with a null argument which breaks Symfony apps (and presumably others too), so this makes sure the AbstractProxyClient::$queue is never null when counting it in AbstractProxyClient::flush method.

@dbu
Copy link
Contributor

dbu commented Aug 17, 2017

looks reasonable, yes. i am trying to fix the 1.4 build in #369 - once i merged that, i will ask you to rebase this branch.

@emodric
Copy link
Contributor Author

emodric commented Aug 17, 2017

@dbu Sure, no problem!

@dbu
Copy link
Contributor

dbu commented Aug 17, 2017

ok, here we go. just fixed the build on 1.4.

can you please also add a test that will show this now works, that would fail without your fix? php warnings break the build so just flushing an empty queue and asserting that this operates normally and does not make any invalidation requests would be good.

@emodric
Copy link
Contributor Author

emodric commented Aug 18, 2017

Actually, it seems that FOS\HttpCache\Tests\Unit\ProxyClient\AbstractProxyClientTest::testFlushEmpty already fails without the patch on PHP 7.2 (it passes on 7.1 since 7.1 does not emit the warning), and the patch fixes it. I've rebased on top of 1.4, but what do you want to do about testing 7.2?

@dbu
Copy link
Contributor

dbu commented Aug 18, 2017

ah ok. lets add 7.1 and 7.2 to the test matrix then. can you do it in your pull request? you will need to adjust .travisci.yml as well to use the right phpunit on all php 7 versions.

@emodric
Copy link
Contributor Author

emodric commented Aug 18, 2017

@dbu I've added PHP 7.1 and 7.2 to the test matrix, but I'm not very well versed in shell scripting so if you have a better suggestion on how to handle 7.0, 7.1 and 7.2 versions, do tell.

.travis.yml Outdated
# Install deps
- composer update $COMPOSER_FLAGS --dev --prefer-source --no-interaction
- sh -c "if [ '$TRAVIS_PHP_VERSION' = '7' ]; then wget https://phar.phpunit.de/phpunit-5.7.phar; fi"
- sh -c "if [ '$TRAVIS_PHP_VERSION' = '7.0' ] || [ '$TRAVIS_PHP_VERSION' = '7.1' ] || [ '$TRAVIS_PHP_VERSION' = '7.2' ]; then wget https://phar.phpunit.de/phpunit-5.7.phar; fi"
Copy link
Contributor

Choose a reason for hiding this comment

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

lets try with bash: bash -c "if [[ '$TRAVIS_PHP_VERSION' == '7.'* ]] ; then wget https://phar.phpunit.de/phpunit-5.7.phar; fi"

and yes, bash is a really messed up thing :-/. the * is outside the quotes...

@emodric
Copy link
Contributor Author

emodric commented Aug 18, 2017

@dbu Now it looks like as if it's randomly failing. Can you restart the build?

@dbu
Copy link
Contributor

dbu commented Aug 18, 2017

yay! thanks a lot. will merge and then try to merge upstream - guess there will be conflicts with .travis.yml

i think the build on 1.4 has been flaky for a while. in 2.0 its better, so i won't bother anymore.

@dbu dbu merged commit 727974a into FriendsOfSymfony:1.4 Aug 18, 2017
@emodric
Copy link
Contributor Author

emodric commented Aug 18, 2017

Thanks @dbu :)

@dbu
Copy link
Contributor

dbu commented Aug 18, 2017

i tagged 1.4.3 with this and plan to release 2.0.2 shortly, waiting for the build to run through

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants