Use static closures whenever `$this` is not used to avoid memory leaks #4457

westonruter · 2023-05-15T21:37:10Z

Add static keyword to eligible closures via composer run-script format -- --sniffs=SlevomatCodingStandard.Functions.StaticClosure
Undo changes to Gutenberg-upstream files.

Trac ticket: https://core.trac.wordpress.org/ticket/58323

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…ctions.StaticClosure

westonruter · 2023-05-15T21:40:17Z

composer.json

 	"require-dev": {
 		"dealerdirect/phpcodesniffer-composer-installer": "^0.7.0",
-		"squizlabs/php_codesniffer": "3.6.0",
+		"squizlabs/php_codesniffer": "^3.7.1",


I'm curious why PHPCS was pinned back on 3.6.0. The current version is 3.7.2. @jrfnl I'm sure you know!

I'd definitely recommend an update, but until WPCS 3.0.0 is released the point is moot.

PHPCS 3.6.1 - 3.7.2 do contain some useful bugfixes, but the majority of updates are related to supporting new PHP syntaxes, which WP Core cannot use anyway since the minimum PHP version is 5.6, so for WP the benefit of updating would be minimal.

Also see the changelogs: https://github.com/squizlabs/PHP_CodeSniffer/releases

PHPCS does need to be pinned to a specific version as there is no committed composer.lock file and having PHPCS be a moving target which can fail builds at random whenever a new version is tagged is not desirable, so using ^ in the version constraint is out of the question.

Aside from that, I'd recommend an update to PHPCS to be done in a separate PR as it may throw new warnings/errors/allow for ignore annotations to be removed, which should not be mixed in with this PR.

westonruter · 2023-05-15T21:49:39Z

composer.json

 		"phpcompatibility/phpcompatibility-wp": "~2.1.3",
-		"yoast/phpunit-polyfills": "^1.0.1"
+		"yoast/phpunit-polyfills": "^1.0.1",
+		"slevomat/coding-standard": "^8.0"


Ah, this sniff requires PHP 7.2+. Per GHA log:

Error: Your requirements could not be resolved to an installable set of packages. Problem 1 - slevomat/coding-standard[8.0.0, ..., 8.12.0] require php ^7.2 || ^8.0 -> your php version (5.6.40) does not satisfy that requirement. - Root composer.json requires slevomat/coding-standard ^8.0 -> satisfiable by slevomat/coding-standard[8.0.0, ..., 8.12.0].

... which is why it is not a dependency of WPCS (aside from a wide variety of bugs and other PHPCS non-standard behaviour, including adding additional external dependencies, which would need to be fixed first anyway).

jrfnl

See my inline comments.

While it is fine to use external sniffs to improve the WP codebase, adding those to the ruleset is a big no-no.

jrfnl · 2023-05-15T23:55:29Z

composer.json

 		"phpcompatibility/phpcompatibility-wp": "~2.1.3",
-		"yoast/phpunit-polyfills": "^1.0.1"
+		"yoast/phpunit-polyfills": "^1.0.1",
+		"slevomat/coding-standard": "^8.0"


... which is why it is not a dependency of WPCS (aside from a wide variety of bugs and other PHPCS non-standard behaviour, including adding additional external dependencies, which would need to be fixed first anyway).

jrfnl · 2023-05-16T00:00:00Z

composer.json

 	"require-dev": {
 		"dealerdirect/phpcodesniffer-composer-installer": "^0.7.0",
-		"squizlabs/php_codesniffer": "3.6.0",
+		"squizlabs/php_codesniffer": "^3.7.1",


I'd definitely recommend an update, but until WPCS 3.0.0 is released the point is moot.

PHPCS 3.6.1 - 3.7.2 do contain some useful bugfixes, but the majority of updates are related to supporting new PHP syntaxes, which WP Core cannot use anyway since the minimum PHP version is 5.6, so for WP the benefit of updating would be minimal.

Also see the changelogs: https://github.com/squizlabs/PHP_CodeSniffer/releases

PHPCS does need to be pinned to a specific version as there is no committed composer.lock file and having PHPCS be a moving target which can fail builds at random whenever a new version is tagged is not desirable, so using ^ in the version constraint is out of the question.

Aside from that, I'd recommend an update to PHPCS to be done in a separate PR as it may throw new warnings/errors/allow for ignore annotations to be removed, which should not be mixed in with this PR.

jrfnl · 2023-05-16T00:07:00Z

phpcs.xml.dist

 		<exclude-pattern>/src/wp-content/themes/twentyfourteen/inc/featured-content\.php</exclude-pattern>
 	</rule>
+
+	<rule ref="SlevomatCodingStandard.Functions.StaticClosure" />


This needs to be removed.

It is not a rule in the official coding standards, so, while I agree, it is a good "best practice", we cannot demand for it to be enforced.

spacedmonkey · 2023-05-16T10:15:06Z

src/wp-includes/global-styles-and-settings.php


 		// The likes of block element styles from theme.json do not have  $metadata['name'] set.
 		if ( ! isset( $metadata['name'] ) && ! empty( $metadata['path'] ) ) {
 			$result = array_values(


This is a file controlled by gutenberg. This change will need to made in the gutenberg repo and ported across.

Note to self, this is the Gutenberg file: https://github.com/WordPress/gutenberg/blob/trunk/lib/global-styles-and-settings.php

spacedmonkey · 2023-05-16T10:15:29Z

src/wp-includes/class-wp-theme-json.php

 		 * Check for allowed pseudo classes (e.g. ":hover") from the $selector ("a:hover").
 		 * This also resets the array keys.
 		 */
 		$pseudo_matches = array_values(


This is a file controlled by gutenberg. This change will need to made in the gutenberg repo and ported across.

I suppose that should be indicated in the file DocBlock.

Note to self, Gutenberg file located at https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-data-gutenberg.php

spacedmonkey

+1 to remove all the coding standard changes. Let's just keep this today code changes. Also, please remove all gutenberg related changes.

This reverts commit 92b1344.

This reverts commit 773c264.

This reverts commit 2a93ce6.

This reverts commit dbdb872.

westonruter · 2023-05-16T21:41:03Z

While it is fine to use external sniffs to improve the WP codebase, adding those to the ruleset is a big no-no.

@jrfnl Just curious why adding these to the ruleset is not OK. I thought the ruleset here was exclusively for wordpress-develop, as it's not located in the WPCS repo.

…into trac-58323 * 'trunk' of https://github.com/WordPress/wordpress-develop: Media: Increase default for `wp_omit_loading_attr_threshold` to 3. Media: Prevent CSRF setting attachment thumbnails. Embeds: Add protocol validation for WordPress Embed code. Editor: Ensure block comments are of a valid form. Editor: Remove shortcode support from block templates. I18N: Introduce sanitization function for locale. Taxonomy: Do not prime term meta in `wp_get_object_terms`. Docs: Clarify `@param` types on `get_sample_permalink_html` filter.

jrfnl · 2023-05-16T22:08:13Z

While it is fine to use external sniffs to improve the WP codebase, adding those to the ruleset is a big no-no.

@jrfnl Just curious why adding these to the ruleset is not OK. I thought the ruleset here was exclusively for wordpress-develop, as it's not located in the WPCS repo.

True, but if we want to enforce something for WP Core, I keep being told that it has to go through a) a Make post, b) a change to the formal coding standards docs, c) only after that the standard can be changed. I don't see why it would be any different for you. 🤷🏻‍♀️

Aside from that, adding the Slevomat standard would be prohibitive for running a composer install on PHP < 7.2, which is, of course, a no-no.

jrfnl

Current state of this PR is all okay.

While looking through the PR, I do see some other things which should probably be addressed (long closures which should be named functions, unnecessary use of references and closure use), but that's definitely outside the scope of this PR and for another time ;-).

jrfnl · 2023-05-16T22:11:16Z

src/wp-includes/blocks.php

 			 * @return string Returns the block content.
 			 */
-			$settings['render_callback'] = function( $attributes, $content, $block ) use ( $template_path ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
+			$settings['render_callback'] = static function( $attributes, $content, $block ) use ( $template_path ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable


Outside of the scope of this PR, but we may as well remove the // phpcs:ignore comment as the VariableAnalysis standard is not being used by Core, nor WPCS.

Isn't this also a Gutenberg file though ?

Isn't this also a Gutenberg file though ?

Based on the filename it would seem so, but it seems to fundamentally be a different file from https://github.com/WordPress/gutenberg/blob/trunk/lib/blocks.php

I agree we should look into // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable, but not in this ticket.

spacedmonkey

Looking good.

westonruter · 2023-05-17T22:49:31Z

Committed to trunk in d5792c7

westonruter added 5 commits May 15, 2023 14:10

Upgrade phpcs to ^3.7.1

dbdb872

Require slevomat/coding-standard:^8.0

2a93ce6

Run composer run-script format -- --sniffs=SlevomatCodingStandard.Fun…

b99b81b

…ctions.StaticClosure

Move slevomat/coding-standard to require-dev

773c264

Improve order of deps in require-dev

92b1344

westonruter commented May 15, 2023

View reviewed changes

jrfnl requested changes May 16, 2023

View reviewed changes

spacedmonkey reviewed May 16, 2023

View reviewed changes

spacedmonkey requested changes May 16, 2023

View reviewed changes

westonruter mentioned this pull request May 16, 2023

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

Merged

3 tasks

westonruter added 4 commits May 16, 2023 14:37

Revert "Improve order of deps in require-dev"

e7a9b54

This reverts commit 92b1344.

Revert "Move slevomat/coding-standard to require-dev"

75127aa

This reverts commit 773c264.

Revert "Require slevomat/coding-standard:^8.0"

449429a

This reverts commit 2a93ce6.

Revert "Upgrade phpcs to ^3.7.1"

b252f86

This reverts commit dbdb872.

westonruter added 3 commits May 16, 2023 14:43

Revert changes to Gutenberg upstream files

fce99bc

Remove Slevomat from ruleset

23ded55

westonruter requested review from jrfnl and spacedmonkey May 16, 2023 21:49

jrfnl approved these changes May 16, 2023

View reviewed changes

spacedmonkey approved these changes May 17, 2023

View reviewed changes

westonruter closed this May 17, 2023

westonruter mentioned this pull request May 19, 2023

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

Merged

Use static closures whenever $this is not used to avoid memory leaks #4457

Use static closures whenever $this is not used to avoid memory leaks #4457

Uh oh!

Conversation

westonruter commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacedmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrfnl commented May 16, 2023

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacedmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use static closures whenever `$this` is not used to avoid memory leaks #4457

Use static closures whenever `$this` is not used to avoid memory leaks #4457

westonruter commented May 15, 2023 •

edited

Loading

jrfnl May 16, 2023 •

edited

Loading

westonruter May 15, 2023 •

edited

Loading

jrfnl May 16, 2023 •

edited

Loading

westonruter commented May 16, 2023 •

edited

Loading