Allow Symfony 4. Fix Travis build #749

dunglas · 2017-11-24T15:45:48Z

Alternative to #747.

dunglas · 2017-11-24T16:23:47Z

And Travis is happy!

stof · 2017-11-24T17:29:21Z

.travis.yml

@@ -2,28 +2,27 @@ language: php

 sudo: false

-php: [5.3, 5.4, 5.5, 5.6, 7.0, hhvm]
+php: [5.4, 5.5, 5.6, 7.0, 7.1, hhvm]


Removing CI on 5.3 without dropping support is a no-go. This is the best way to drop support without telling it to Composer.

There is a trick to force it to keep running on 5.3 precise for now.

What do you prefer @stof? Using the trick for Travis (I know which one) or dropping the support?

To fix PHP 5.3 build you need to move it from general config to matrix, that uses precise OS version for that PHP version specifically.

@aik099 yes I know ;).

@dunglas , not fixed.

please add PHP 7.2 in this list.

And I would remove HHVM, as they decided to not care about PHP parity anymore.

stof · 2017-11-24T17:30:02Z

.travis.yml

      env: COMPOSER_FLAGS='--prefer-lowest --prefer-stable' SYMFONY_DEPRECATIONS_HELPER=weak
-    - php: 5.6
+    - php: 5.4


Not good. This would not test against a dev version of SF for instance, because they dropped support for 5.4. This job should run on the highest stable PHP version instead

stof · 2017-11-24T17:30:43Z

.travis.yml

      env: DEPENDENCIES=dev

 cache:
  directories:
    - $HOME/.composer/cache/files

 before_install:
-  - composer self-update
-  - if [ "$DEPENDENCIES" = "dev" ]; then perl -pi -e 's/^}$/,"minimum-stability":"dev"}/' composer.json; fi;
+  - if [ "$DEPENDENCIES" = "dev" ]; then composer config minimum-stability dev; fi;

 install:
  - composer update $COMPOSER_FLAGS


please also add vendor/bin/simple-phpunit install here, so that installation does not get mixed in the test output

stof · 2017-11-24T17:32:14Z

.travis.yml

      env: COMPOSER_FLAGS='--prefer-lowest --prefer-stable' SYMFONY_DEPRECATIONS_HELPER=weak
-    - php: 5.6
+    - php: 5.4
      env: DEPENDENCIES=dev

 cache:
  directories:
    - $HOME/.composer/cache/files


you should also ensure that the PHPUnit install performed by the bridge is cached whenever possible. See KnpLabs/KnpMenu@170f8d7 for an example

dunglas · 2017-11-24T18:52:30Z

@stof all comments fixed

sroze · 2017-12-04T16:01:27Z

ping @stof :)

ruudk · 2017-12-10T13:20:16Z

Any news on this? I feel like this is the last step to get Symfony2Extension compatible with SF 4 🙏

sroze · 2017-12-13T10:51:14Z

Alright, we can't block Symfony's Behat extension with that for that long. I'll remove Mink from the dependencies for now, do a release and maybe add it back at some point when this is merged.

aik099 · 2017-12-13T11:04:13Z

.travis.yml


-script: phpunit -v --coverage-clover=coverage.clover
+script: vendor/bin/simple-phpunit -v --coverage-clover=coverage.clover


What is simple-phpunit and how it's better than regular phpunit?

https://symfony.com/doc/current/components/phpunit_bridge.html

Here it is basically used to run tests both on PHP 5 and 7 (not easy with PHPUnit directly).

for you @dunglas

