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

Editor: Initial WordPress package updates for 6.5 #5922

Closed
wants to merge 12 commits into from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jan 22, 2024

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

This PR is going to represent the initial packages update commit for 6.5 to bring all the latest updates from the Gutenberg repository. We will most likely discover shortcomings or requirements that need to be backported within this PR or prior to this PR.

Requirements

@youknowriad youknowriad self-assigned this Jan 22, 2024
@youknowriad
Copy link
Contributor Author

youknowriad commented Jan 22, 2024

Hey @luisherranz When I updated the packages, I was forced to update the build config for the Interactivity API related code because these are meant to be esmodules now. I made some incomplete tweaks so far.

  • For example, I don't see the src/wp-includes/blocks/*/view.js files for some reason
  • I'm guessing we also need to add some php code to the script loader of WordPress to "auto register" the modules (the view scripts and the interactivity package so far).
  • Also, I added a new webpack config file to core that is specific to the packages that are written as modules, I'm assuming the interactivity package is just the first one and probably not the last

@luisherranz
Copy link
Member

I'll take a look. Thanks, @youknowriad!

package.json Show resolved Hide resolved
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

I was able to give this a test run locally once I'd commented out the line calling WP_Navigation_Block_Renderer and it seems to be working pretty well 👍
Once the packages are updated to the latest versions I can give it another go.

