Skip to content

Classes/NewReadonlyProperties: start using PHPCSUtils 1.1.0 getDeclared*() #1841

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 6, 2025

A while ago, someone reported a performance issue with the Generic.PHP.LowerCaseType sniff, which was due to the sniff examining all variables, instead of only examining OO properties.

Along the same lines, a similar performance issue was previously reported in #1477 and an initial fix was put in place via #1577.

PHPCSUtils 1.1.0 now introduces the new ObjectDeclarations::getDeclaredProperties and ObjectDeclarations::getDeclaredMethods() methods which can retrieve a list of all properties/methods declared in an OO structure.

These PHPCSUtils methods are highly optimized and will cache the results, which means that if multiple sniffs use these methods, the results will be nearly instantaneously returned on subsequent uses.

This commits starts using these methods in this sniff, which should further improve performance.

Note: the one downside of using the new methods, is that the names of the declared properties and methods in an OO construct have to be unique. As that's already a requirement in PHP anyway, I'm not concerned about the "real world" impact of this, though it does mean we need to be careful with this when creating test case files for sniffs which use these methods.

For that reason, there are some minor adjustments in the test case file for this sniff.


Some non-scientific performance impact estimation:

Using the test file which was shared with me - https://github.com/Automattic/HyperDB/blob/trunk/db.php - and running the below on PHP 8.4:

phpcs -ps db.php --standard=PHPCompatibility --report=source -vvv --sniffs=PHPCompatibility.Classes.NewReadonlyProperties

... yielded the following difference in runtime:

Baseline - Timing when run without this commit:

        *** START SNIFF PROCESSING REPORT ***
        PHPCompatibility\Sniffs\Classes\NewReadonlyProperties: 0.1542 secs
        *** END SNIFF PROCESSING REPORT ***

Timing when run with this commit:

        *** START SNIFF PROCESSING REPORT ***
        PHPCompatibility\Sniffs\Classes\NewReadonlyProperties: 0.002 secs
        *** END SNIFF PROCESSING REPORT ***

While it doesn't make that much difference on the total runtime with just one file, you can see the sniff runtime has gone down significantly - from 0.1542 secs to 0.002 secs, which should have a significant positive impact when scanning complete codebases.

…ed*()

A while ago, someone reported a performance issue with the `Generic.PHP.LowerCaseType` sniff, which was due to the sniff examining all variables, instead of only examining OO properties.

Along the same lines, a similar performance issue was previously reported in 1477 and an initial fix was put in place via 1577.

PHPCSUtils 1.1.0 now introduces the new `ObjectDeclarations::getDeclaredProperties` and `ObjectDeclarations::getDeclaredMethods()` methods which can retrieve a list of all properties/methods declared in an OO structure.

These PHPCSUtils methods are highly optimized and will cache the results, which means that if multiple sniffs use these methods, the results will be nearly instantaneously returned on subsequent uses.

This commits starts using these methods in this sniff, which should further improve performance.

Note: the one downside of using the new methods, is that the names of the declared properties and methods in an OO construct have to be unique.
As that's already a requirement in PHP anyway, I'm not concerned about the "real world" impact of this, though it does mean we need to be careful with this when creating test case files for sniffs which use these methods.

For that reason, there are some minor adjustments in the test case file for this sniff.

---

Some non-scientific performance impact estimation:

Using the test file which was shared with me - https://github.com/Automattic/HyperDB/blob/trunk/db.php - and running the below on PHP 8.4:
```
phpcs -ps db.php --standard=PHPCompatibility --report=source -vvv --sniffs=PHPCompatibility.Classes.NewReadonlyProperties
```

... yielded the following difference in runtime:

Baseline - Timing when run without this commit:
```
        *** START SNIFF PROCESSING REPORT ***
        PHPCompatibility\Sniffs\Classes\NewReadonlyProperties: 0.1542 secs
        *** END SNIFF PROCESSING REPORT ***
```

Timing when run with this commit:
```
        *** START SNIFF PROCESSING REPORT ***
        PHPCompatibility\Sniffs\Classes\NewReadonlyProperties: 0.002 secs
        *** END SNIFF PROCESSING REPORT ***
```

While it doesn't make that much difference on the total runtime with just one file, you can see the sniff runtime has gone down significantly - from 0.1542 secs to 0.002 secs, which should have a significant positive impact when scanning complete codebases.
@jrfnl jrfnl added this to the 10.0.0 milestone Aug 6, 2025
@jrfnl jrfnl added chores/QA PR: quick merge PR only contains relatively simple changes PR: ready for review labels Aug 6, 2025
@jrfnl jrfnl requested a review from wimg August 6, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chores/QA PR: quick merge PR only contains relatively simple changes PR: ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant