Skip to content

fix #112 - EPV doesn't work with PHP 8 #115

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.

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

Merged
merged 21 commits into from
Apr 26, 2025
Merged

Conversation

Skouat
Copy link
Contributor

@Skouat Skouat commented Sep 22, 2024

From Titania, when we submit a revision and Titania runs under PHP8, EPV fails with the following message:

[phpBB Debug] PHP Warning
: in file
[ROOT]/ext/phpbb/titania/vendor/nikic/php-parser/lib/PhpParser/Lexer.php
on line
258
:
Undefined array key 265

[phpBB Debug] PHP Warning
: in file
[ROOT]/ext/phpbb/titania/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php
on line
190
:
Undefined array key ""

[phpBB Debug] PHP Warning
: in file
[ROOT]/ext/phpbb/titania/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php
on line
359
:
Undefined array key ""

Upgrading nikic/php-parser to version 4 fix this issue, and also the issue #112

Copy link

private-packagist bot commented Sep 22, 2024

composer.lock

Click to show 62 changes in this composer.lock file

Package changes

Package Operation From To About
composer/class-map-generator add - 1.6.1 view code - License: MIT License
composer/metadata-minifier add - 1.0.0 view code - License: MIT License
composer/pcre add - 2.3.2 view code - License: MIT License
marc-mabe/php-enum add - v4.7.1 view code - License: BSD 3-Clause "New" or "Revised" License
react/promise add - v3.2.0 view code - License: MIT License
seld/signal-handler add - 2.0.2 view code - License: MIT License
symfony/deprecation-contracts add - v2.5.4 view code - License: MIT License
symfony/polyfill-intl-grapheme add - v1.31.0 view code - License: MIT License
symfony/polyfill-intl-normalizer add - v1.31.0 view code - License: MIT License
symfony/polyfill-php81 add - v1.31.0 view code - License: MIT License
symfony/string add - v5.4.47 view code - License: MIT License
composer/ca-bundle upgrade 1.3.1 1.5.6 diff
composer/composer upgrade 1.10.26 ⚠️ 2.8.8 ✅ diff
composer/semver upgrade 1.7.2 3.4.3 diff
composer/spdx-licenses upgrade 1.5.6 1.5.8 diff
composer/xdebug-handler upgrade 1.4.6 3.0.5 diff
gitonomy/gitlib upgrade v0.1.8 v1.3.8 diff
justinrainbow/json-schema upgrade 5.2.12 6.4.1 diff
nikic/php-parser upgrade v3.1.5 v4.19.4 diff
psr/container upgrade 1.0.0 1.1.1 diff
seld/jsonlint upgrade 1.9.0 1.11.0 diff
seld/phar-utils upgrade 1.2.0 1.2.1 diff
sensiolabs/ansi-to-html upgrade v1.2.1 v1.3.0 diff
symfony/console upgrade v4.4.16 v5.4.47 diff
symfony/filesystem upgrade v5.4.7 v5.4.45 diff
symfony/finder upgrade v5.1.8 v5.4.45 diff
symfony/polyfill-ctype upgrade v1.25.0 v1.31.0 diff
symfony/polyfill-mbstring upgrade v1.20.0 v1.31.0 diff
symfony/polyfill-php73 upgrade v1.20.0 v1.31.0 diff
symfony/polyfill-php80 upgrade v1.25.0 v1.31.0 diff
symfony/process upgrade v3.4.46 ⚠️ v5.4.47 ✅ diff
symfony/service-contracts upgrade v2.2.0 v2.5.4 diff
symfony/yaml upgrade v4.4.16 v5.4.45 diff

Dev Package changes

Package Operation From To About
phar-io/manifest add - 2.0.4 view code - License: BSD 3-Clause "New" or "Revised" License
phar-io/version add - 3.2.1 view code - License: BSD 3-Clause "New" or "Revised" License
sebastian/object-reflector add - 1.1.3 view code - License: BSD 3-Clause "New" or "Revised" License
sebastian/type add - 1.1.5 view code - License: BSD 3-Clause "New" or "Revised" License
theseer/tokenizer add - 1.2.3 view code - License: BSD 3-Clause "New" or "Revised" License
doctrine/instantiator upgrade 1.4.0 1.5.0 diff
myclabs/deep-copy upgrade 1.10.2 1.13.0 diff
phpunit/php-code-coverage upgrade 4.0.8 7.0.17 diff
phpunit/php-file-iterator upgrade 1.4.5 2.0.6 diff
phpunit/php-timer upgrade 1.0.9 2.1.4 diff
phpunit/php-token-stream upgrade 2.0.2 3.1.3 diff
phpunit/phpunit upgrade 5.7.27 8.5.41 diff
sebastian/code-unit-reverse-lookup upgrade 1.0.1 1.0.3 diff
sebastian/comparator upgrade 1.2.4 3.0.5 diff
sebastian/diff upgrade 1.4.3 3.0.6 diff
sebastian/environment upgrade 2.0.0 4.2.5 diff
sebastian/exporter upgrade 2.0.0 3.1.6 diff
sebastian/global-state upgrade 1.1.1 3.0.5 diff
sebastian/object-enumerator upgrade 2.0.1 3.0.5 diff
sebastian/recursion-context upgrade 2.0.0 3.0.2 diff
sebastian/resource-operations upgrade 1.0.0 2.0.3 diff
phpdocumentor/reflection-common remove 2.2.0 - -
phpdocumentor/reflection-docblock remove 5.2.2 - -
phpdocumentor/type-resolver remove 1.4.0 - -
phpspec/prophecy remove v1.10.3 - -
phpunit/phpunit-mock-objects remove 3.4.4 - -
webmozart/assert remove 1.9.1 - -

Important Metadata Changes

Package Version Metadata From To
justinrainbow/json-schema 6.4.1 dist url https://api.github.com/repos/justinrainbow/json-schema/zipball/ad87d5a5ca981228e0e205c2bc7dfb8e24559b60 https://api.github.com/repos/jsonrainbow/json-schema/zipball/35d262c94959571e8736db1e5c9bc36ab94ae900
justinrainbow/json-schema 6.4.1 source url https://github.com/justinrainbow/json-schema.git https://github.com/jsonrainbow/json-schema.git

Settings · Docs · Powered by Private Packagist

@paul999
Copy link
Member

paul999 commented Sep 23, 2024

@Skouat But does it still work correctly? Looking at the tests a bunch of them fail now, which didn't happen in the past. It would also be nice if we run it at least on 1 php8 version on GitHub.

We also will need to have it both php7 and php8 compatible.

@Skouat
Copy link
Contributor Author

Skouat commented Sep 24, 2024

Hi @paul999

Looking at the tests a bunch of them fail now, which didn't happen in the past.

Please, take on look on the PR #111. It fails. So maybe the issue is on an higher level and not due to my PR.

also be nice if we run it at least on 1 php8 version on GitHub.

We also will need to have it both php7 and php8 compatible.

Sure. I can check this.

@Skouat Skouat changed the title fix #112 - EPV doesn't work with PHP 8 Draft -fix #112 - EPV doesn't work with PHP 8 Sep 25, 2024
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Skouat Skouat changed the title Draft -fix #112 - EPV doesn't work with PHP 8 WIP -fix #112 - EPV doesn't work with PHP 8 Sep 27, 2024
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Skouat Skouat force-pushed the fix-112 branch 2 times, most recently from 22cab79 to 63921cc Compare October 4, 2024 13:17
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

1 similar comment
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Skouat Skouat changed the title WIP -fix #112 - EPV doesn't work with PHP 8 fix #112 - EPV doesn't work with PHP 8 Oct 4, 2024
@Skouat
Copy link
Contributor Author

Skouat commented Oct 4, 2024

@paul999 Normally, now it works with PHP 7 and PHP 8

@iMattPro
Copy link
Member

iMattPro commented Oct 4, 2024

I think the test runner needs to be updated to run on ubuntu-22.04

@Skouat
Copy link
Contributor Author

Skouat commented Oct 5, 2024

Your master branch use ubuntu-latest.
Can you confirm I can force ubuntu-22.04?