Regarding the PHP tests, looks like there are some legit failures (unrelated to the errors caused by https://core.trac.wordpress.org/ticket/59969). I think the Quote fixtures may need updating.

src/wp-includes/blocks/block.php Outdated Show resolved Hide resolved
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.

@luisherranz
Copy link
Member

This is a preliminary fix to bundle and register the new Interactivity API modules: 8a06712

  • It contains a new file called interactivity-api.php which should be removed once the backport for the WP_Interactivity_API class lands. I'll remove it in the backport itself.
  • It contains some extra hardcoded code in the Webpack files due to the fact that the build-module output of the Interactivity API packages still contains React's JSX pragma instead of Preact's. @sirreal is working on that.
  • The @wordpress/interactivity-router package is not working yet. I'll look into it and I'll report back.

@sirreal
Copy link
Member

sirreal commented Jan 25, 2024

  • It contains some extra hardcoded code in the Webpack files due to the fact that the build-module output of the Interactivity API packages still contains React's JSX pragma instead of Preact's. @sirreal is working on that.

PR with short-term fix is here: WordPress/gutenberg#58258

@luisherranz
Copy link
Member

  • The @wordpress/interactivity-router package is not working yet. I'll look into it and I'll report back.

5c8c5af solves the problem with the router, but this fix needs to wait until this PR is committed because without the caniuselite package update, Webpack refuses to bundle:

It also uncovered a bug when @wordpress/interactivity-router is imported statically that we will solve in Gutenberg.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

I'm not sure how draft this is so left a couple of comments as I've seen a couple of reviews.

I'll submit some upstream PRs if I have any nitpicks.

defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN ? gutenberg_url( '/build/interactivity/file.min.js' ) : includes_url( 'blocks/file/view.min.js' ),
array( '@wordpress/interactivity' ),
defined( 'GUTENBERG_VERSION' ) ? GUTENBERG_VERSION : get_bloginfo( 'version' )
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for GB to deregister the Core modules and re-register it's own versions in the plugin? That would allow us to avoid adding references to gutenberg_url() in WP Core.

Genuine question, not a change request: I haven't kept up to date wiith the script module API.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we still need to figure out that logic. It is still not clear how we're going to do that.

src/wp-includes/blocks/footnotes.php Show resolved Hide resolved
@youknowriad
Copy link
Contributor Author

Anyone know why the phpunit are failing on some php versions? Did we use some unsupported syntax or something? I'm having hard time reading the job jobs.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Jan 26, 2024

Anyone know why the phpunit are failing on some php versions? Did we use some unsupported syntax or something? I'm having hard time reading the job jobs.

It looks like some of the jobs are hitting the 20 minute timeout limit and getting abandoned.

@youknowriad Maybe try bumping the value on this branch to 30 minutes to see if it helps. It shouldn't cause any problems running the actions but me know if it does.

@youknowriad
Copy link
Contributor Author

With the increased timeout, the tests are starting to pass. Here's my suggested plan:

Given that this PR is kind of mandatory for 6.5 and a requirements for other backports to land, we need to try and land it quickly to prevent any potential delays in Beta1 later. So I suggest that we merge the PR with the timeout increase in the tests given that the performance tests don't show any regression in terms of real user WordPress performance.

That said, it's still concerning to me that the tests are so slow with this patch and I'll create a follow-up trac ticket that we'll have to address and identify the problematic change before beta1 while still allowing follow-up backports to happen in the meantime.

@getdave
Copy link
Contributor

getdave commented Jan 29, 2024

What @youknowriad suggests seems logical to me, especially if performance tests don't suggest major regressions that a user would experience.

We should try to avoid blocking the packages sync but be sure to followup in short order with debugging why the PHP tests have required a increased timeout.

In terms of performance, looking at the diff I’m suspicious of the PHP changes to the Navigation and Navigation Link blocks.

There have been two major changes:

There was also WordPress/gutenberg#57979 but that just moved all existing rendering code into a class so that's probably the least likely source.

We should start investigating those. Any suggestions for workflows to validate before/after would be greatly appreciated.

@youknowriad
Copy link
Contributor Author

@getdave That's good starting point.

The test run fails for some versions even with 60mn of timeout, basically I think we'd need something like 70mn for all the tests to pass. It's a lot. Let's give it one more day before committing this and in the mean time, let's try to comment some of that suspicious code you just shared to see if we find anything.

@getdave
Copy link
Contributor

getdave commented Jan 29, 2024

Let's give it one more day before committing this and in the mean time, let's try to comment some of that suspicious code you just shared to see if we find anything.

60 minutes seems excessive. It would be good to try a little bit more debugging first.

I'll try a fork which removes the changes to Nav files so we can rule those changes in/out.

@getdave
Copy link
Contributor

getdave commented Jan 29, 2024

Folks I think the culprit is the Navigation Link block file src/wp-includes/blocks/navigation-link.php. Specifically changes made in this PR WordPress/gutenberg#54801.

You can see that on my fork of this packages sync PR I have reverted the synced changes introduced to src/wp-includes/blocks/navigation-link.php and all the Unit Tests run successfully.

I suspect that these two lines need to be looked at carefully,although they don't seem to do much "work". Turns out they run on every single post type registration so it could be it

@getdave
Copy link
Contributor

getdave commented Jan 29, 2024

Please see #5967 (comment)

I believe WordPress/gutenberg#58389 will resolve our unit test problem.

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

I left a few questions that may or may not be relevant (I have not become deeply familiar with the Interactivity API, or the Navigation block changes), but I have some concerns around the fact that a large handful of functions/methods and arguments are being removed in this PR that were previously included in a publicly available version of WordPress without receiving proper (_deprecated_argument()|_deprecated_function()|_deprecated_method()) treatment.

Also, the 5x increase in job runtime on PHP <= 7.2 is a blocker for me. That will have cascading impact across the GitHub org that will could potentially result in a bottle neck of queued jobs. I don't see anything obvious standing out, but a few things I noticed that seem like they could be impactful from a performance standpoint:

  • What does "overrides": { "type": "object" } do? I see that added to a good number of blocks.
  • The blocks/blocks-json.php file grew by quite a bit.
  • A lot type and source content attributes changed from string/html to rich-text/rich-text.

My recommended strategy here would be to create other PRs and incrementally apply the updates here in as small of a chunk as possible to see where the slowness is introduced. Alternatively you could revert certain built files in version control one at a time and allow the tests to run for each. The workflow may fail because there is a check to ensure all versioned files are untouched, but we don't run build before running unit tests, so those steps should complete.

.github/workflows/test-old-branches.yml Outdated Show resolved Hide resolved
src/wp-includes/blocks/social-link.php Show resolved Hide resolved
}
}

