#58323 closed enhancement (fixed)
Use static closures whenever $this is not used to avoid memory leaks
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-unit-tests changes-requested |
Focuses: | performance, coding-standards | Cc: |
Description
Historically closures were not usable in WordPress since they were introduced in PHP 5.3 and WordPress supported PHP 5.2. However, now WordPress requires PHP 5.6 so closures have started making their way into the codebase. Whenever a closure is used inside of a class instance, the $this
is automatically captured by the closure. If the closure is added as a filter and not removed, for example, the class instance won't be able to be garbage collected even if there are otherwise no other references. For any such closures that don't reference $this
, this can be avoided by adding a static
keyword to the closure. Doing so prevents $this
from being captured and thus avoids a memory leak. See Stack Overflow answer.
This can be avoided in the future by incoporating the SlevomatCodingStandard.Functions.StaticClosure sniff.
Also implemented in PR for Performance Lab plugin.
Change History (16)
This ticket was mentioned in PR #4457 on WordPress/wordpress-develop by @westonruter.
21 months ago
#1
- Keywords has-patch has-unit-tests added
#2
follow-up:
↓ 3
@
21 months ago
- Focuses coding-standards added
- Keywords has-unit-tests removed
I've left a detailed review on the PR.
Summary:
- Making closures which don't use
$this
static
is a good improvement. - Including the Slevomat sniff is out of the question.
#3
in reply to:
↑ 2
@
21 months ago
Replying to jrf:
- Including the Slevomat sniff is out of the question.
+1
Left my thoughts on the PR.
@westonruter commented on PR #4457:
21 months ago
#4
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.
#5
@
21 months ago
- Milestone changed from Awaiting Review to 6.3
- Owner set to westonruter
- Status changed from new to assigned
21 months ago
#6
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.
@westonruter commented on PR #4457:
21 months ago
#9
#10
@
21 months ago
Also opened a PR for Gutenberg to apply the same change.
#11
@
20 months ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Re-opening to backport corresponding Gutenberg changes.
This ticket was mentioned in PR #4629 on WordPress/wordpress-develop by @westonruter.
20 months ago
#12
- Keywords has-unit-tests added
This backports the changes from https://github.com/WordPress/gutenberg/issues/51077
Trac ticket: https://core.trac.wordpress.org/ticket/58323
@spacedmonkey commented on PR #4629:
20 months ago
#13
@westonruter All the updates to the blocks php are unneeded. When the blocks are synced as part of the release, this will happen automatically.
#14
@
20 months ago
- Keywords changes-requested added
This ticket was discussed in the recent performance bug scrub.
There is some feedback on the PR that needs to be addressed.
Props to @spacedmonkey
@westonruter commented on PR #4629:
20 months ago
#16
Committed in https://core.trac.wordpress.org/changeset/56038
SlevomatCodingStandard.Functions.StaticClosure
sniff from slevomat/coding-standardstatic
keyword to eligible closures viaRun composer run-script format -- --sniffs=SlevomatCodingStandard.Functions.StaticClosure
Trac ticket: https://core.trac.wordpress.org/ticket/58323