Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support PHP 8 #4289

Merged
merged 10 commits into from
Apr 9, 2021
Merged

Support PHP 8 #4289

merged 10 commits into from
Apr 9, 2021

Conversation

Al2Klimov
Copy link
Member

fixes #4287

@Al2Klimov
Copy link
Member Author

This one made the setup wizard working for me.

@Al2Klimov Al2Klimov changed the title /setup/: ensure not to pass an empty array to max() Support PHP 8 Dec 2, 2020
@Al2Klimov
Copy link
Member Author

Note: PHP 8 doesn’t like our Mockery version (parent keyword w/o a parent class), neither us in combination with the latest Mockery version (base/derived method incompatibility). I consider the latter unfounded and a bug in PHP itself.

@Al2Klimov Al2Klimov force-pushed the bugfix/php8-4287 branch 3 times, most recently from 3ef5f53 to d1ba493 Compare December 4, 2020 11:22
@Al2Klimov
Copy link
Member Author

Clicked through the framework and the monitoring mod and got only the errors I've fixed here (and #4291).

@nilmerg IMAO the only TBD are the tests, but I'd like to hear which way to solve this do you prefer first.

@sebastic
Copy link
Contributor

sebastic commented Dec 18, 2020 •
edited
Loading

The changes from this PR along with 9748994 & 3ef5f53 have been included as patches in the icingaweb2 (2.8.2-2~exp1) package in Debian in preparation of the transition to php8.0.

While the web interface mostly works, the dashboard does not.

The browser console shows:

Uncaught ReferenceError: Icinga is not defined

This appears to be caused by the JShrink, from the apache error log:

PHP Fatal error:  Maximum execution time of 30 seconds exceeded in
/usr/share/icingaweb2/library/vendor/JShrink/Minifier.php on line 292

This seems to be known issue: tedious/JShrink#96

Updating Minifier.php to the unreleased version in master (tedious/JShrink@ab780a6) resolves this issue.

Ondřej Surý, the maintainer of PHP in Debian, also mentioned that newer versions of Mockery and PHPUnit are required for PHP 8.

@Al2Klimov
Copy link
Member Author

Updating Minifier.php to the unreleased version in master (tedious/JShrink@ab780a6) resolves this issue.

Hello @sebastic and thank you for figuring that out!

Please could you open a PR?

Ondřej Surý, the maintainer of PHP in Debian, also mentioned that newer versions of Mockery and PHPUnit are required for PHP 8.

Yes, how to make the test also working is still TBD (#4289 (comment)).

Best,
AK

@sebastic sebastic mentioned this pull request Dec 21, 2020
Merged
@sebastic
Copy link
Contributor

Updating Minifier.php to the unreleased version in master (tedious/JShrink@ab780a6) resolves this issue.

Hello @sebastic and thank you for figuring that out!

Please could you open a PR?

Done: #4298

@sklaes
Copy link

sklaes commented Feb 14, 2021

JShrink 1.4 with PHP 8.0 support has been released:

https://github.com/tedious/JShrink/releases/tag/v1.4.0

@sebastic sebastic mentioned this pull request Mar 30, 2021
Closed
@nilmerg nilmerg force-pushed the bugfix/php8-4287 branch from d1ba493 to fe5249f Compare April 8, 2021 08:39
@nilmerg nilmerg added this to the 2.9.0 milestone Apr 8, 2021
@nilmerg nilmerg self-assigned this Apr 8, 2021
@nilmerg nilmerg force-pushed the bugfix/php8-4287 branch 11 times, most recently from 90f96d7 to 30a0f9b Compare April 9, 2021 09:49
@nilmerg nilmerg force-pushed the bugfix/php8-4287 branch from 30a0f9b to 9d94c4b Compare April 9, 2021 09:53
Al2Klimov and others added 10 commits April 9, 2021 12:37
@Al2Klimov @nilmerg
/setup/: ensure not to pass an empty array to max()
2c5f141 
... as PHP 8 doesn't tolerate such.

refs #4287
@Al2Klimov @nilmerg
GitHub actions: test also with PHP 8.0
2990ca0 
refs #4287
@Al2Klimov @nilmerg
*downtimestarthistoryQuery#joinBaseTables(): don't call func_get_arg(…
6d5024c 
…) with an invalid index

... as PHP 8 doesn't tolerate that.

refs #4287
@Al2Klimov @nilmerg
ApplicationBootstrap#setupErrorHandling(): migrate error handler to P…
eff10ec 
…HP 8

... as recommended here:
https://www.php.net/manual/de/migration80.incompatible.php

refs #4287
@nilmerg
php: Don't test on PHP 5.6 through 7.1
19a092c
@nilmerg
tests: Add :void return type declarations
cb87190
@nilmerg
BaseTestCase: Inherit from Mockery\Adapter\Phpunit\MockeryTestCase
bc1c967
@nilmerg
tests: Don't make use of deprecated phpunit features
f27e872
@nilmerg
tests: Don't use deprecated assertion assertRegExp()
e671324
@nilmerg
php: Don't lint tests on PHP 5.6
b3eeb5a
@nilmerg nilmerg force-pushed the bugfix/php8-4287 branch from 9d94c4b to b3eeb5a Compare April 9, 2021 10:38
@nilmerg
Copy link
Member

nilmerg commented Apr 9, 2021

Got the checks all working. Also, I've removed the @ operator from all adjusted calls, it's redundant now. Though, I really think we're gonna haunted by the change that the silence operator (@) doesn't silence fatal errors anymore and some warnings are now fatal errors in PHP 8. There are way too much usages in all our projects and without a documented list of all changes (max and func_get_arg changes are not documented explicitly) we cannot ensure all is still working.

Gonna test this myself on a PHP 8 installation now. If all works, I'll merge this today. (Also #4298, once I had a look at it)

@nilmerg nilmerg marked this pull request as ready for review April 9, 2021 12:41
@nilmerg nilmerg merged commit 4bc5350 into master Apr 9, 2021
@nilmerg nilmerg deleted the bugfix/php8-4287 branch April 9, 2021 12:42
@nilmerg nilmerg added the enhancement New feature or improvement label Apr 9, 2021
nilmerg added a commit that referenced this pull request Apr 14, 2021
@nilmerg
zend: PHP 8 compatiblity changes
0659153 
`ReflectionClass::newInstanceArgs()` seems to respect string keys
since PHP 8.

refs #4289
flourish86 pushed a commit that referenced this pull request Jun 9, 2021
@nilmerg @flourish86
zend: PHP 8 compatiblity changes
f19cc67 
`ReflectionClass::newInstanceArgs()` seems to respect string keys
since PHP 8.

refs #4289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MEschenbacher MEschenbacher MEschenbacher left review comments

@Thomas-Gelf Thomas-Gelf Thomas-Gelf left review comments

Assignees

@nilmerg nilmerg

Labels
enhancement New feature or improvement
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

Icingaweb2 isn’t compatible with php8.0
6 participants
@Al2Klimov @sebastic @sklaes @nilmerg @Thomas-Gelf @MEschenbacher