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.

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

PhpParser 5 & PhpUnit 11 #1404

Merged
merged 2 commits into from
Mar 12, 2024
Merged

PhpParser 5 & PhpUnit 11 #1404

merged 2 commits into from
Mar 12, 2024

Conversation

kukulich
Copy link
Collaborator

  1. There are some little BC breaks, see changes eg. inClassForSourceStubberExpected.php

/** Comment */
define('CONSTANT_NAME', true);

Node\Expr\FuncCall is now wrapped in Node\Stmt\Expression and the comment is part of the wrapper. I've made a little hack and I copy the comment to the FuncCall node. See eg. FindReflectionsInTree

@kukulich kukulich force-pushed the parser5 branch 2 times, most recently from d1e4709 to 358e6d3 Compare March 11, 2024 22:55
phpunit.xml.dist Outdated Show resolved Hide resolved
src/BetterReflection.php Show resolved Hide resolved
@Ocramius
Copy link
Member

Note: didn't read through the whole patch yet 😁

@kukulich kukulich force-pushed the parser5 branch 2 times, most recently from dc61def to 8f94dee Compare March 12, 2024 07:41
@kukulich kukulich marked this pull request as ready for review March 12, 2024 07:59
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kukulich I don't see any explicit BC breaks, or am I missing something?

Comment on lines 323 to 325
if ($nodeDocComment !== null) {
$functionCall->setDocComment($nodeDocComment);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suspicious of this change: why is the AutoloadSourceLocator making decisions about changing the AST?

I'm not questioning the fact that this fixes something, but rather the fact that this call is specifically part of a source locator 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kukulich this is the last concern in this patch: what's going on with us manipulating nodes during *Locator activities?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks I've copied too much yesterday. The "copy" hack is not neccessary in AutoloadSourceLocator. Removed now.

@Ocramius
Copy link
Member

Did a local test of performance improvements:

betterreflection on  6.27.x [$?] via 🐘 v8.2.16 via ❄️  impure (nix-shell) 
❯ ./tools/vendor/bin/phpbench run --warmup=3 --revs=10 --iterations=20 --retry-threshold=4 --tag=6.27.x
betterreflection on  HEAD (8f94dee) [$?] via 🐘 v8.2.16 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench report --ref=6.27.x --report=aggregate
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                      | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+
| PhpUnitTestCaseBench | benchReflectClass            |     | 10   | 20  | 17.154mb | 2.300μs | ±1.90% |
| PhpUnitTestCaseBench | benchReflectMethodParameters |     | 10   | 20  | 17.154mb | 0.900μs | ±0.00% |
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+
betterreflection on  HEAD (8f94dee) [$?] via 🐘 v8.2.16 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench run --warmup=3 --revs=10 --iterations=20 --retry-threshold=4 --tag=1404
betterreflection on  HEAD (8f94dee) [$?] via 🐘 v8.2.16 via ❄️  impure (nix-shell) took 8s 
❯ ./vendor/bin/phpbench report --ref=1404 --report=aggregate
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                      | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+
| PhpUnitTestCaseBench | benchReflectClass            |     | 10   | 20  | 11.882mb | 2.300μs | ±2.21% |
| PhpUnitTestCaseBench | benchReflectMethodParameters |     | 10   | 20  | 11.882mb | 0.900μs | ±0.00% |
+----------------------+------------------------------+-----+------+-----+----------+---------+--------+

Time-wise, we're running the same stuff, but memory usage dropped sharply, probably due to PHP-Parser relying less on array structures internally 💪

@Ocramius Ocramius added enhancement dependencies Pull requests that update a dependency file labels Mar 12, 2024
@Ocramius Ocramius added this to the 6.27.0 milestone Mar 12, 2024
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most excellent, thanks @kukulich!

@Ocramius Ocramius merged commit c713190 into Roave:6.27.x Mar 12, 2024
23 checks passed
@Ocramius
Copy link
Member

This is going in a minor release :)

People should pin their own nikic/php-parser, if they rely on it directly ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants