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.

Sign up for GitHub

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

Jump to bottom

Use static closures when not using $this #50723

Merged
merged 2 commits into from
May 31, 2023
Merged

Use static closures when not using $this #50723

merged 2 commits into from
May 31, 2023

Conversation

westonruter
Copy link
Member

What?

Add static keyword to closures that don't use $this

Why?

Since closures capture a reference to $this, memory leaks can ensue.

This applies the same change that was just made in Core-58323

How?

I used the SlevomatCodingStandard.Functions.StaticClosure sniff to apply the change with phpcbf.

Testing Instructions

n/a

Testing Instructions for Keyboard

n/a

Screenshots or screencast

@westonruter westonruter added the [Type] Performance Related to performance efforts label May 17, 2023
@gziolo
Copy link
Member

gziolo commented May 19, 2023

I see that @aristath opened another PR #50657 with similar changes.

CI errors reported are unrelated to this PR. I belive it must be reported on trunk, too.

@aristath aristath mentioned this pull request May 19, 2023
Closed
@aristath
Copy link
Member

I see that @aristath opened another PR #50657 with similar changes.

Yes, it was almost identical, inspired by @westonruter's PR in WordPress/performance#729 👍
I closed the other PR, we can continue with this one 🎉

spacedmonkey
spacedmonkey reviewed May 19, 2023
View reviewed changes
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This is nearly there. I have one nitpic, then I will approve.

@gziolo
Copy link
Member

gziolo commented May 19, 2023

This is nearly there. I have one nitpic, then I will approve.

Once approved, it can be merged with the admin permissions if the JS linting errors are the only blocker in the CI check.

@westonruter
Copy link
Member Author

@aristath Sorry that I didn't check for your PR first!

What do we think about including slevomat/coding-standard in the phpcs.xml.dist? It was discouraged from core in WordPress/wordpress-develop#4457 for Core-58323.

@westonruter
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into a…
89b582f 
…dd/static-closures

* 'trunk' of https://github.com/WordPress/gutenberg: (26 commits)
  Add transparent outline to input control BackdropUI focus style. (#50772)
  Added wrapper element for RichText in File block (#50607)
  Remove the experimental flag of the command center (#50781)
  Update the document title in the site editor to open the command center (#50369)
  Remove `unwrap` from transforms and add `ungroup` to more blocks (#50385)
  Add new experimental version of DropdownMenu (#49473)
  Force display of in custom css input boxes to LTR (#50768)
  Polish experimental navigation block (#50670)
  Support negation operator in selectors in the Interactivity API (#50732)
  Minor updates to theme.json schema pages (#50742)
  $revisions_controller is not used. Let's delete it. (#50763)
  Remove OffCanvasEditor (#50705)
  Mobile - E2E test - Update code to use the new navigateUp helper (#50736)
  Try: Smaller external link icon (#50728)
  Block Editor: Remove unused 'useIsDimensionsSupportValid' method (#50735)
  Fix flaky media inserter drag-and-dropping e2e test (#50740)
  docs: Fix change log typo (#50737)
  Edit Site: Fix `useEditedEntityRecord()` loading state (#50730)
  Fix labelling, description, and focus style of the block transform to pattern previews (#50577)
  Fix Global Styles sidebar block selection on zoom out mode (#50708)
  ...
@github-actions
Copy link

Flaky tests detected in 89b582f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5026422903
📝 Reported issues:

@westonruter
Copy link
Member Author

So include slevomat/coding-standard or not?

aristath
aristath approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

So include slevomat/coding-standard or not?

I suppose we could merge the improvements from this PR without including slevomat/coding-standard just to get this part out of the way, and create a separate issue to include the slevomat/coding-standard package where we can focus more on that discussion 👍

@westonruter westonruter merged commit d665608 into trunk May 31, 2023
@westonruter westonruter deleted the add/static-closures branch May 31, 2023 17:15
@github-actions github-actions bot added this to the Gutenberg 16.0 milestone May 31, 2023
@ramonjd ramonjd mentioned this pull request Jun 5, 2023
Closed
@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jun 5, 2023
@westonruter westonruter mentioned this pull request Jun 16, 2023
Closed
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 26, 2023
@westonruter
Editor: Use static closures to avoid memory leaks.
c7597fa 
Backports Gutenberg changes from WordPress/gutenberg#50723.

Amends [55822].

Fixes #58323.
Props westonruter, spacedmonkey, flixos90. 


git-svn-id: https://develop.svn.wordpress.org/trunk@56038 602fd350-edb4-49c9-b593-d223f7449a82
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Jun 26, 2023
@westonruter
Editor: Use static closures to avoid memory leaks.
366bf22 
Backports Gutenberg changes from WordPress/gutenberg#50723.

Amends [55822].

Fixes #58323.
Props westonruter, spacedmonkey, flixos90. 

Built from https://develop.svn.wordpress.org/trunk@56038


git-svn-id: https://core.svn.wordpress.org/trunk@55550 1a063a9b-81f0-0310-95a4-ce76da25c4cd
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 26, 2023
@westonruter
Editor: Use static closures to avoid memory leaks.
33f71c4 
Backports Gutenberg changes from WordPress/gutenberg#50723.

Amends [55822].

Fixes #58323.
Props westonruter, spacedmonkey, flixos90. 

Built from https://develop.svn.wordpress.org/trunk@56038


git-svn-id: http://core.svn.wordpress.org/trunk@55550 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@ramonjd ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 27, 2023
sethrubenstein pushed a commit to pewresearch/gutenberg that referenced this pull request Jul 13, 2023
@westonruter @sethrubenstein
Use static closures when not using $this (WordPress#50723)
722aca3
@Mamaduka Mamaduka mentioned this pull request Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aristath aristath aristath approved these changes

@spacedmonkey spacedmonkey spacedmonkey approved these changes

@ntwb ntwb Awaiting requested review from ntwb ntwb is a code owner

@nerrad nerrad Awaiting requested review from nerrad nerrad is a code owner

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@tellthemachines tellthemachines Awaiting requested review from tellthemachines tellthemachines is a code owner

Assignees
No one assigned
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Milestone
Gutenberg 16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@westonruter @gziolo @aristath @spacedmonkey @ramonjd