Never had any issues with this. Just use PHPUnit version that Travis CI gives you (the code you've changed was doing that) and PHPUnit version compatible with used PHP version would be picked automatically.

@aik099 Travis CI gives you a version of PHPUnit compatible with the PHP runtime. but when it gives you PHPUnit 4.8, it might give you a version older than 4.8.35, and so without the new namespaced class names for TestCase, making hard to have a testsuite compatible with PHPUnit 6 as well (which is used by default on Travis for PHP 7.2 for instance)

using a phpunit version installed through composer could also be working. But the drawback is that it would install an old unmaintained version of phpunit when using --prefer-lowest. And this does not make sense (we want to test the lowest versions of our deps, but we can still use an uptodate testing tool for that, avoiding to be hit by old PHPUnit bugs)

aik099 · 2017-12-13T11:07:09Z

Alright, we can't block Symfony's Behat extension with that for that long. I'll remove Mink from the dependencies for now, do a release and maybe add it back at some point when this is merged.

@sroze , is Mink a dependency of Behat? I thought that only MinkExtension needs it. If you plan to release new version of MinkExtension without Mink listed as dependency, then it would be very confusing.

@dunglas , I wonder why we just can't add Symfony 4 in composer.json and that's it? This is the way how it was done for other projects. In that case PR could be merged right away (as long as all tests pass of course).

sroze · 2017-12-13T11:08:16Z

@aik099 it's a dev dependency (through the mink extension), because of things like that:
https://github.com/Behat/Symfony2Extension/blob/c59756034c0837e11f36806893c308fe27fe9c4c/src/Behat/Symfony2Extension/ServiceContainer/Symfony2Extension.php#L51-L53

aik099 · 2017-12-13T11:12:51Z

@sroze , understood. Now all we're waiting for @dunglas to fix issues reported by @stof .

sroze · 2017-12-13T11:26:49Z

He fixed them already 😉

#749 (comment)

aik099 · 2017-12-13T11:32:03Z

@sroze , not the one where PHP 5.3 was removed from the build.

dunglas · 2017-12-13T11:34:43Z

@aik099 it is actually fixed: https://travis-ci.org/minkphp/Mink/jobs/306878662

aik099 · 2017-12-13T11:37:47Z

.travis.yml


 after_script:
-  - if [[ "7.0" != "$TRAVIS_PHP_VERSION" && "$TRAVIS_PHP_VERSION" != "hhvm" ]]; then wget https://scrutinizer-ci.com/ocular.phar && php ocular.phar code-coverage:upload --format=php-clover coverage.clover; fi
+  - wget https://scrutinizer-ci.com/ocular.phar


@dunglas , any idea why these 2 lines now can be performed for all PHP versions, but before for all but PHP7 and HHVM?

Because now ocular supports PHP 7 (it wasn't the case when the initial PHP 7 support has been added to .travis.yml I guess).

aik099 · 2017-12-13T11:39:13Z

... it is actually fixed: https://travis-ci.org/minkphp/Mink/jobs/306878662

Ups. When discussion happens on one code line, but changes related to it are added to another code line it's hard to trace. That's fixed good.

Now only my last concern about need to use simple-phpunit (see relevant comment) and we're done.

dunglas · 2017-12-13T11:40:11Z

@aik099 I replied to that, it's the only reliable way to run tests on both PHP versions.

aik099 · 2017-12-13T11:41:56Z

composer.json

    },

    "require-dev": {
-        "symfony/phpunit-bridge": "~2.7|~3.0"
+        "symfony/phpunit-bridge": "^3.3|^4.0"


You've increased 2.1 to 2.7 for symfony/css-selector. Why not doing something similar for symfony/phpunit-bridge as well?

The feature allowing to run both PHPUnit major versions has been added in 3.3, it doesn't work with earlier versions.

I guess Symfony 3.x works fine on PHP 5.3 it Travis CI is happy.

the PHPUnit bridge is special. We kept PHP 5.3 support in it (allowing Symfony 2.7 to use the latest bridge version for its own testing, instead of having to backport nice changes all the time).

aik099 · 2017-12-13T11:47:56Z

.travis.yml

      env: COMPOSER_FLAGS='--prefer-lowest --prefer-stable' SYMFONY_DEPRECATIONS_HELPER=weak
-    - php: 5.6
+    - php: 7.1


Is it needed to specify -php 7.1 several times? You can combine multiple env vars into single line as well (above COMPOSER_FLAGS and SYMFONY_DEPRECATIONS_HELPER are combined into single line).

Well, we want separate jobs for the lowest deps and the unstable deps (these cannot be combined together, as they are about different versions of deps).

and if you don't specify the PHP version for a job, Travis defaults to 5.5.

I see. So that was on purpose. Thanks for explaining.

aik099 · 2017-12-13T11:53:23Z

Now it seems PR is ready for mering. I can merge as well, but I need a confirmation, that there is all done what should be done.

stof · 2017-12-13T13:10:27Z

hmm, the Symfony PHPUnit Bridge installs PHPunit 6 on PHP 7.2 (as previous versions are triggering PHP deprecations on it). So it requires switching to namespaced PHPunit TestCase class.

dunglas · 2017-12-13T13:13:22Z

@stof can I do that in another PR (I'll drop the last commit for now)?

stof · 2017-12-13T13:20:54Z

@dunglas yes, sure

dunglas · 2017-12-13T13:21:22Z

done

aik099 · 2017-12-13T13:27:50Z

Merging, thanks @dunglas .

sroze · 2017-12-13T13:52:20Z

A release would be 🎆

aik099 · 2017-12-13T14:12:47Z

I thought, that this could not be as easy as just merging the PR.

@sroze , shouldn't you be using something like "behat/mink": "~1.7@dev", in your composer.json. This way you'll be getting latest dev code for testing.

This is how all drivers are connected to Mink.

Allow Symfony 4. Fix Travis build.

dunglas force-pushed the sf4 branch from 5c982be to 0865edc Compare November 24, 2017 15:46

dunglas changed the title ~~Alternative to #747.~~ Allow Symfony 4. Fix Travis build. Nov 24, 2017

dunglas force-pushed the sf4 branch from 0865edc to f55f770 Compare November 24, 2017 16:06

Allow Symfony 4. Fix Travis build.

f1d4b02

dunglas force-pushed the sf4 branch 2 times, most recently from 17d7109 to f1d4b02 Compare November 24, 2017 16:23

stof requested changes Nov 24, 2017

View reviewed changes

dunglas added 2 commits November 24, 2017 19:21

Fix comment

a88b92d

Always run ocular

a7f6a45

sroze approved these changes Nov 27, 2017

View reviewed changes

sroze mentioned this pull request Dec 4, 2017

Support for Symfony 4 Behat/Symfony2Extension#126

Merged

3 tasks

aik099 reviewed Dec 13, 2017

View reviewed changes

aik099 changed the title ~~Allow Symfony 4. Fix Travis build.~~ Allow Symfony 4. Fix Travis build Dec 13, 2017

aik099 reviewed Dec 13, 2017

View reviewed changes

dunglas force-pushed the sf4 branch from 9a88679 to a7f6a45 Compare December 13, 2017 13:14

aik099 merged commit 808702d into minkphp:master Dec 13, 2017

dunglas deleted the sf4 branch December 13, 2017 13:34

sroze mentioned this pull request Dec 13, 2017

Symfony 4 support added #747

Closed

mxr576 pushed a commit to tamasd/Mink that referenced this pull request May 3, 2019

Allow Symfony 4. Fix Travis build (minkphp#749)

a26554b

Allow Symfony 4. Fix Travis build.


		script: phpunit -v --coverage-clover=coverage.clover
		script: vendor/bin/simple-phpunit -v --coverage-clover=coverage.clover

Allow Symfony 4. Fix Travis build #749

Allow Symfony 4. Fix Travis build #749

Conversation

dunglas commented Nov 24, 2017 • edited

dunglas commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Nov 24, 2017

sroze commented Dec 4, 2017

ruudk commented Dec 10, 2017

sroze commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Dec 13, 2017

sroze commented Dec 13, 2017 • edited

aik099 commented Dec 13, 2017

sroze commented Dec 13, 2017

aik099 commented Dec 13, 2017

dunglas commented Dec 13, 2017

Choose a reason for hiding this comment

dunglas Dec 13, 2017 • edited

Choose a reason for hiding this comment

aik099 commented Dec 13, 2017 • edited

dunglas commented Dec 13, 2017

aik099 Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Dec 13, 2017

stof commented Dec 13, 2017

dunglas commented Dec 13, 2017 • edited

stof commented Dec 13, 2017

dunglas commented Dec 13, 2017

aik099 commented Dec 13, 2017

sroze commented Dec 13, 2017

aik099 commented Dec 13, 2017

dunglas commented Nov 24, 2017 •

edited

sroze commented Dec 13, 2017 •

edited

dunglas Dec 13, 2017 •

edited

aik099 commented Dec 13, 2017 •

edited

aik099 Dec 13, 2017 •

edited

dunglas commented Dec 13, 2017 •

edited