function render_block_core_file( $attributes, $content ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the third parameter removed here? It looks like it was included in the last release of WP, so _depreceted_argument() should be used here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here:
#5922 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

*
* @global WP_Scripts $wp_scripts
*/
function block_core_file_ensure_interactivity_dependency() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're removing a function that was included in a previous release of WordPress, it should be moved to deprecated-functions.php and be properly documented as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in cbravobernal@a3eba9d

It seems that I cannot make PRs to your fork @youknowriad

Copy link
Contributor

Choose a reason for hiding this comment

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

We should try and make any changes in the Gutenberg repo and then update this PR so that we have a clear audit trail. Otherwise we're going to have a lot of Backport from WordPress Core PRs which can become very confusing.

The optimal flow is:

Gutenberg -> WP Core.

@@ -32,6 +32,21 @@ function block_core_gallery_data_id_backcompatibility( $parsed_block ) {

add_filter( 'render_block_data', 'block_core_gallery_data_id_backcompatibility' );

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a @since notation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t-hamano Can you please follow-up with a PR in Gutenberg to address the comments that are related to the changes here

Choose a reason for hiding this comment

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

Sorry for the late confirmation. I have submitted a PR.

WordPress/gutenberg#58440

src/wp-includes/blocks/post-terms.php Show resolved Hide resolved
*
* @global WP_Scripts $wp_scripts
*/
function block_core_query_ensure_interactivity_dependency() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another deprecated function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in cbravobernal@a3eba9d

It seems that I cannot make PRs to your fork @youknowriad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c4rl0sbr4v0 Make them directly to WordPress/wordpress-develop. The current PR has been committed already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #5973

@@ -16,7 +16,7 @@
*
* @return string The search block markup.
*/
function render_block_core_search( $attributes, $content, $block ) {
function render_block_core_search( $attributes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated parameters should be treated with _deprecated_argument().

*
* @global WP_Scripts $wp_scripts
*/
function block_core_search_ensure_interactivity_dependency() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another deprecated function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding render_block_core_search and all render_callback functions. The reason of removing those parameters is that they are not used for those core renders anymore. As they are part of render_callback, they will be still be available for any filter/action that replace them. Like:

function plugin_new_search($attributes, $content, $block) {
  var_dump($block); // print $block object.
}

add_filter('register_block_type_args', function ($settings, $name) {
    if ($name == 'core/search') {
        $settings['render_callback'] = 'plugin_new_search';
    }
    return $settings;
}, null, 2);

Adding a deprecate argument like this one will always show:

function render_block_core_search( $attributes, $deprecated_block_content = null, $deprecated_block = null ) {
	if ( $deprecated_block_content || $deprecated_block ) {
		_deprecated_argument( __FUNCTION__, '6.5.0' );
	}..
}

Screenshot 2024-01-29 at 18 35 26

As the render callback will always pass those variables.

Other option would be leave them there without using them on the functions and disable the phpcs linter for that line:
Screenshot 2024-01-29 at 18 36 57

I will be happy to update here: WordPress/gutenberg#58391 with the solution you best consider.

I can also take care of deprecating all the Interactivity API functions related.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in cbravobernal@a3eba9d

It seems that I cannot make PRs to your fork @youknowriad

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #5973

@@ -275,8 +281,8 @@ function register_block_core_template_part() {
register_block_type_from_metadata(
__DIR__ . '/template-part',
array(
'render_callback' => 'render_block_core_template_part',
'variations' => build_template_part_block_variations(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would developers be calling register_block_type_from_metadata() with variations? If so, is a dev note in the works for this change? What impact does this have if someone does not update to variations_callback? Will all their variations disappear? Is there any way we can detect this and use a _doing_it_wrong() warning to guide devs to use the new format?

Also, it doesn't look like variations_callback was added to the $property_mappings in register_block_type_from_metadata(). Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kt-12 might know more here.

@desrosj
Copy link
Contributor

desrosj commented Jan 29, 2024

@getdave Thanks for the update above! I missed it while performing my review. Just wanted it clear that my feedback was given without your new context, so read it with that lens.

@youknowriad
Copy link
Contributor Author

With the last commit, I was able to add a code to the bootstrap of the php unit tests to unregister the problematic hooks. Now the tests run properly.

This is going to help us unblock this PR while we work on the different follow-ups for the next package upgrade.

@youknowriad
Copy link
Contributor Author

I went ahead and landed a first version of the package update PR in https://core.trac.wordpress.org/changeset/57377
This will unblock the other backports from the Gutenberg repository.

I've left the ticket open for follow-ups, mainly the deprecated functions and the change for the navigation link variations.

@youknowriad
Copy link
Contributor Author

closing this one now, follow-up here #5984

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.