PHP 8 Support #828

driesvints · 2020-10-30T10:51:08Z

Working on providing full PHP 8 support for php-webdriver. We'll need this for Laravel Dusk.

Currently using this PR to run the builds.

phpunit.xml.dist

driesvints · 2020-10-30T16:14:45Z

Currently waiting on sminnee/phpunit-mock-objects#6 to be merged and tagged before continuing here.

sminnee · 2020-11-01T18:58:59Z

You may wish to run your php 8 build with --ignore-platform-reqs. Without dropping PHP 5.6 and 7.0 support, any upgrading to at least phpunit 7 || 8 || 9, you'll likely run into issues.

If you're looking to patch my phpunit forks, please test the new packages against phpunit/phpunit first, and confirm that the tests pass - this is what I got blocked on trying to upgrade some of the dependencies.

driesvints · 2020-11-02T14:53:34Z

@sminnee I've added "sminnee/phpunit" which indeed works thanks.

I'm a bit stuck atm... For some bizar reason when I only run the build for PHP 8 it succeeds (see this build and my second to last commit) but when I re-enable all the other builds it fails and claims it cannot install PHP CS Fixer even though I've added --ignore-platform-req php (see this build and my last commit). Apparently for some reason it seems that parallel builds in Travis influence each other somehow? In any case the PHP 8 nightly build should properly install because of the --ignore-platform-req php flag.

driesvints · 2020-11-02T14:54:37Z

Also the PHP Code Style Github Action fails atm but I suspect that has nothing to do with the current PR (since it's run on PHP 7.4). I believe when you trigger it on main it'll also fail. Probably something got changed on phpstan's side.

driesvints · 2020-11-02T16:39:16Z

I think I'm a little closer at figuring it out. As you can see below there's two nightly builds, one that fails and one that passes. I'm sure that the one that passes has the --ignore-platform-req php and the one that fails doesn't.

There's two questions:

Why are there two builds?
How do we get the --ignore-platform-req php to apply on the one that fails?

.travis.yml

driesvints · 2020-11-02T16:54:52Z

Okay this one is ready for review now. Please note that I had to disable code coverage for PHP 8 for now because of an issue with PHPUnit and xdebug. See https://travis-ci.com/github/php-webdriver/php-webdriver/builds/197177332

This was fixed in a newer version of PHPUnit later on but I guess sminnee/phpunit doesn't has that one yet (cc @sminnee). I'm however a bit too tired to figure out that specific one and don't believe it should be a blocker for this PR.

In general I want to suggest for this project to drop support for older PHP versions so you can remove the PHPUnit dependency overrides. As most of these PHP versions are EoL I don't think it makes sense in maintaining them anymore. However, I understand that this is a personal choice of the maintainer of course.

Anyway, would appreciate a review, merge and tag so we can finally patch Laravel Dusk. Let me know if anything else needs to be done. Thanks all! 🙂

.travis.yml

composer.json

driesvints · 2020-11-02T17:02:12Z

For some bizar reason the PHP 5.6 til 7.2 builds are now failing after my last commit... https://github.com/php-webdriver/php-webdriver/pull/828/checks?check_run_id=1343276205

.php_cs.dist

williamdes · 2020-11-03T00:19:08Z

Hi @OndraM !
This PR started with a PHP 8 support and I finished it with a phpunit 5.7->9 support.
Because only the CI runs phpunit tests on PHP 5 I did set a script block to reverse some method signatures like I do on Debian (#828 (comment)).

I upgraded things and did fix everything that was red.
Some fixes/hacks are needed and can not be removed. But we can merge this and remove those things when we drop php 5 support.

driesvints · 2020-11-03T08:21:23Z

@williamdes fantastic work 🙌

.php_cs.dist

williamdes · 2020-11-03T13:19:53Z

Hi @GrahamCampbell

Just delete this line.

I can not do that because phpcs will report me to use some phpunit asserts that are not right.
See: 26c6de1
Deprecated: sebastianbergmann/phpunit#3369

tests/unit/Interactions/Internal/WebDriverCoordinatesTest.php

.travis.yml

tests/functional/WebDriverTestCase.php

OndraM · 2020-11-16T11:13:17Z

LGTM, thanks everyone!

The final question is how to rebase & fixup those 25 commits to keep the changes contained. Any preferences @driesvints @williamdes?
I suggest keeping just two commits, one which adds Travis builds, second one with all the changes from @williamdes :).

williamdes · 2020-11-16T11:19:38Z

@OndraM all good to me. I'll leave it up to @williamdes to decide on that since he did the bulk of the work here

Done, I did a reset soft and created 2 commits from the changes.
And added Dries Vints <dries@vints.io> as a co author on 06ff635

WTF: phpstan broken ?? https://github.com/php-webdriver/php-webdriver/pull/828/checks?check_run_id=1406013380#step:6:37

driesvints · 2020-11-16T12:03:55Z

@williamdes can you try to retrigger the static analysis github action? It might just be a hiccup.

williamdes · 2020-11-16T12:26:05Z

Hi @ondrejmirtes
This phpstan error seems to be recent, is it an internal bug ?
https://github.com/php-webdriver/php-webdriver/runs/1406254306?check_suite_focus=true#step:6:28

ondrejmirtes · 2020-11-16T12:29:55Z

@williamdes phpstan/phpstan#3956

OndraM · 2020-11-16T12:33:46Z

So maybe we can temporarily add constrain to composer.json to avoid latest PHPStan version(s)?

ondrejmirtes · 2020-11-16T12:35:58Z

It'd be easier to:

Avoid using non-UTF-8 PHP code in your project. It's 2020 :)
The root issue is reported here (Handling non-UTF-8 strings clue/reactphp-ndjson#21). I don't know what to do about it right now. I don't know anything about the encoding of the analysed codebse so I don't know what to convert it from to UTF-8.

OndraM · 2020-11-16T12:52:23Z

@ondrejmirtes AFAIK this whole codebase is UTF-8. Is there a way how to find out which file causes the trouble?

I also tried this and find no non-UTF-8 files... https://gist.github.com/rmetzler/2947828

williamdes · 2020-11-16T13:08:05Z

I tried on my local workstation:

composer analyse
# fails
vendor/bin/phpstan.phar analyze ./lib ./tests --level 2 -c phpstan.neon --ansi
# fails
vendor/bin/phpstan.phar analyze ./lib ./tests --level 2 -c phpstan.neon --ansi
# fails
vendor/bin/phpstan.phar analyze ./lib ./tests --level 2 -c phpstan.neon --ansi --debug -vvv
# SUCCESS
vendor/bin/phpstan.phar analyze ./lib ./tests --level 2 -c phpstan.neon --ansi
# SUCCESS, WTF
rm -rf /tmp/phpstan
vendor/bin/phpstan.phar analyze ./lib ./tests --level 2 -c phpstan.neon --ansi
# fails

@ondrejmirtes --debug -vvv was added in the latest release, and before this release the analysis was successing.
Is it by coincidence this new error ?

ondrejmirtes · 2020-11-16T13:21:09Z

The reason why it fails are these two commits:

* phpstan/phpstan-src@95101a0

phpstan/phpstan-src@14928bf

So it's not a new bug, it just surfaces an error condition that was failing silently before, possibly causing error reports to be missed.

driesvints · 2020-11-16T14:04:09Z

@OndraM @williamdes is there any way we can work around this for now so we can merge this PR? Afaik this has nothing to do with this PR and it'll probably also fail on the main branch?

OndraM · 2020-11-16T14:14:26Z

From what I found, the cause is in WebDriveKeys. However we need to declare WebDriver key codes there... Doing this is nothing invalid from PHP point of view.

@ondrejmirtes So this error is caused by clue/reactphp-ndjson#21 , which is used by PHPStan, and which causes non-unicode characters to break JSON parsing used internally in PHPStan?

OndraM · 2020-11-16T14:15:04Z

@OndraM @williamdes is there any way we can work around this for now so we can merge this PR? Afaik this has nothing to do with this PR and it'll probably also fail on the main branch?

Yes, will probably do this. I just wanted to know what is the root cause for this and whether it is related to this PR.

OndraM · 2020-11-16T14:35:29Z

Workaround for the PHPStan issue here: #830

OndraM · 2020-11-16T15:20:18Z

phpstan.neon

@@ -7,8 +7,6 @@ parameters:
    ignoreErrors:
        # To be fixed:
        - '#Call to an undefined method Facebook\\WebDriver\\WebDriver::getTouch\(\)#'
-        - '#Call to an undefined method Facebook\\WebDriver\\WebDriverElement::getCoordinates\(\)#'
-        - '#Call to an undefined method Facebook\\WebDriver\\WebDriverElement::equals\(\)#'


And now those needs to be put back 🙃 .

Thank you !
Happy you did point that out because I was searching why the error was occurring ^^

Co-Authored-By: Dries Vints <dries@vints.io>

driesvints · 2020-11-16T15:31:37Z

@ondrejmirtes @OndraM @williamdes thanks all! 🙂

OndraM · 2020-11-16T15:59:15Z

PHP 8 support is now merged to main branch. Nice job everyone!

~~I will get this released as 1.8.4 quite soon.~~

This will have to be 1.9.0, as we already have other new feature in main.

viezel · 2020-11-16T18:42:48Z

@ondrejmirtes @OndraM @williamdes awesome work. Looking forward to that 1.9.0 tag!
PHP8 here we go! 🚀

driesvints · 2020-11-19T13:59:50Z

Hey @OndraM. Is there anything blocking the 1.9.0 tag? We'd really like to patch Dusk soon since it's currently blocking a lot of other projects from shipping PHP 8 support. Let me know if there's anything I can do to help out.

OndraM · 2020-11-19T14:06:14Z

@driesvints Sorry for the delay, I'd like to finish one other PR (#556) to get it merged into the 1.9.0 release. Should be finished once CI builds are done, and the release will be just afterwards :)

driesvints · 2020-11-19T14:09:00Z

@OndraM no sorry needed. Awesome, looking forward to it :)

OndraM · 2020-11-19T15:24:00Z

📢 1.9.0 with PHP 8 support was just released.

driesvints · 2020-11-19T15:46:08Z

@OndraM amazing!

driesvints · 2020-12-04T11:33:59Z

scripts/apply-phpunit-patches.sh

+sed -i 's/function runBare(): void/function runBare()/g' tests/functional/WebDriverTestCase.php;
+
+# Return back to original dir
+cd - > /dev/null


@OndraM @williamdes would it be okay with you two if I used this script for algolia/algoliasearch-client-php#644 as well?

Yes you can copy it :)

