allow php 8 #1313

garak · 2020-12-12T15:59:20Z

Q	A
Branch?	2.0
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1309
License	MIT
Doc PR	not needed

Allow PHP 8

fbourigault

Thank you for handling this!

To keep the matrix pilosophy I introducted in #1202, I requested some changes.

The major ideas in the matrix is to:

test each supported php version with latest dependencies.
test latest supported php version with code coverage enabled.
test lowest dependencies with oldest and latest supported php versions.
test each supported Symfony version with it's minimum required PHP.

.travis.yml

@@ -21,7 +21,7 @@ matrix:
    - php: '7.2'


fbourigault · 2020-12-13T22:34:37Z

.travis.yml

@@ -21,7 +21,7 @@ matrix:
    - php: '7.2'

    # Enable code coverage with the latest supported PHP version
-    - php: '7.3'
+    - php: '7.4'


This should be 8.0. This would match the comment.

fbourigault · 2020-12-13T22:36:19Z

.travis.yml

@@ -40,20 +40,23 @@ matrix:
        - SYMFONY_VERSION=3.4.*
    - php: '7.1'
      env:
-        - SYMFONY_VERSION=4.3.*
+        - SYMFONY_VERSION=4.4.*

    - php: '7.2.9'
      env:
        - SYMFONY_VERSION=5.0.*


Should this be kept as Symfony 5.0 isn't supported anymore?

fbourigault · 2020-12-13T22:38:35Z

.travis.yml


    - php: '7.2.9'
      env:
        - SYMFONY_VERSION=5.0.*

-    - php: '7.2.9'
+    - php: '7.4'


This should not change as minimum supported version of Symfony 5.1 is 7.2.

fbourigault · 2020-12-13T22:40:21Z

.travis.yml

      env:
        - SYMFONY_VERSION=5.1.*

+    - php: '8.0'


This should be 7.2.9 as Symfony 5.2 requires at least this version.

fbourigault · 2020-12-14T08:16:31Z

.travis.yml

      env:
        - COMPOSER_FLAGS="--prefer-lowest"

    # Test each supported Symfony version with lowest supported PHP version
-    - php: '7.1'
+    - php: '5.5.9'


This one must be kept on 7.1 as it's the minimal PHP version required by this bundle!

fbourigault · 2020-12-14T23:38:43Z

The matrix looks good now, but the build is failing.

7.X failures are unrelated with this PR and I started to fix those in #1315.

dbu · 2020-12-15T14:47:05Z

#1315 fixed the existing builds, can you please rebase this PR on master?

garak · 2020-12-15T15:04:06Z

No luck for php8, imagemagick does not compile 😞

garak · 2020-12-15T15:05:47Z

Related: Imagick/imagick#358

fbourigault · 2020-12-15T17:01:31Z

You can add an IMAGINE_ENABLED environment variable with de default value of true, install imagick extension only if this variable value is true and set the variable to false for PHP 8.0 jobs.

A few tests will be skipped as imagick won't be available, but that's acceptable as users won't be able to have such setup (php 8 + imagick).

garak · 2020-12-16T08:16:03Z

Still not working since a dependency can't install. Related issue: php-enqueue/enqueue-dev#1108

dbu · 2020-12-16T08:25:00Z

as enqueue is optional, could we have the php 8 build remove it from composer before installing and have the tests that rely on enqueue skip when enqueue is not available?

coveralls · 2020-12-16T08:58:31Z

Coverage decreased (-0.4%) to 81.047% when pulling 11c7f89 on garak:allow-php8 into 4704b34 on liip:master.

garak · 2020-12-16T09:11:33Z

OK, I think we're almost there...
It looks like phpunit 7.5 doesn't work well with php8, so I think we should give up with the possibility of testing php8 with it.
I would remove SYMFONY_PHPUNIT_VERSION="7.5" from line 41 of travis config, what do you think?

dbu · 2020-12-16T09:48:16Z

yeah, phpunit 7 does not work on php 8. afaik phpunit 8 now works on both php 7 and 8, lets try that? phpunit 9 brings a couple of BC breaks that means we would need to adjust assertions and such. we eventually want to do that, but should not mix in too many things into one PR

can you try explicitly setting it to the current 8.x version?

garak · 2020-12-16T09:57:54Z

Unfortunately, even phpunit 8.5 is failing (due to missing return type on tearDown)

dbu · 2020-12-16T10:36:54Z

this has been fixed in symfony 3.4.23
i suggest changing the composer.json to "symfony/framework-bundle": "^3.4.23|^4.3|^5.0",

.travis.yml

fbourigault · 2020-12-16T10:51:46Z

.travis.yml

+      yes | pecl install imagick;
+    fi
+  - if [[ "$ENQUEUE_ENABLED" == "false" ]]; then
+      composer remove --dev enqueue/enqueue-bundle;


You should add the --no-update option as the update is performed at install step.

garak · 2020-12-16T11:07:36Z

Any idea of the reasons of last failures? 😞 Why images are not found?

fbourigault · 2020-12-16T11:55:08Z

Could you rebase please? I will look at those failure localy.

fbourigault · 2020-12-17T20:57:13Z

.travis.yml

        - XDEBUG_MODE=coverage
        - PHPUNIT_FLAGS="-v --coverage-text --coverage-clover var/build/clover.xml"

    # Minimum supported dependencies with the latest and oldest supported PHP versions
    - php: '7.1'
      env:
        - COMPOSER_FLAGS="--prefer-lowest"
-    - php: '7.3'
+    - php: '8.0'


This is the last failing job. I think we should stick with php 7.4 here as having php 8.0 with --prefer-lowest would work only if any dependencies doesn't declare supporting php with the >= operator without being compatible with php 8.0.

Note: Symfony does the same (see https://github.com/symfony/symfony/blob/5.x/.travis.yml#L23-L30)

yeah, i agree. we could also drop the "latest php version prefer-lowest" completely. the main thing we care about is "lowest php version with prefer-lowest".

The highest supported PHP version with lowest dependencies tests the case where, in a project, an unrelated dependency lock one of our dependency to a lower version than expected.

I took the idea from Symfony IIRC but maybe this is already covered with other jobs. I’m neutral about keep/remove this job from the matrix.

there is always a chance of some combinations of versions having a problem. but builds are fast and parallel, so lets change this to 7.4 for now.

lsmith77 · 2020-12-30T12:58:15Z

closing in favor of #1325

fbourigault suggested changes Dec 13, 2020

View reviewed changes

fbourigault reviewed Dec 14, 2020

View reviewed changes

fbourigault mentioned this pull request Dec 15, 2020

Fix tests related to a missing configuration entry #1315

Merged

garak force-pushed the allow-php8 branch from 9e49141 to 19fbf3b Compare December 15, 2020 14:48

fbourigault reviewed Dec 16, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

fbourigault reviewed Dec 16, 2020

View reviewed changes

garak force-pushed the allow-php8 branch from bd11e6d to 189f740 Compare December 16, 2020 12:06

allow php 8

11c7f89

garak force-pushed the allow-php8 branch from 189f740 to 11c7f89 Compare December 16, 2020 12:06

fbourigault reviewed Dec 17, 2020

View reviewed changes

fbourigault mentioned this pull request Dec 17, 2020

GitHub Actions #1320

Merged

nicwortel mentioned this pull request Dec 24, 2020

Support PHP 8 #1325

Merged

lsmith77 closed this Dec 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow php 8 #1313

allow php 8 #1313

garak commented Dec 12, 2020

fbourigault left a comment

This comment was marked as resolved.

fbourigault Dec 13, 2020

fbourigault Dec 13, 2020

fbourigault Dec 13, 2020

fbourigault Dec 13, 2020

fbourigault Dec 14, 2020

fbourigault commented Dec 14, 2020

dbu commented Dec 15, 2020

garak commented Dec 15, 2020

garak commented Dec 15, 2020

fbourigault commented Dec 15, 2020

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

coveralls commented Dec 16, 2020 •

edited

Loading

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

fbourigault Dec 16, 2020

garak commented Dec 16, 2020

fbourigault commented Dec 16, 2020

fbourigault Dec 17, 2020

dbu Dec 19, 2020

fbourigault Dec 19, 2020

dbu Dec 19, 2020

lsmith77 commented Dec 30, 2020

allow php 8 #1313

allow php 8 #1313

Conversation

garak commented Dec 12, 2020

fbourigault left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

fbourigault commented Dec 14, 2020

dbu commented Dec 15, 2020

garak commented Dec 15, 2020

garak commented Dec 15, 2020

fbourigault commented Dec 15, 2020

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

coveralls commented Dec 16, 2020 • edited Loading

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

garak commented Dec 16, 2020

dbu commented Dec 16, 2020

Choose a reason for hiding this comment

garak commented Dec 16, 2020

fbourigault commented Dec 16, 2020

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

lsmith77 commented Dec 30, 2020

coveralls commented Dec 16, 2020 •

edited

Loading