Add support for PHP 8 #17

dshoreman · 2020-10-26T07:24:14Z

Description and Background

With the recent changes in ChrisB9/php8-xdebug that fix xdebug options, update composer and (afaict) remove the profiler that caused errors previously, there seem to be only two errors remaining in the tests on PHP 8.

Both errors are with the assert() lines, which throw an AssertionError in v8, but only trigger a warning (converted to ErrorException) when ran under v7. This is caused by a change in php8 which now enables the assert.exception option by default.

I considered a couple methods, but simply enabling the option by default seems to be the cleanest route. The option itself was added in v7, so this change just makes things consistent across both versions.

How Has This Been Tested

Existing tests fixed, ran with docker-compose run php composer test && docker-compose run php8 composer test.

Psalm is happy, but the infection tests are currently still failing as a result of xdebug connection issues.

Checklist

My code follows the PSR coding guideline.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Resolves #15

Since PHP 7, `assert()` would throw an `AssertionError` if the ini option `assert.exception` was enabled, but it didn't become the default until PHP 8 so instead warnings were converted to an `ErrorException`. This commit updates phpunit's bootstrap script to make sure that the `assert.exception` option is enabled for both PHP 7 *and* 8, enabling us to switch the expectation to a proper `AssertionError` in the tests.

codecov · 2020-10-26T09:24:17Z

Codecov Report

Merging #17 into main will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##                main       #17   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       236       236           
===========================================
  Files             21        21           
  Lines            632       632           
===========================================
  Hits             632       632

