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

Fix Code Standards for RC 2 release #59774

Merged
merged 8 commits into from Mar 12, 2024
Merged

Fix Code Standards for RC 2 release #59774

merged 8 commits into from Mar 12, 2024

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Mar 12, 2024 •
edited

What?

Updates code for WP 6.5 RC2 destined for WP Core to align with standards.

See WordPress/wordpress-develop#6253

Why?

CS

How?

Resolve all comments from WordPress/wordpress-develop#6253

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Co-authored-by: getdave get_dave@git.wordpress.org
Co-authored-by: swissspidy swissspidy@git.wordpress.org

@getdave getdave mentioned this pull request Mar 12, 2024
Closed
@getdave getdave added [Type] Code Quality Issues or PRs that relate to code quality Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 12, 2024
swissspidy
swissspidy reviewed Mar 12, 2024
View reviewed changes
packages/block-library/src/navigation/index.php
Comment on lines +1552 to +1555
/*
* Injection of hooked blocks into the Navigation block relies on some functions present in WP >= 6.5
* that are not present in Gutenberg's WP 6.5 compatibility layer.
*/
Copy link
Member

Choose a reason for hiding this comment

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

So all these comments are only really relevant for GB, not core, right? 'cause obviously when merging this change into core, these functions exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file is maintained in Gutenberg and those comments are relevant. Whilst I appreciate they aren't relevant for Core, we don't have a process for handling exclude comments at this time.

How critical is it to resolve this for RC 2?

Can we discuss a solution for RC 3?

Copy link
Member

Choose a reason for hiding this comment

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

Just a general remark, not a blocker. I suppose we could remove remove these hardening checks & comments in the future though, e.g. in 6.6?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think at some point we should gitignore these files in Core, they should be considered a "library" like any JS library. But I guess that change can only happen when/if we embrace php packages as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I raised this Discussion in the hope we can improve the whole process for further releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the Block Hooks related logic found in the Navigation block code specifically, we might be able to absorb it into the more generic inject_ignored_hooked_blocks_metadata_attributes function present in Core, see https://core.trac.wordpress.org/ticket/60759.

getdave and others added 2 commits March 12, 2024 10:47
@getdave
Restore actual code
b31bcbc
@getdave @swissspidy
Update doc block
aee6760 
Co-authored-by: Pascal Birchler <pascalb@google.com>
@getdave getdave marked this pull request as ready for review March 12, 2024 11:03
@getdave getdave requested a review from ajitbohra as a code owner March 12, 2024 11:03
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 12, 2024 •
edited

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ockham ockham requested a review from tjcafferkey March 12, 2024 11:38
@getdave
Copy link
Contributor Author

getdave commented Mar 12, 2024

Just waiting on an ✅ Approval 🙏

ockham
ockham approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thank you!

@getdave getdave requested a review from swissspidy March 12, 2024 12:02
@getdave
Copy link
Contributor Author

getdave commented Mar 12, 2024

@youknowriad I think we need to get this merged into order to make the release code freeze. Any further followups will need to wait until RC3 or we can discuss delaying the release.

@youknowriad
Copy link
Contributor

Merging, we don't have much time left before the release.

@youknowriad youknowriad merged commit fdf8fa6 into trunk Mar 12, 2024
61 checks passed
@youknowriad youknowriad deleted the fix/wp-code-standards-6-5-rc2 branch March 12, 2024 12:35
@getdave
Copy link
Contributor Author

getdave commented Mar 12, 2024

I'll cherry pick this to the wp branch and re-run the release

@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 12, 2024
getdave added a commit that referenced this pull request Mar 12, 2024
@getdave
Fix Code Standards for RC 2 release (#59774)
bfe5935 
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@git.wordpress.org>
# Conflicts:
#	packages/block-library/src/navigation/index.php
@getdave getdave added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 14, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
@getdave @swissspidy
@youknowriad @ockham @carstingaxion
Fix Code Standards for RC 2 release (WordPress#59774)
cc942f6 
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ockham ockham ockham approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

@dream-encode dream-encode Awaiting requested review from dream-encode

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

@tjcafferkey tjcafferkey Awaiting requested review from tjcafferkey

@swissspidy swissspidy Awaiting requested review from swissspidy

Assignees

@youknowriad youknowriad

@getdave getdave

Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 18.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@getdave @youknowriad @ockham @swissspidy