@rxu
Copy link

rxu commented Apr 23, 2025

PHP-Parser 4.x parses PHP 5.2 to PHP 8.0 (https://github.com/nikic/PHP-Parser/blob/4.x/doc/0_Introduction.markdown#introduction), so can fail in cases of using some 8.1+ specific syntax.

PHP-Parser 5.x parses PHP 7 and PHP 8 (https://github.com/nikic/PHP-Parser/blob/master/doc/0_Introduction.markdown#what-can-it-parse), so I'd suggest trying to use it instead. That will require more code changes in EPV as things got changed in the Parser code, f.e. (new ParserFactory)->create(ParserFactory::PREFER_PHP7); won't work as neither create() method nor PREFER_PHP7 exist anymore.

@iMattPro
Copy link
Member

That should be a separate PR. This one is just about getting EPV to just work. Plus I'm not sure who's writing extensions using PHP 8.1 or newer syntax yet, that's less of a priority.

@rxu
Copy link

rxu commented Apr 23, 2025

Well, 4.0 is PHP 8.1 minimum requirement, and all extensions will be as well :)

@iMattPro
Copy link
Member

Well, 4.0 is PHP 8.1 minimum requirement, and all extensions will be as well :)

Sure but that does not mean they are rewritten using 8.1 introduced constructs.

@iMattPro
Copy link
Member

PHP-Parser 4.x parses PHP 5.2 to PHP 8.0 (https://github.com/nikic/PHP-Parser/blob/4.x/doc/0_Introduction.markdown#introduction), so can fail in cases of using some 8.1+ specific syntax.

PHP-Parser 5.x parses PHP 7 and PHP 8 (https://github.com/nikic/PHP-Parser/blob/master/doc/0_Introduction.markdown#what-can-it-parse), so I'd suggest trying to use it instead. That will require more code changes in EPV as things got changed in the Parser code, f.e. (new ParserFactory)->create(ParserFactory::PREFER_PHP7); won't work as neither create() method nor PREFER_PHP7 exist anymore.

So (new ParserFactory)->create(ParserFactory::PREFER_PHP7); becomes (new ParserFactory())->createForHostVersion();?

@rxu
Copy link

rxu commented Apr 23, 2025

Looks like that, but there're more changes here https://github.com/nikic/PHP-Parser/blob/master/UPGRADE-5.0.md (I actually didn't dig deeply into EPV code, so no idea how many of these are relevant). Also 5.x requires PHP 7.4+ so respective version constraints in composer.json should be piped (like "php": ">=7.2 <7.4 || >=7.4" and "nikic/php-parser": "~4.0 || ~5.0", or so).

@rxu
Copy link

rxu commented Apr 23, 2025

Probably you're right and this is something for another PR.

@iMattPro
Copy link
Member

iMattPro commented Apr 23, 2025

Well now that sounds like updating that dependency could break BC with the current phpBB PHP specs, so not just something for another PR, but for another future version of EPV that drops support for phpBB 3.3.x and PHP 7.1, 7.2 and 7.3.

@rxu
Copy link

rxu commented Apr 23, 2025

But shouldn't version constrains work automatically? Like, if you test on PHP 7.2/7.3, it installs PHP-Parser 4.x, and on PHP 7.4+ it installs PHP-Parser 5.x.

@iMattPro
Copy link
Member

I think the lock file needs updating again?

@Skouat
Copy link
Contributor Author

Skouat commented Apr 23, 2025

Also 5.x requires PHP 7.4+ so respective version constraints in composer.json should be piped (like "php": ">=7.2 <7.4 || >=7.4" and "nikic/php-parser": "~4.0 || ~5.0", or so).

Because of the --ignore-platform-reqs argument used when we run EPV in GA, php-parser 5 is installed on all PHP version even those under 7.4.

@iMattPro iMattPro requested a review from paul999 April 23, 2025 18:41
@iMattPro
Copy link
Member

Everything looks good to me, except that to run this using PHP 8.1 with phpBB 3.3.x requires the --with-all-dependencies arg. Other builds, like the standard PHP 7 w phpBB 3.3.x and PHP 8 with phpBB 4 all work good w/o that extra argument. Maybe that's just how it has to be, I never understand composer especially when it starts failing to resolve dependencies.

@Skouat
Copy link
Contributor Author

Skouat commented Apr 23, 2025

I think the lock file needs updating again?

With PHP 7.2, only the content-hash is updated when I run composer update.

@iMattPro iMattPro merged commit f6c828c into phpbb:master Apr 26, 2025
7 of 8 checks passed
@Skouat Skouat deleted the fix-112 branch April 26, 2025 16:04
@LukeWCS
Copy link
Contributor

LukeWCS commented Apr 29, 2025

Hello

First of all, thanks to everyone who made it possible to run EPV under PHP 8. It's also very important to me that PHP code >=7.2 is now supported. :-) In my specific case: 7.4 code.

What is the current status of EPV? Can we already use the current development version, or should we wait for 0.0.13? Tests have been very positive, see issue 116, and I would now of course like to use the latest version of EPV with phpBB Ext Check (14 Ext developers).

@Skouat
Copy link
Contributor Author

Skouat commented Apr 29, 2025

Hello,
Yes the php-parser is now compatible with PHP8.

Just keep in mind that for PHP8 and newer, you must add the --with-all-dependencies argument in your tests.yml, when you run the composer update.

@iMattPro
Copy link
Member

Also to clarify further, --with-all-dependencies is only needed using PHP 8 with phpBB 3.3.x. When EPV is used with phpBB's master dev branch, it shouldn't be needed.

@LukeWCS
Copy link
Contributor

LukeWCS commented Apr 29, 2025

Hello Skouat and Matt

Thanks for your answers. You've really confused me, and it took me a few minutes to understand your answers. At least, I hope so. ^^

I think there's a misunderstanding here. I use EPV as a standalone tool within my own test suite, phpBB Ext Check. It has nothing to do with GitHub Actions or with phpBB directly, and I don't have a tests.yml file there either. If you follow the link I included in my previous post, you can see two screenshots of the test suite where EPV is used. An Ext developer simply uploads a ZIP file with their extension and receives a report, just like the one shown in the screenshots.

This test suite was developed because many Ext developers are having trouble with GitHub. This gives this group the opportunity to receive a report from the official phpBB.com tools "Extension Pre Validator" and "phpBB PHP Strict Standard Extensions," which they wouldn't otherwise receive. Plus more reports from additional tools.

@LukeWCS
Copy link
Contributor

LukeWCS commented May 1, 2025

@LukeWCS
Copy link
Contributor

LukeWCS commented May 7, 2025

Hi @rxu

Regarding the following information:

PHP-Parser 4.x parses PHP 5.2 to PHP 8.0 (https://github.com/nikic/PHP-Parser/blob/4.x/doc/0_Introduction.markdown#introduction), so it can fail in cases of using some 8.1+ specific syntax.

The root of https://github.com/nikic/PHP-Parser/ reads:

Documentation for version 4.x (supported; for running on PHP >= 7.0; for parsing PHP 5.2 to PHP 8.3).

Which of the two statements is true regarding EPV? In our test of phpBB Ext Check 1.9.0, 30 extensions were successfully tested with the latest EPV. Of these, 7 extensions had PHP 7.4 code and 2 extensions had PHP 8.0 code. Therefore, it would be interesting to know where the "upper limit" of the current EPV lies.

@rxu
Copy link

rxu commented May 7, 2025

Which of the two statements is true regarding EPV?

I don't know, EPV uses PHP-Parser but documentation for the latter is authored by PHP-Parser author. But it turns out this documentation contradicts to itself. Then asking in https://github.com/nikic/PHP-Parser/issues would be a logical step probably.
Those statements might be just outdated and require updates.

@LukeWCS
Copy link
Contributor

LukeWCS commented May 7, 2025

Okay, then I'll ask there and post the answer here so that this information is complete.

@LukeWCS
Copy link
Contributor

LukeWCS commented May 7, 2025

This is correct:

PHP 5.2 to PHP 8.3

nikic/PHP-Parser#1083 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants