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.

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

Remove empty attrs. #54496

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Remove empty attrs. #54496

merged 2 commits into from
Oct 13, 2023

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Sep 15, 2023

What?

Following work in #54424, this replaces the $empty_attrs object, which used to have a role, with an empty array wherever it was previously used.

Why?

The reason for having the property was that the block parser used to create a special object for empty attributes to disambiguate an empty object and an empty array in JSON. Because the block parser no longer creates this special object there's no more need to have an $empty_attrs property. It's not causing harm, but its continued presence might confuse people who are wondering what its purpose is.

How?

Directly replaces the property lookup with in-place array() construction.

Testing Instructions

A basic smoke test should verify failure because any problem in the block parser is likely to break a site in obvious ways. Ensure that the test suite passes.

@github-actions
Copy link

Flaky tests detected in 2cc7fc5.
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/6197299771
📝 Reported issues:

@dmsnell dmsnell added [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f Needs PHP backport Needs PHP backport to Core labels Sep 15, 2023
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

This seems good to me, thanks @spacedmonkey for following-up on #54496 #54424.

I'm leaving a pre-emptive approval on this because all that remains is filling in the PR description and taking care of some styling issues that CI will reject.

@mikachan
Copy link
Member

👋 @dmsnell This is on the 6.4 Editor Tasks board, so I'm wondering if this needs to be merged ready for the upcoming 6.4 RC. As @spacedmonkey is away at the moment, I wondered if you could help here. I'm happy to help fix the CS issues, but I'm not sure how high a priority this is or if it's not ready to be merged yet. Thank you!

@dmsnell
Copy link
Member

dmsnell commented Oct 13, 2023

@mikachan there is no urgency here, but it's ready apart from the styling linter not liking it. It's not a behavioral change so it shouldn't cause any hassle with or without it; it's just a follow-up to #54424 without a description, explanation, instructions, or links. I'll add a description.

@mikachan mikachan added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Needs PHP backport Needs PHP backport to Core labels Oct 13, 2023
@mikachan mikachan merged commit 8fa0d97 into trunk Oct 13, 2023
51 checks passed
@mikachan mikachan deleted the fix/types branch October 13, 2023 23:43
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 13, 2023
SiobhyB pushed a commit that referenced this pull request Oct 16, 2023
* Remove empty attrs.

* Fix linter errors

---------

Co-authored-by: Sarah Norris <[email protected]>
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 16, 2023

I just cherry-picked this PR to the 6.4-rc1-2 branch to get it included in the next release: 0afdb21

@SiobhyB SiobhyB removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 16, 2023
SiobhyB pushed a commit that referenced this pull request Oct 16, 2023
* Add selector as id to layout style overrides. (#55291)

* Fix flickering when focusing on global style variations (#55267)

* ProgressBar: use text color to ensure enough contrast against background (#55285)

* Use text color at different opacities for track and indicator

* Add high contrast mode styles

* CHANGELOG
# Conflicts:
#	packages/components/CHANGELOG.md

* Remove empty attrs. (#54496)

* Remove empty attrs.

* Fix linter errors

---------

Co-authored-by: Sarah Norris <[email protected]>

* Add IS_GUTENBERG_PLUGIN flag to LastRevision (#55253)

* useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered (#55288)

* useBlockPreview: Try alternative fix for displaying local style overrides

* Avoid duplicate styles, fix rendering issues in Safari

* Add more explanatory comments

* Remove additional check for styles within the block preview, as it is not needed since EditorStyles handles its own style overrides retrieval

* Bug: PHP notice when an image with lightbox is deleted (#55370)

* Fix PHP notice when an image with lightbox is deleted

* Fix PHP notice when an image with lightbox is deleted

---------

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Jonny Harris <[email protected]>
Co-authored-by: Andrew Serong <[email protected]>
Co-authored-by: Kishan Jasani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Performance Related to performance efforts
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants