Use static closures for minor performance improvement whenever instance access is not needed #729

westonruter · 2023-05-15T20:55:11Z

Summary

Fixes #728

Relevant technical choices

Added the SlevomatCodingStandard.Functions.StaticClosure sniff to the PHPCS ruleset.
Ran PHPCBF to add static where relevant.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

felixarntz

@westonruter LGTM, just one non-blocking question.

phpcs.xml.dist

felixarntz · 2023-05-15T20:59:02Z

@westonruter I've updated the label since this is more of an enhancement than a bug (nothing broken without this), plus changed the title slightly since that is used to auto-generate the eventual changelog for the release that this will be part of.

aristath

LGTM 👍
I had a small nitpick and added a comment, but definitely not a blocker

tests/admin/load-tests.php

spacedmonkey · 2023-05-16T10:58:20Z

composer.json

@@ -15,6 +15,7 @@
    "dealerdirect/phpcodesniffer-composer-installer": "^0.7",
    "phpcompatibility/php-compatibility": "^9.3",
    "phpunit/phpunit": "^4|^5|^6|^7|^8|^9",
+    "slevomat/coding-standard": "^8.0",


This coding standard requires php 7.2. Will would make this plugin break in 7.2- no.

https://github.com/slevomat/coding-standard/blob/f69e2524e8770efb9b3e5ac4a0ebc0d54eb446d7/composer.json#L19

It would not break the plugin, because the composer autoloader is only used in tests.

It would just break tests running on PHP 5.6. However, right now tests are not running on this version on CI, so... 🤷

Edit: looks like there is #399 for updating tests.

Please continue discussion at #730 (comment)

As I understand, there are three possible ways to about this:

Have a separate composer file for PHP 7.2+

In GHA workflow, just-in-time composer require this package before doing composer install before running PHPCS

In GHA workflow, just-in-time composer remove this package before doing composer install prior to running PHPUnit in PHP<7.2.

I think the third option is ideal, since all normal development environments would be running PHP 7.2+. It's only the exception case, specifically on GHA, where PHP 5.6 is needed. This would allow a developer to have all packages installed normally.

The first option doesn't seem ideal since there would be a separate composer file to maintain, and Dependabot wouldn't automatically be aware of it.

We may not want this sniff after all. See WordPress/wordpress-develop#4457 (comment)

Noting that we could still make a separate decision from core here, there are several plugins that have stricter requirements than core to enforce better coding practices. I don't feel strongly about this one, but I feel like if we can find a way that we can integrate it that remains compatible with PHP 5.6, I'd be happy to include it.

I am not against using this rule set, but php 5.6 compat is the key here.

Can the 5.6 compat be handled as part of #399? Without CI actually running anything in 5.6, it makes it a bit difficult to verify the compat is working, or at least it seems a bit premature.

felixarntz

Marking this as needing changes due to #730 (comment) which applies here too.

Co-authored-by: Ari Stathopoulos <[email protected]>

mukeshpanchal27

Thanks, @westonruter. I left non-blocking feedback other than looks good to me.

tests/server-timing/perflab-server-timing-tests.php

Co-authored-by: Mukesh Panchal <[email protected]>

westonruter · 2023-06-01T17:20:23Z

Marking this as needing changes due to #730 (comment) which applies here too.

@felixarntz Considering your #730 (comment) I think this can move forward.

felixarntz · 2023-06-13T16:23:55Z

@westonruter Can you refresh this PR please so we can hopefully merge it by tomorrow for the 2.4.0 release?

… add/static-closures * 'trunk' of https://github.com/WordPress/performance: Wrap link around the entire third step instruction. Implement an admin notice and admin pointer to prompt admins using the SQLite module to migrate to the standalone plugin. Enhance interoperability between the SQLite module and the standalone plugin. Make Server-Timing header output more robust Reuse __return_null Replace deprecated RegExp assertions Require phpstan/phpstan-deprecation-rules Fix covers tags and a couple typos Require phpstan/phpstan-phpunit Add missing default value for message param in assertImageNotHasSource phpdoc Add missing return value null for void Fix composer script name in lint-staged Add missing parameter in phpdocc for perflab_disable_object_cache_dropin filter Add phpstan/extension-installer to allow-plugins Add phpstan config to branches.paths in php-lint workflow Run composer update Add phpstan config Add PHPStan to php-lint workflow Add package scripts and lint-staged for phpstan Require phpstan packages

westonruter · 2023-06-13T16:36:40Z

@westonruter Can you refresh this PR please so we can hopefully merge it by tomorrow for the 2.4.0 release?

@felixarntz Done ✅

felixarntz

Thanks @westonruter!

westonruter added 2 commits May 15, 2023 13:50

Add SlevomatCodingStandard.Functions.StaticClosure sniff

0bab7d6

Run phpcbf

7418bf6

westonruter added [Type] Bug An existing feature is broken Infrastructure Issues for the overall performance plugin infrastructure labels May 15, 2023

westonruter added this to the 2.4.0 milestone May 15, 2023

westonruter requested review from adamsilverstein, felixarntz, mitogh, OllieJones and JustinyAhin as code owners May 15, 2023 20:55

felixarntz approved these changes May 15, 2023

View reviewed changes

phpcs.xml.dist Outdated Show resolved Hide resolved

felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Bug An existing feature is broken labels May 15, 2023

felixarntz changed the title ~~Fix: Static closures should be employed whenever $this is not used~~ Use static closures should whenever $this is not used May 15, 2023

Run composer update

2c65a34

Apply StaticClosure sniff to tests as well

f8b62ef

westonruter requested a review from manuelRod as a code owner May 15, 2023 20:59

felixarntz changed the title ~~Use static closures should whenever $this is not used~~ Use static closures whenever $this is not used May 15, 2023

aristath approved these changes May 16, 2023

View reviewed changes

tests/admin/load-tests.php Outdated Show resolved Hide resolved

spacedmonkey reviewed May 16, 2023

View reviewed changes

swissspidy mentioned this pull request May 16, 2023

Enhance code quality by adding PHPStan and fixing level 0 issues #730

Merged

3 tasks

felixarntz requested changes May 16, 2023

View reviewed changes

Reuse __return_null

81a77e9

Co-authored-by: Ari Stathopoulos <[email protected]>

westonruter requested a review from felixarntz May 16, 2023 16:21

mukeshpanchal27 reviewed May 18, 2023

View reviewed changes

tests/server-timing/perflab-server-timing-tests.php Outdated Show resolved Hide resolved

tests/server-timing/perflab-server-timing-tests.php Outdated Show resolved Hide resolved

Use __return_null in two additional cases

816040b

Co-authored-by: Mukesh Panchal <[email protected]>

aristath mentioned this pull request May 19, 2023

Use static closures when not using $this WordPress/gutenberg#50723

Merged

westonruter added 2 commits June 13, 2023 09:32

Add missing static keywords to new closures

6e8833b

felixarntz approved these changes Jun 13, 2023

View reviewed changes

felixarntz changed the title ~~Use static closures whenever $this is not used~~ Use static closures for minor performance improvement whenever instance access is not needed Jun 13, 2023

felixarntz merged commit 2d2f5f2 into trunk Jun 13, 2023

felixarntz deleted the add/static-closures branch June 13, 2023 16:56

mukeshpanchal27 mentioned this pull request Jan 8, 2024

Change logic for finding plugin slug in CLI WordPress/plugin-check#367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use static closures for minor performance improvement whenever instance access is not needed #729

Use static closures for minor performance improvement whenever instance access is not needed #729

westonruter commented May 15, 2023 •

edited

Loading

felixarntz left a comment

felixarntz commented May 15, 2023

aristath left a comment

spacedmonkey May 16, 2023

swissspidy May 16, 2023 •

edited

Loading

westonruter May 16, 2023 •

edited

Loading

This comment was marked as duplicate.

westonruter May 16, 2023

felixarntz May 16, 2023

spacedmonkey May 17, 2023

westonruter May 17, 2023

felixarntz left a comment

mukeshpanchal27 left a comment

westonruter commented Jun 1, 2023 •

edited

Loading

felixarntz commented Jun 13, 2023

westonruter commented Jun 13, 2023

felixarntz left a comment

Use static closures for minor performance improvement whenever instance access is not needed #729

Use static closures for minor performance improvement whenever instance access is not needed #729

Conversation

westonruter commented May 15, 2023 • edited Loading

Summary

Relevant technical choices

Checklist

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented May 15, 2023

aristath left a comment

Choose a reason for hiding this comment

spacedmonkey May 16, 2023

Choose a reason for hiding this comment

swissspidy May 16, 2023 • edited Loading

Choose a reason for hiding this comment

westonruter May 16, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

westonruter May 16, 2023

Choose a reason for hiding this comment

felixarntz May 16, 2023

Choose a reason for hiding this comment

spacedmonkey May 17, 2023

Choose a reason for hiding this comment

westonruter May 17, 2023

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

westonruter commented Jun 1, 2023 • edited Loading

felixarntz commented Jun 13, 2023

westonruter commented Jun 13, 2023

felixarntz left a comment

Choose a reason for hiding this comment

westonruter commented May 15, 2023 •

edited

Loading

swissspidy May 16, 2023 •

edited

Loading

westonruter May 16, 2023 •

edited

Loading

westonruter commented Jun 1, 2023 •

edited

Loading