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

Update packages for WordPress 6.5 RC 2 #6253

Closed
wants to merge 2 commits into from

Conversation

getdave
Copy link

@getdave getdave commented Mar 12, 2024

Packages update for the WP 6.5 RC2 release.

Based on

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


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.

Copy link

github-actions bot commented Mar 12, 2024

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props get_dave, swissspidy, bernhard-reiter, youknowriad.

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

@getdave
Copy link
Author

getdave commented Mar 12, 2024

@ockham Pinging you here to check the changes to Navigation are as expected based on your fix for the entities issue that was uncovered during RC 1 🙏

Also @youknowriad @dream-encode @swissspidy for review.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Member

Choose a reason for hiding this comment

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

Aside: this whole file has a lot of incorrect multiline comments, incorrect docblocks (missing @since and documented params that don't actually exist, e.g. in block_core_navigation_insert_hooked_blocks_into_rest_response), and weird function_exists checks for functions that are clearly in core.

Copy link
Author

@getdave getdave Mar 12, 2024

Choose a reason for hiding this comment

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

We can correct any obvious documentation problems including comments, docblocks...etc.

However, this file is maintained in the Gutenberg repo and so there will be additional checks in the file given its home in Gutenberg.

Currently we don't have a process where we can ship different code in Core than in the Gutenberg repo when code is maintained in the Gutenberg repo (as this file is). We can explore a solution for future releases but we cannot solve that prior to RC2 today.

My opinion is:

  • we fix any critical code issues (those you specifically identified) and move forward with package sync quickly in time for RC 2 13:00 UTC code freeze.
  • we raise a followup PR to further standardise the remainder of the file in time for RC3
  • we raise an Issue to discuss a means to improve compatibility with PHP files that are maintained in Gutenberg but which are synced to Core.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Dave. Sorry about the coding standards issues, I probably introduced some of those myself and/or didn't catch them in review; I'm a bit surprised that they weren't flagged by WPCS in CI.

However, this file is maintained in the Gutenberg repo and so there will be additional checks in the file given it's home in Gutenberg.

Currently we don't have a process where we can ship different code in Core than in the Gutenberg repo when code is maintained in the Gutenberg repo (as this file is). We can explore a solution for future releases but we cannot solve that prior to RC2 today.

Yeah, this is crucial. Those function_exists checks are unfortunate, but they are currently required, due to the reasons that Dave mentioned.

In the future, we might be able to harmonize some of this code with what we have for templates (possibly rendering some of the code here obsolete), but that's clearly not within the scope for RC2. (I'll create a ticket for that shortly.)

For now, I agree with Dave's plan 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, we might be able to harmonize some of this code with what we have for templates (possibly rendering some of the code here obsolete), but that's clearly not within the scope for RC2. (I'll create a ticket for that shortly.)

https://core.trac.wordpress.org/ticket/60759

getdave added a commit to WordPress/gutenberg that referenced this pull request Mar 12, 2024
getdave added a commit to WordPress/gutenberg that referenced this pull request Mar 12, 2024
getdave added a commit to WordPress/gutenberg that referenced this pull request Mar 12, 2024
@getdave
Copy link
Author

getdave commented Mar 12, 2024

This PR is being discussed in WP Core Slack and addressed in WordPress/gutenberg#59774

@ockham
Copy link
Contributor

ockham commented Mar 12, 2024

@ockham Pinging you here to check the changes to Navigation are as expected based on your fix for the entities issue that was uncovered during RC 1 🙏

Confirming that it's fixed 🎉

image

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.

I've tested the changes to the Navigation block. Confirming that the entities issue is fixed ✅

LGTM!

@youknowriad
Copy link
Contributor

The performance job seem to be failing, I doubt that it's limited to this PR but do we know anything about that?

@getdave
Copy link
Author

getdave commented Mar 12, 2024

Downloading update from https://wordpress.org/wordpress-6.1.1.zip...

Seems to be related to Docker unable to download this.

@getdave
Copy link
Author

getdave commented Mar 12, 2024

I'm re-publishing the @wordpress/* packages and will sync this branch up shortly.

@youknowriad
Copy link
Contributor

We should also remove the pattern overrides code in this PR I think.

@getdave
Copy link
Author

getdave commented Mar 12, 2024

Packages were published and I've updated this PR to reflect the changes there.

@getdave
Copy link
Author

getdave commented Mar 12, 2024

Performance tests are failing due to errors during environment setup. Latest one is that it cannot download:

https://downloads.wordpress.org/plugin/wordpress-importer.0.8.2.zip...

The link goes to 404.

@swissspidy
Copy link
Member

@getdave That's just GitHub accidentally including the dots in the link. The actual download is https://downloads.wordpress.org/plugin/wordpress-importer.0.8.2.zip without dots, and that works.

@getdave
Copy link
Author

getdave commented Mar 12, 2024

Tests are ✅ Please can we now commit this?

@swissspidy
Copy link
Member

I think @youknowriad just did in https://core.trac.wordpress.org/changeset/57814, so closing

@swissspidy swissspidy closed this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants