PHP 8 support #9665

Cheddam · 2020-08-29T00:11:37Z

This PR introduces a nightly build, representing PHP 8, and achieves baseline support for it in framework. This is the first step towards shipping full PHP 8 support in core, which we hope to ship with CMS Recipe 4.7 around the same time that PHP 8 goes stable.

Notable changes

(Commentary by @sminnee)

These are necessary to get PHP 8 working but are worth bearing in mind by reviewers:

phpunit has to run on a sminnee fork, which has PHP 8 support added. Similar setup to sminnee/phpunit-mock-objects
one internal interface has had its inconsistent method signature cleared up (see PHP 8: DataQueryManipulator uses deprecated default value #9666)
league/csv now supports both version 8 and 9, instead of just version 8. this means that unless a user has specified that they need version 8, they may upgrade to version 9 as part of an update. IMO not technically a breakage but might be worth listing in upgrade notes. If people haven't pinned a requirement but have made direct use of this package (bad practise, but people might do this) then their code might break.

Note also that in this initial PR, on PHP 8 we are running composer with --ignore-platform-reqs. Before removing this all dependent packages will need to release an official PHP 8 version. However, as long as our tests pass, this shouldn't block merge. I'd suggest that we stand up a PR that removes this after merging this, and see what's required to get it installing.

Fixes #9666
Fixes #9667

To do:

Revert HttpMethodBypass to use array_reduce without &s
Test CSV generation for bom-marker inclusion in at least 1 test, and get the 7.1 build working (which uses league/csv 8)
Refactor expectedExceptionMessageRegExp into a setter method in the test

sminnee · 2020-08-29T05:02:33Z

This seems fine but also a little arbitrary. IMO it would be better to get this PR including the php 8 nightly build on travis, and surveying what else needs to be remedied.

Arguably, spurious failures on the nightly line might be a distraction, but I would argue:

Let's see, and address that if/when it does (it's easy enough to comment out a build)
If we get tests passing on php 8 now, people will be able to running CMS 4.7 on their php 8 servers, rather than needing upgrade to 4.8 or 4.9 or whatever.

Since PHP 8 is only a few months away (expected Nov 2020) that seems reasonable.

sminnee · 2020-08-29T19:51:48Z

I see that some dependencies (embed at least) are pinned to ^7 support; I might try a build with ignore-platform-reqs for nightly to see how far it gets.

sminnee · 2020-08-30T05:11:28Z

OK I got some way with this, notes:

The remapper capability in CsvBulkLoader needs to be implemented in league/csv 9
phpunit 5 throws warnings with PHP 8 when the Configuration section is updated. We'll need to decide whether to support newer versions of PHPUnit (worth its own issue) or to fork and fix PHPUnit 5, as I have done with phpunit-mock-objects
I'm still installing with ignore-platform-reqs. Dependent packages will need PRs added to them to mark PHP 8 as supported and fix as needed.
The ClassInfo stuff still needs fixing, from the looks of it
There are some bugs where class non-existence doesn't seem to be detected

Cheddam · 2020-08-30T05:16:30Z

I had a few of these already covered, was planning on raising individual PRs as I made progress 😅

(TIL that nightly is the PHP 8 build - I'd assumed Travis just didn't have one yet.)

I'll pull down your work and see if I can make some further headway.

sminnee · 2020-08-30T05:51:49Z

@Cheddam sorry just saw this after I force-pushed with a squashed fix into the league/csv. I'll stop force-pushing now! :P

sminnee · 2020-08-30T05:53:32Z

I had a few of these already covered, was planning on raising individual PRs as I made progress 😅

I like the idea of this in principle, however I'm a little nervous about merging code that's not the subject of a test run. Which leads you to "add a PHP 8 build as part of the merge" which in turn leads to "fix all the PHP 8 failures before merging" :|

One way out that would be to configure a PHP 8 build that has some stuff disabled, and then re-enable some things in subsequent PRs that fix it. Sometimes that's easier said than done, though.

Cheddam · 2020-08-30T06:12:38Z

I think it'd make sense to merge a PHP 8 build marked to ignore failures, and then iterate on getting it green across multiple PRs. The key thing we want to ensure is that we're not breaking builds for older PHP versions in the process of fixing them for PHP 8. Worst case, a 'fix' for PHP 8 doesn't quite hit the mark in a way that isn't clear from the change in build failure, and we take another crack at it.

I'd be pretty stoked if we manage to ship support by November 26th / CMS 4.7.0, but I don't think this single PR should remain open until we do - it risks getting into rebase hell.

Let's see how far we get in the next few days. If there's no light at the end of the tunnel, I suggest we mark the PHP 8 build as ignored, get this PR reviewed and merged, and iterate from there.

sminnee · 2020-08-30T06:14:22Z

FYI I'm looking into a PHP 8 compatible version of PHPUnit 5.7 here http://github.com/sminnee/phpunit

Will publish as sminnee/phpunit, replaces phpunit/phpunit, once it's working. Same thing I did for phpunit-mock-objects.

sminnee · 2020-08-30T06:23:47Z

To be honest I think with that forked version of phpunit and leaving --ignore-platform-reqs in place we'll be pretty close to a passing build.

The changes already in this PR will need a solid review, but I don't think it's as big as it otherwise might be.

sminnee · 2020-08-30T08:35:09Z

OK, with the forked and patched version of phpunit, we're down to just a handful of failures:

Deprecated method

It looks like this is disabled by default in PHP 8 so can probably only run libxml_disable_entity_loader if PHP_MAJOR_VERSION < 8.

SilverStripe\Core\Tests\ConvertTest

testXML2Array - libxml_disable_entity_loader() is deprecated

Different tokens in parsed code

I believe this is what you were working on. Check out the i18n text collector too - it might have its own implementation/

SilverStripe\Core\Tests\ObjectTest

testParseClassSpec -Failed asserting that two arrays are equal.

SilverStripe\i18n\Tests\i18nTextCollectorTest

testCollectFromCodeNamespace - Failed asserting that two arrays are equal.

Different behaviour when classes don't exist

My guess is that non-existent classes throw some different kind of error / result in the Reflection system,.

SilverStripe\Core\Tests\ClassInfoTest

testNonClassName - Failed asserting that exception message 'Class "IAmAClassThatDoesNotExist" does not exist'

SilverStripe\ORM\Tests\DataListTest

testSimpleFilterWithNonExistingComparisator - Failed asserting that exception message 'ReflectionException: Class "DataListFilter.Bogus" does not exist in /home/travis/build/silverstripe/silverstripe-framework/src/Core/Injector/InjectionCreator.php:17
testInvalidModifier - Failed asserting that exception message 'ReflectionException: Class "DataListFilter.invalidmodifier" does not exist in /home/travis/build/silverstripe/silverstripe-framework/src/Core/Injector/InjectionCreator.php:17

sminnee · 2020-08-30T08:42:30Z

Given the above, I don't think we need allowed-failures to merge this – I think we can probably get it over the line?

Cheddam · 2020-08-30T10:37:28Z

Yep, sounds sensible - unfortunately I was fighting my PHP 8 testing environment this evening so didn't make any further inroads, but can help tackle the remaining items over the next few days. Exciting times, thanks for getting stuck in on this!

sminnee · 2020-08-30T19:43:25Z

Lol Travis is my PHP 8 env! ;-)

If you get a shareable vagrant or docker based environment running id definitely be interested :-)

src/Core/Injector/InjectorNotFoundException.php

Cheddam · 2020-09-06T11:11:38Z

Seems that PHP 8 builds on Travis have ceased to function for the time being 😞 (some context here)

Once they're operational again, I'll be keen to see whether we're green after the last few patches I pushed up.

sminnee · 2020-09-06T23:48:16Z

Fair enough. Although... I think '8.0snapshot' is probably the better build to run with if it's available.

sminnee · 2020-09-06T23:55:46Z

It looks like nightly builds are now operational but 8.0snapshot is not, so forget I said anything :p

sminnee · 2020-09-07T00:45:42Z

@Cheddam let's rebase this onto 4, it looks like a recently added test of @dnsl48 needs the named keys on its test data pushed up 1 more level as I did in 98ef847

sminnee

I approve this, but I've been heavily involved in its development ;-)

dhensby

This looks good and it's exciting to see PHP 8 support being added to the 4.x line.

It's a shame we can't just upgrade to PHPUnit 8, but there has been some work to the ss-upgrader tool to add an easier upgrade path for that in SS-5.

src/Control/Middleware/ConfirmationMiddleware/HttpMethodBypass.php

tests/php/Dev/SapphireTestTest/DataProvider.php

tests/php/Forms/GridField/GridFieldExportButtonTest.php

tests/php/ORM/DataListTest.php

sminnee · 2020-09-08T01:22:18Z

One thing to highlight for @silverstripe/core-team

I added a prefer-lowest build to this PR and identified a few places where we lie about our requirements:

Support for nikic/php-parser ^2 was lost in 25759ff.
Support for monolog/logger < 1.16 was lost in 7ab55a4

So I've updated the composer requirements. Because support hasn't worked for 2 years and 18 months respectively, I think this is okay but wanted to highlight it as part of the review of this PR. Alternatively we could look to fix these two areas so the older packages can still be used, but I think that's probably overkill.

dhensby · 2020-09-09T20:18:42Z

Looking good except for some failures with CSVs in the 7.1 build (--prefer-lowest) probably like some of the other dependencies we actually rely on a more specific version than we declare

sminnee · 2020-09-15T05:28:37Z

💚

sminnee · 2020-09-15T05:33:02Z

Shall we remove "WIP" from the subject Garion or is there still more tidyup you want to do before the next round of review?

Cheddam · 2020-09-15T05:37:47Z

Happy with the contents; I'll tweak the commit structure a bit first now that it's green, and then drop the WIP 👍

Throws errors in PHP 8

Hopefully this has better PHP 8 support.

…provider

This provides 5.7.28 with fixes for PHP 8 support.

This build will ensure that older versions of packages that we claim work with silverstripe/framework do, in fact, work. ‘Composer install’ changed to ‘composer update’ as that’s what’s being executed in the absence of a composer.lock, and better clarifies intent. Note that: * Support for nikic/php-parser ^2 was lost in 25759ff. * Support for monolog/logger < 1.16 was lost in 7ab55a4

This maintains support for embed 3.0.0.

.travis.yml

src/Core/Injector/InjectorNotFoundException.php

sminnee force-pushed the pulls/4/php8-fqcn-token branch 4 times, most recently from 4ad8980 to 0e62c63 Compare August 30, 2020 04:31

Cheddam changed the title ~~NEW Support T_NAME_QUALIFIED token in ClassInfo::parse_class_spec()~~ (WIP) Initial support for PHP 8 Aug 30, 2020

sminnee force-pushed the pulls/4/php8-fqcn-token branch from 0e3f2b4 to 98ef847 Compare August 30, 2020 05:49

sminnee closed this Aug 30, 2020

sminnee reopened this Aug 30, 2020

sminnee mentioned this pull request Aug 30, 2020

RFC: Deprecate SapphireTest (was: Split unit testing support into silverstripe/test-support) #9668

Open

sminnee mentioned this pull request Aug 30, 2020

PHP 8: Names matter on injector's constructor arguments #9667

Closed

sminnee reviewed Sep 4, 2020

View reviewed changes

src/Core/Injector/InjectorNotFoundException.php Outdated Show resolved Hide resolved

sminnee mentioned this pull request Sep 7, 2020

FIX: Drop use of abandoned package in favour of its replacement. silverstripe/silverstripe-crontask#57

Merged

Cheddam force-pushed the pulls/4/php8-fqcn-token branch from 378db4a to 47973f5 Compare September 7, 2020 01:31

sminnee approved these changes Sep 7, 2020

View reviewed changes

dhensby reviewed Sep 7, 2020

View reviewed changes

sminnee changed the title ~~PHP 8 support~~ [WIP] PHP 8 support Sep 8, 2020

emteknetnz added the impact/high label Sep 8, 2020

Cheddam force-pushed the pulls/4/php8-fqcn-token branch from 579184e to 338b2db Compare September 15, 2020 05:00

Sam Minnee and others added 11 commits September 15, 2020 17:40

FIX: Avoid pass-literal-by-reference warning in PHP 8

5cb3d07

FIX: Avoid passing non-class to get_parent_class()

09fb33e

Throws errors in PHP 8

NEW: Allow league/csv ^9

b3dd279

Hopefully this has better PHP 8 support.

FIX: Fix test mistakenly labelling content as name arguments in data …

57bdef3

…provider

FIX: Switch to sminnee fork of phpunit 5.7

fb9e6e5

This provides 5.7.28 with fixes for PHP 8 support.

FIX Disable libxml_disable_entity_loader() calls in PHP 8

bad0662

FIX Support PHP 8 namespace tokens in i18nTextCollector

44685ec

FIX Allow quotes in expected ReflectionExceptions within tests

f1c94e6

FIX Remove extraneous @Depends annotations

099ee2d

FIX: Ensure Embed options are always array

85252ca

This maintains support for embed 3.0.0.

Cheddam force-pushed the pulls/4/php8-fqcn-token branch from 338b2db to 85252ca Compare September 15, 2020 05:40

Cheddam changed the title ~~[WIP] PHP 8 support~~ PHP 8 support Sep 15, 2020

sminnee requested review from dhensby and dnsl48 September 17, 2020 02:55

dhensby approved these changes Sep 18, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

src/Core/Injector/InjectorNotFoundException.php Show resolved Hide resolved

ScopeyNZ approved these changes Sep 18, 2020

View reviewed changes

dhensby merged commit ae0ece2 into silverstripe:4 Sep 18, 2020

Cheddam mentioned this pull request Sep 22, 2020

PHP 8 Compatibility across core #9704

Closed

22 tasks

bimthebam mentioned this pull request Feb 8, 2022

i18nTextCollector produces corrupt output / namespaces when running under PHP8.0 #10227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8 support #9665

PHP 8 support #9665

Cheddam commented Aug 29, 2020 •

edited

Loading

sminnee commented Aug 29, 2020

sminnee commented Aug 29, 2020 •

edited

Loading

sminnee commented Aug 30, 2020

Cheddam commented Aug 30, 2020

sminnee commented Aug 30, 2020

sminnee commented Aug 30, 2020 •

edited

Loading

Cheddam commented Aug 30, 2020 •

edited

Loading

sminnee commented Aug 30, 2020

sminnee commented Aug 30, 2020 •

edited

Loading

sminnee commented Aug 30, 2020 •

edited

Loading

sminnee commented Aug 30, 2020

Cheddam commented Aug 30, 2020

sminnee commented Aug 30, 2020

Cheddam commented Sep 6, 2020

sminnee commented Sep 6, 2020

sminnee commented Sep 6, 2020

sminnee commented Sep 7, 2020

sminnee left a comment

dhensby left a comment

sminnee commented Sep 8, 2020 •

edited

Loading

dhensby commented Sep 9, 2020

sminnee commented Sep 15, 2020

sminnee commented Sep 15, 2020

Cheddam commented Sep 15, 2020

PHP 8 support #9665

PHP 8 support #9665

Conversation

Cheddam commented Aug 29, 2020 • edited Loading

Notable changes

sminnee commented Aug 29, 2020

sminnee commented Aug 29, 2020 • edited Loading

sminnee commented Aug 30, 2020

Cheddam commented Aug 30, 2020

sminnee commented Aug 30, 2020

sminnee commented Aug 30, 2020 • edited Loading

Cheddam commented Aug 30, 2020 • edited Loading

sminnee commented Aug 30, 2020

sminnee commented Aug 30, 2020 • edited Loading

sminnee commented Aug 30, 2020 • edited Loading

Deprecated method

Different tokens in parsed code

Different behaviour when classes don't exist

sminnee commented Aug 30, 2020

Cheddam commented Aug 30, 2020

sminnee commented Aug 30, 2020

Cheddam commented Sep 6, 2020

sminnee commented Sep 6, 2020

sminnee commented Sep 6, 2020

sminnee commented Sep 7, 2020

sminnee left a comment

Choose a reason for hiding this comment

dhensby left a comment

Choose a reason for hiding this comment

sminnee commented Sep 8, 2020 • edited Loading

dhensby commented Sep 9, 2020

sminnee commented Sep 15, 2020

sminnee commented Sep 15, 2020

Cheddam commented Sep 15, 2020

Cheddam commented Aug 29, 2020 •

edited

Loading

sminnee commented Aug 29, 2020 •

edited

Loading

sminnee commented Aug 30, 2020 •

edited

Loading

Cheddam commented Aug 30, 2020 •

edited

Loading

sminnee commented Aug 30, 2020 •

edited

Loading

sminnee commented Aug 30, 2020 •

edited

Loading

sminnee commented Sep 8, 2020 •

edited

Loading