Impacted Files	Coverage Δ	Complexity Δ
src/Conditional/BasicExpression.php	`100.00% <ø> (ø)`	`14.00 <0.00> (ø)`
src/Literal/ShellWord.php	`100.00% <ø> (ø)`	`26.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43be21d...1731971. Read the comment docs.

dshoreman · 2020-10-26T09:32:12Z

The grumphp-config Composer dep was blocking Travis build due to lack of composer-2 support, so I've updated it to 4.0.1 which adds compatibility for it. Now that it's running again, it looks like the errors I'm getting locally with the mutation tests (xdebug could not connect to debugging client) are also happening in Travis, albeit only for php nightly.

I've tried updating the EXPOSE line from 9000 to 9003 in the php8-xdebug dockerfile and rebuilding from that local image, but that made no visible difference. Running phpinfo shows the settings from php8-xdebug (eg mode=profile,debug,develop etc) are in fact being applied so that seems fine at least... though I'm not really sure where to go from there.

ChrisB9 · 2020-10-29T11:06:50Z

Thanks alot for your effort.
I think a major issue is, that the dependencies are not all PHP 8.0 ready and that may cause troubles on all fronts.
I might actually already merge this because the tests are running and the only reason why infection doesn't work is due to external factors. Maybe though we could first extend the dependencies here or extract that into a second pull request? Such as phpunit ^9.0, psalm ^4.0, infection ^0.19 and php-invoker ^3.0. (I know it will fail thanks to grumphp and webmozart/glob but both will add PHP 8.0 support soon.

dshoreman · 2020-10-30T09:42:16Z

I've just updated phpunit to 9.x and (so far) it's still green, although there are some new warnings now:

[snipped xdebug errors - see below]
PHPUnit 9.4.2 by Sebastian Bergmann and contributors.

Warning:       xdebug.mode=coverage has to be set in php.ini
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Error:         PHP extension pcntl is required for enforcing time limits

Running the migration command has kindly removed most line breaks so I gotta fix that before I can diff them, but with any luck we won't need to adjust too much.

Assuming the phpunit stuff goes well, I'll push that up then see if there's time to look at the rest before I pass out

For reference, these are the xdebug warnings from php itself:

Creating shellcommandbuilder_php8_run ... done
Xdebug: [Step Debug] Could not connect to debugging client. Tried: internal:9003 (fallback through xdebug.client_host/xdebug.client_port) :-(
Xdebug: [Profiler] File '/tmp/debug/cachegrind.out.1' could not be opened.
> vendor/bin/phpunit -c tests/phpunit.xml --testdox --color=always
Xdebug: [Step Debug] Could not connect to debugging client. Tried: internal:9003 (fallback through xdebug.client_host/xdebug.client_port) :-(
Xdebug: [Profiler] File '/tmp/debug/cachegrind.out.12' could not be opened.

ChrisB9 · 2020-10-30T13:38:38Z

Regarding the XDebug error. It seems like this error happens because XDebug cannot establish a connection to PHPStorm.
https://3.xdebug.org/docs/step_debug#log
I changed the XDebug default settings disabling debug by default.
https://github.com/ChrisB9/php8-xdebug/pull/8/files

dshoreman · 2020-10-30T14:51:50Z

That would make sense as I don't have PHPStorm installed :D
Thanks for the update. I'll pull the new docker bits later and give it a try, but that might just be all the warnings taken care of.

The "cachegrind.out" and /tmp/debug errors can be solved easily a couple ways, depending which you prefer:

Add ./tests/xdebug:/tmp/debug (or similar) to volumes: list in ShellCommandBuilder/docker-compose.yml
- How I fixed it locally (for now), seems to do the job
- The debug dir requires 775 permissions, so it'd need to be tracked in git
- To avoid unecessary cruft, ./tests/xdebug/* would be added to .gitignore
Same as above, but in the php8-xdebug repo
RUN mkdir /tmp/debug && chmod 775 $_ somewhere in php8-xdebug/php-dev-8.dockerfile

Option 3 could be the better route? But I'm not sure if other tools need to access the profiling data. If they do then we might need to go with option 1 regardless, in which case I can add it to this PR.

The only other question is that of the PHP minimum version. Phpunit requires >=7.3 and infection requires ^7.4 || ^8, but for now since both are dev-deps I think we'd be able to stay on 7.2 to avoid also updating that container. With 7.2 at EOL though, bumping that to 7.3 or .4 could be something for the future.

ChrisB9 · 2020-10-30T15:07:09Z

I actually added this line too
RUN mkdir /tmp/debug && chmod 775 $_ somewhere in php8-xdebug/php-dev-8.dockerfile

dshoreman · 2020-10-31T12:49:13Z

It turns out the chmod isn't needed when it's created directly in php-dev-8.dockerfile. I should've tested that beforehand, sorry! Good news though is everything's so far running smooth from what I can see.

I've updated phpunit and infection locally and both are now running without issue.
Infection flagged one mutation so I've added a test to solve that too.

PHP 8 has also removed pcntl (used for timing) from core due to lack of maintainers. I've added that (and removed the aforementioned chmod) among some other cleanup which I'll post in a PR to php8-xdebug shortly.

If there's anything that needs changing in that PR first, please let me know and I'll get to it asap - I'll hold pushes to this PR until that's merged in case the pcntl error ends up causing CI to fail

ChrisB9 · 2020-10-31T13:09:07Z

I just merged it, so let's see whether this now works. Thanks for digging into this!

dshoreman · 2020-10-31T13:17:59Z

No worries, thanks for the fast merge! I'll get the commits pushed right now... 🙏

dshoreman · 2020-10-31T14:22:27Z

Forgot about infection requiring 7.4, so I've added --ignore-platform-reqs to those versions in .travis.yml. I did try removing it from nightly too, but spatie/phpunit-watcher still trips that with a ^7.2 requirement.

Tests themselves are running again, but there's still some work to get coverage/infection working on Travis. I'll dig into that some more either today/tomorrow and update the PR if I figure anything out.

7.2 tests seem to only be failing currently due to latest infection using newer syntax (guessing trailing commas). We could just disable the infection task under 7.2? If that sounds good, I can add it in while I'm playing with the coverage bits.

Turning out to be quite the adventure, but... I think we're getting there!

ChrisB9 · 2020-10-31T16:09:44Z

did try removing it from nightly too, but spatie/phpunit-watcher still trips that with a ^7.2 requirement.

For phpunit-watcher, I quickly created a pull request to their repository spatie/phpunit-watcher#124 so that we can omit --ignore-platform-reqs

7.2 tests seem to only be failing currently due to latest infection using newer syntax (guessing trailing commas). We could just disable the infection task under 7.2? If that sounds good, I can add it in while I'm playing with the coverage bits.

I wish I could drop 7.2 support altogether but sadly phpsu and our agency needs it.
Which line is the culprit for those failing infections?

Turning out to be quite the adventure, but... I think we're getting there!
That's great, I am sure we will get to the bottom of this

dshoreman · 2020-10-31T16:57:50Z

The issue in 7.2 isn't failing infections, but Infection PHP itself using syntax that's incompatible with 7.2:

$ composer infection

> vendor/bin/infection --threads=4 --only-covered --min-msi=100 --min-covered-msi=100 --ansi

PHP Parse error:  syntax error, unexpected ')' in /home/travis/build/phpsu/ShellCommandBuilder/vendor/infection/infection/src/Container.php on line 223

Parse error: syntax error, unexpected ')' in /home/travis/build/phpsu/ShellCommandBuilder/vendor/infection/infection/src/Container.php on line 223

Script vendor/bin/infection --threads=4 --only-covered --min-msi=100 --min-covered-msi=100 --ansi handling the infection event returned with error code 255

My first thought was infection simply wouldn't run, but seeing your PR to phpunit-watcher hinted at an ^A | ^B syntax for Composer's version specifiers. That's especially useful now because I only just realised phpunit was also refusing to run on 7.2:

$ composer test

> vendor/bin/phpunit -c tests/phpunit.xml --testdox --color=always

This version of PHPUnit is supported on PHP 7.3 and PHP 7.4.

You are using PHP 7.2.27 (/home/travis/.phpenv/versions/7.2.27/bin/php).

Script vendor/bin/phpunit -c tests/phpunit.xml --testdox --color=always handling the test event returned with error code 1

The command "composer test" exited with 1.

I'll try updating the composer.json to accept either version for phpunit and infection; hopefully that should sort it without having to exclude 7.2 from the infection/phpunit runs.

dshoreman · 2020-11-01T10:54:43Z

Some 30 builds and one sleep later, Travis is finally happy across all 7.x jobs. Huzzah!

Still got some work to do for the nightly job to pass, but once it does I'll rebase to clean up the huge string of commits.

Bumps infection, phpunit and php-invoker to the latest versions to get support for php8 and the xdebug config changes. With the update to phpunit comes a new xml config schema. The new `coverage` section is made up of the `filter.whitelist` value (now 'include') and the `log.coverage-*` values as children of `report`.

After updating the upstream image to include pcntl, Infection PHP flagged a mutation in `Collection\CollectionTuple::__toArray()` where `$this->value instanceof ShellInterface` could be changed to `false`. That results in the first half of the ternary never executing. To catch it, this commit adds a test for `__toArray()` that uses an instance of `ShellBuilder`, complimenting the existing string-based test.

After updating psalm, 17 errors were uncovered - all either a MissingConstructor error or PropertyNotSetInConstructor - largely depending on whether it found it in the `final` class or a parent. In all cases, the root cause was simply lack of a default value.

The most recent version of Infection PHP that will run on 7.2 is 0.15.x. v0.19 adds support for php 8, but also drops support for versions older than 7.4 meaning we also need to allow for 0.18.x to get 7.3 working.

Phpunit 9 won't run under 7.2, but phpunit 8 doesn't understand the config for v9. This here is an attempt at a "dual boot" of sorts. The old config is restored to a separate file so we can pass the legacy config for 7.2 but still use the new config format for other versions.

Manually installs the latest copy of phpunit 8.x for php 7.3 because while phpunit9 is compatible with php7.3, infection is not able to read the new xml config schema that phpunit uses as of v9.

Copies the legacy config rather than moving it so that xmllint can scan it, which was the last thing preventing the 7.x build from passing. Since the config is always available under the same filename, we also no longer need to pass a custom `--configuration` flag for phpunit.

For php 8, we need to manually set the new `xdebug.mode` option to enable coverage mode, but it won't work if we simply do something like `php -d xdebug.mode=coverage composer infection` because InfectionPHP spawns new processes that would lose that option. To provide for this, there's the `--initial-tests-php-options` shell option to infection that will pass that on when it calls PHP for the initial test run, which is the only time we need coverage enabled. Since this is only required in PHP 8 and there are no other parts using the `PHPUNIT_LEGACY` env variable, we can instead test against php 7.x to remove the need for PHPUNIT_LEGACY entirely.

Infection PHP hasn't released the commit adding php 8 support yet, so for now we reference it directly as a fallback with 1.24 preemptively set ready for when it's actually available. phpro/grumphp still requires ^7.3 so we can't stop ignoring platform requirements just yet, but enforcing 4.0.1 explicitly at least removes the error about 4.0.0 requiring the composer v1 plugin api package.

dshoreman · 2020-11-05T12:50:50Z

@ChrisB9 I've removed allow-failures from nightly builds (and it's still green!) but we're still waiting on a couple dependencies before we can remove --ignore-platform-reqs entirely from composer.

It all seems to be working fine though, so I'll create a separate issue with the details on that.
Think this is finally good to review! 🎉

ChrisB9

Looks good so far
Thanks for all your effort here!

.travis.yml

Co-authored-by: Chris Ben <[email protected]>

dshoreman · 2020-11-06T13:30:43Z

@ChrisB9 Good idea to limit the ignores, committed that now so touch wood it'll all pass soon and it won't flag anything else!

dshoreman added 2 commits October 26, 2020 06:29

Update grumphp to 4.0.1 for Composer 2 support

26cc417

dshoreman changed the title ~~[WIP] Fix errors with assert in tests under php8~~ Fix errors with assert() in tests under php8 Oct 26, 2020

ChrisB9 added the hacktoberfest-accepted label Oct 29, 2020

dshoreman mentioned this pull request Oct 31, 2020

Add pcntl extension and improve installation of others ChrisB9/php8-xdebug#9

Merged

dshoreman force-pushed the fix/php8 branch 2 times, most recently from 76ff453 to 753b930 Compare October 31, 2020 13:52

dshoreman force-pushed the fix/php8 branch 8 times, most recently from d989c4b to 09a0e75 Compare November 1, 2020 05:12

dshoreman force-pushed the fix/php8 branch from 42c452a to 51faf08 Compare November 1, 2020 17:26

dshoreman force-pushed the fix/php8 branch 11 times, most recently from 2b92cb7 to 9514227 Compare November 5, 2020 10:55

dshoreman added 8 commits November 5, 2020 11:02

Update psalm

cd38d35

After updating psalm, 17 errors were uncovered - all either a MissingConstructor error or PropertyNotSetInConstructor - largely depending on whether it found it in the `final` class or a parent. In all cases, the root cause was simply lack of a default value.

Expand version constraints for 7.2 and 7.3 support

41f1f80

The most recent version of Infection PHP that will run on 7.2 is 0.15.x. v0.19 adds support for php 8, but also drops support for versions older than 7.4 meaning we also need to allow for 0.18.x to get 7.3 working.

Fix Infection job in travis under php7.3

8dd0ae5

Manually installs the latest copy of phpunit 8.x for php 7.3 because while phpunit9 is compatible with php7.3, infection is not able to read the new xml config schema that phpunit uses as of v9.

dshoreman force-pushed the fix/php8 branch from 9514227 to e8c107f Compare November 5, 2020 11:31

dshoreman force-pushed the fix/php8 branch from e9d4d7d to 2497b8f Compare November 5, 2020 12:29

dshoreman changed the title ~~Fix errors with assert() in tests under php8~~ Add support for PHP 8 Nov 5, 2020

dshoreman mentioned this pull request Nov 5, 2020

Remove --ignore-platform-reqs from Composer commands in Travis builds #18

Open

ChrisB9 requested changes Nov 5, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

Only ignore php requirements

1731971

Co-authored-by: Chris Ben <[email protected]>

ChrisB9 approved these changes Nov 6, 2020

View reviewed changes

ChrisB9 merged commit ec95a23 into phpsu:main Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PHP 8 #17

Add support for PHP 8 #17

dshoreman commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

dshoreman commented Oct 26, 2020 •

edited

Loading

ChrisB9 commented Oct 29, 2020

dshoreman commented Oct 30, 2020 •

edited

Loading

ChrisB9 commented Oct 30, 2020

dshoreman commented Oct 30, 2020

ChrisB9 commented Oct 30, 2020

dshoreman commented Oct 31, 2020 •

edited

Loading

ChrisB9 commented Oct 31, 2020

dshoreman commented Oct 31, 2020

dshoreman commented Oct 31, 2020

ChrisB9 commented Oct 31, 2020

dshoreman commented Oct 31, 2020 •

edited

Loading

dshoreman commented Nov 1, 2020 •

edited

Loading

dshoreman commented Nov 5, 2020 •

edited

Loading

ChrisB9 left a comment

dshoreman commented Nov 6, 2020

Add support for PHP 8 #17

Add support for PHP 8 #17

Conversation

dshoreman commented Oct 26, 2020 • edited Loading

Description and Background

How Has This Been Tested

Checklist

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

dshoreman commented Oct 26, 2020 • edited Loading

ChrisB9 commented Oct 29, 2020

dshoreman commented Oct 30, 2020 • edited Loading

ChrisB9 commented Oct 30, 2020

dshoreman commented Oct 30, 2020

ChrisB9 commented Oct 30, 2020

dshoreman commented Oct 31, 2020 • edited Loading

ChrisB9 commented Oct 31, 2020

dshoreman commented Oct 31, 2020

dshoreman commented Oct 31, 2020

ChrisB9 commented Oct 31, 2020

dshoreman commented Oct 31, 2020 • edited Loading

dshoreman commented Nov 1, 2020 • edited Loading

dshoreman commented Nov 5, 2020 • edited Loading

ChrisB9 left a comment

Choose a reason for hiding this comment

dshoreman commented Nov 6, 2020

dshoreman commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

dshoreman commented Oct 26, 2020 •

edited

Loading

dshoreman commented Oct 30, 2020 •

edited

Loading

dshoreman commented Oct 31, 2020 •

edited

Loading

dshoreman commented Oct 31, 2020 •

edited

Loading

dshoreman commented Nov 1, 2020 •

edited

Loading

dshoreman commented Nov 5, 2020 •

edited

Loading