driesvints mentioned this pull request Oct 30, 2020

Add PHP nightly build to Travis CI #826

Closed

williamdes reviewed Oct 30, 2020

View reviewed changes

phpunit.xml.dist Outdated Show resolved Hide resolved

williamdes reviewed Nov 2, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

driesvints marked this pull request as ready for review November 2, 2020 16:55

williamdes reviewed Nov 2, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

williamdes reviewed Nov 2, 2020

View reviewed changes

composer.json Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

williamdes reviewed Nov 2, 2020

View reviewed changes

.php_cs.dist Show resolved Hide resolved

williamdes requested review from OndraM and andrewnicols November 3, 2020 00:23

GrahamCampbell suggested changes Nov 3, 2020

View reviewed changes

.php_cs.dist Show resolved Hide resolved

owenvoke mentioned this pull request Nov 3, 2020

feat: update to support PHP 8 laravel-zero/framework#421

Merged

viezel mentioned this pull request Nov 3, 2020

PHP8 Support livewire/livewire#1933

Merged

3 tasks

driesvints mentioned this pull request Nov 6, 2020

[6.x] PHP 8 Support laravel/dusk#833

Merged

MauricioFauth approved these changes Nov 6, 2020

View reviewed changes

OndraM reviewed Nov 10, 2020

View reviewed changes

tests/unit/Interactions/Internal/WebDriverCoordinatesTest.php Show resolved Hide resolved

OndraM reviewed Nov 10, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

OndraM reviewed Nov 10, 2020

View reviewed changes

tests/functional/WebDriverTestCase.php Show resolved Hide resolved

williamdes requested a review from OndraM November 13, 2020 13:54

OndraM mentioned this pull request Nov 16, 2020

Workaround for broken PHPStan builds #830

Merged

OndraM reviewed Nov 16, 2020

View reviewed changes

williamdes and others added 2 commits November 16, 2020 16:22

Scripts and configuration changes for PHP 8

0440592

Co-Authored-By: Dries Vints <dries@vints.io>

PHP 8 and phpunit compatibility changes on source files tests/*

dbbde45

OndraM merged commit 6583dd2 into php-webdriver:main Nov 16, 2020

driesvints deleted the php8 branch November 16, 2020 16:20

driesvints mentioned this pull request Nov 19, 2020

Update to latest PHPUnit #500

Closed

driesvints commented Dec 4, 2020

View reviewed changes

JohnRDOrazio mentioned this pull request May 3, 2021

PHP 8 compatibility phpmyadmin/phpmyadmin#16429

Closed

PHP 8 Support #828

PHP 8 Support #828

Conversation

driesvints commented Oct 30, 2020 • edited

driesvints commented Oct 30, 2020

sminnee commented Nov 1, 2020

driesvints commented Nov 2, 2020

driesvints commented Nov 2, 2020

driesvints commented Nov 2, 2020

driesvints commented Nov 2, 2020

driesvints commented Nov 2, 2020

This comment has been minimized.

williamdes commented Nov 3, 2020

driesvints commented Nov 3, 2020

williamdes commented Nov 3, 2020

OndraM commented Nov 16, 2020

williamdes commented Nov 16, 2020 • edited

driesvints commented Nov 16, 2020

williamdes commented Nov 16, 2020

ondrejmirtes commented Nov 16, 2020

OndraM commented Nov 16, 2020

ondrejmirtes commented Nov 16, 2020

OndraM commented Nov 16, 2020

williamdes commented Nov 16, 2020 • edited

ondrejmirtes commented Nov 16, 2020

driesvints commented Nov 16, 2020

OndraM commented Nov 16, 2020 • edited

OndraM commented Nov 16, 2020

OndraM commented Nov 16, 2020

OndraM Nov 16, 2020

Choose a reason for hiding this comment

williamdes Nov 16, 2020

Choose a reason for hiding this comment

driesvints commented Nov 16, 2020

OndraM commented Nov 16, 2020 • edited

viezel commented Nov 16, 2020

driesvints commented Nov 19, 2020 • edited

OndraM commented Nov 19, 2020

driesvints commented Nov 19, 2020

OndraM commented Nov 19, 2020

driesvints commented Nov 19, 2020

driesvints Dec 4, 2020

Choose a reason for hiding this comment

williamdes Dec 4, 2020

Choose a reason for hiding this comment

OndraM Dec 4, 2020

Choose a reason for hiding this comment

driesvints commented Oct 30, 2020 •

edited

williamdes commented Nov 16, 2020 •

edited

williamdes commented Nov 16, 2020 •

edited

OndraM commented Nov 16, 2020 •

edited

OndraM commented Nov 16, 2020 •

edited

driesvints commented Nov 19, 2020 •

edited