Navigation Link: use get_block_type_variations to register variations #58389

gaambo · 2024-01-29T15:34:52Z

This is an exploration to follow up to #54801, but using the new get_block_type_variations filter to register those variations.
This has performance benefits (see #56952) and also should fix the phpunit timeouts happening after merging the original PR and merging that into core.

Why?

See Navigation Link Block: Variations for post types may not contain all post types #53826 for original bug
New lazy-loading/cached API for block type variations was added for performance reasons (see Remove building wp_template_parts variation call for frontend wordpress-develop#5718)
Original PR made core unit tests timeout (see comment: Navigation Link block: register variations on post type / taxonomy registration #54801 (comment))

How?

Instead of using the registered_post_type and registered_taxonomy hooks (which lead to timeouts an can be a performance issue on sites with many CPTs/taxonomies), this uses the new get_block_type_variations filter.
Since Gutenberg has to support the last two core versions and the old functions have been in Gutenberg since November, those were kept in and marked deprecated (I've still added a @since and a @deprecated tag).
The unit tests use the old deprecated methods on core versions <6.5 so unit tests work on older and new versions.

This way:

Core versions < 6.5 have the same experience as before the changes in november: CPTs/taxonomies registered after init#10 don't have a variation)
Core versions >= 6.5 have the added performance boost of using get_block_type_variations

Alternatives tried
Using the variation_callback instead of the filter was tried, and would register CPTs/taxonomies registered after init#10. But unregistering them would not unregister those variations, since the callback is only called once per request.

Testing Instructions

Automated testing

Run npm run test:unit:php:base -- --filter Block_Navigation_Link_Variations_Test

Manual Testing

Post Type

Register a custom post type (example code from developer.wordpress.org).

add_action('init', function () {
    register_post_type(
        'wporg_product',
        array(
            'labels'      => array(
                'name'          => __('Products', 'textdomain'),
                'singular_name' => __('Product', 'textdomain'),
                'item_link' => __('Product Link', 'textdomain')
            ),
            'public'      => true,
            'has_archive' => true,
            'show_in_rest' => true,
            'show_in_nav_menus' => true
        )
    );
}, 11);

Important: Set the item_link label or the block variation is called Post Link.
Important: change add_action priority to something higher than 10 (eg 11) - when using something lower it already works in trunk.
2. Go into site-editor and add a navigation block
3. try to search for a block named like the custom taxonomy (eg "Product Link").
4. Block is available

Taxonomy

Register a custom taxonomy (example code from developer.wordpress.org)

add_action('init', function() {
	 $args   = array(
		 'labels'            => array(
		     'link_item'         => __( 'Course' ),
	         ),
		 'show_in_nav_menus' => true
	 );
	 register_taxonomy( 'course', [ 'post' ], $args );
});

Important: Set the item_link label or the block variation is called Post Link.
Important: change add_action priority to something higher than 10 (eg 11) - when using something lower it already works in trunk.
3. Go into site-editor and add a navigation block
4. try to search for a block named like the custom taxonomy (eg "Product Link").
5. Block is available

Testing unregistering

Post Type

Register a custom post type (example code from developer.wordpress.org).

add_action('init', function () {
    register_post_type(
        'wporg_product',
        array(
            'labels'      => array(
                'name'          => __('Products', 'textdomain'),
                'singular_name' => __('Product', 'textdomain'),
                'item_link' => __('Product Link', 'textdomain')
            ),
            'public'      => true,
            'has_archive' => true,
            'show_in_rest' => true,
            'show_in_nav_menus' => true
        )
    );
}, 11);

Important: Set the item_link label or the block variation is called Post Link.
Important: change add_action priority to something higher than 10 (eg 11) - when using something lower it already works in trunk.
2. Unregister the custom post type after the navigation link block is registered and the variation is created:

add_action('init', function () {
    unregister_post_type('wporg_product');
}, 100);

Go into site-editor and add a navigation block
try to search for a block named like the custom taxonomy (eg "Product Link").
Block variation should not be available

Taxonomy

Register a custom taxonomy (example code from developer.wordpress.org)

add_action('init', function() {
	 $args   = array(
		 'labels'            => array(
		     'link_item'         => __( 'Course' ),
	         ),
		 'show_in_nav_menus' => true
	 );
	 register_taxonomy( 'course', [ 'post' ], $args );
}, 11);

Important: Set the item_link label or the block variation is called Post Link.
Important: change add_action priority to something higher than 10 (eg 11) - when using something lower it already works in trunk.
2. Unregister the custom taxonomy after the navigation link block is registered and the variation is created:

add_action('init', function () {
    unregister_taxonomy('course');
}, 100);

Go into site-editor and add a navigation block
try to search for a block named like the custom taxonomy (eg "Course Link").
Block variation should not be available

Co-authored-by: gaambo [email protected]
Co-authored-by: getdave [email protected]
Co-authored-by: youknowriad [email protected]
Co-authored-by: Andrei Draganescu [email protected]

getdave · 2024-01-29T15:58:07Z

I tested this by applying the code directly from this diff to WordPress/wordpress-develop#5967.

Now all the PHPUnit tests pass without timeout which makes me think this is the culprit of the issue there.

Let's look to bring this in.

gaambo · 2024-01-29T16:42:55Z

Regarding backwards-compatibility:

The changes causing the timeout have been in Gutenberg since November, so the two functions register_block_core_navigation_link_post_type_variation and register_block_core_navigation_link_taxonomy_variation have been in there. But they haven't been in any core version yet. Do we have to keep them? What's the rule here?
Up until core 6.4 variations are registered on block type registration, therefore at least core CPTs/taxonomies and all registered up to init#10 have their variations created. Only CPTs/taxonomies registered later don't have a variation created.
The new way using the get_block_type_variations will only work in WordPress 6.5, because this filter was only added there
If the code will only use the new filter, and does not register variations on block type registration, there will be no CPT/taxonomy variations created on older versions using this newer Gutenberg version.
As a workaround, I could keep the code on block type registration to register all variations known until there and then on get_block_type_variations register all newly added CPTs/taxonomies - like so:

register_block_type_from_metadata(
		__DIR__ . '/navigation-link',
		array(
			'render_callback' => 'render_block_core_navigation_link',
			'variations'      => build_navigation_link_block_variations(),
		)
	);

BUT that would mean, all the performance benefits of the variations_callback will not be used in this block.

I could instead check if the get_block_type_variations filter is available and then use that OR set the variations property for older WordPress versions. But is there any precedent for that? Would it be okay to check the version from ABSPATH . WPINC . '/version.php';
Unit testes would need to be adapted whether get_variations is available or not. The best way would probably to not use unit tests, but instead use e2e tests in the editor to check the availability of those variations?

gaambo · 2024-01-29T17:35:59Z

After trying some things out, I came to the following conclusions

We can't fix the original bug for core versions prior to 6.5. Because all shims and workarounds would still run on block type registration and therefore only register variations for previously registered CPTs/taxonomies. The only way to fix this bug was by hooking into registered_post_type/registered_taxonomy.
We can only try to fix it for WP 6.5 and newer versions which have the get_block_type_variations filter.
Therefore, the unit tests in Block_Navigation_Link_Variations_Test should therefore actually only run on WordPress 6.5+ - since the test set_up function runs after block type registration. Is that possible? Otherwise they will constantly fail.

If anybody else has some ideas, that would be great 😅

youknowriad · 2024-01-29T19:29:01Z

I don't know much about the problem but I can answer some of the questions above.

The changes causing the timeout have been in Gutenberg since November, so the two functions register_block_core_navigation_link_post_type_variation and register_block_core_navigation_link_taxonomy_variation have been in there. But they haven't been in any core version yet. Do we have to keep them? What's the rule here?

Ideally we deprecate and remove since they were not in Core but if we think that it's very unlikely that these things are used by third-party code, I believe it's fine to just remove them.

If the code will only use the new filter, and does not register variations on block type registration, there will be no CPT/taxonomy variations created on older versions using this newer Gutenberg version.

I think that's fine personally. What would it take to support previous core versions? (6.4 and 6.3), if it's possible to do so somehow, we can potentially add this code to lib/compact/wordpress-6.5 folder or something (so it doesn't get included into core but only runs in the plugin with an additional version check for this code)

gaambo · 2024-01-30T08:56:51Z

@youknowriad thank you for your input!

I just pushed a commit which

deprecates the functions in navigation-block/index.php (which leads to them having a since and deprecated docblock with the same version 😅)
adds to-dos that these functions can be removed in core 6.7/when gutenberg does not need to support core versions <6.5 anymore. (I can create issues for them and assign myself and make a reminder).
fixes the unit tests for core versions < 6.5 (by using the deprecated methods above and expecting them).

I've also updated the PR description with all this new information.

This should also include the code style changes from #58385 (so i closed that one).

gaambo · 2024-02-05T10:00:41Z

@getdave should we try bringing this in for the next release, or do you think the workaround with disabling the filters in unit tests is enough for now?
For the future, I still think using the new get_block_type_variations should definitely be done, since it was implemented exactly for a use case like this.

github-actions · 2024-02-05T10:00:57Z

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: gaambo <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: draganescu <[email protected]>
Co-authored-by: youknowriad <[email protected]>

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

getdave · 2024-02-06T16:19:25Z

@gaambo I'm going to review this as a priority tomorrow and try and catch up here.

getdave · 2024-02-08T11:05:16Z

Looking now

getdave · 2024-02-08T11:07:17Z

OPTIONAL - Confirm if PHP changes require backporting to WordPress Core / Detect PHP changes (pull_request) Failing after 1m

I can confirm these changes will not need backporting to WP 6.5 so long as this PR lands in time for the Gutenberg Plugin 17.7.0 RC release on Friday 9th Feb 2024.

getdave

Let's bring this in. A few nits.

Thank you for all the work on this 🙇

packages/block-library/src/navigation-link/index.php

phpunit/blocks/block-navigation-link-variations-test.php

draganescu

Did not test but the code updates LGTM apart from some nits.

packages/block-library/src/navigation-link/index.php

gaambo · 2024-02-08T12:33:27Z

Thanks for the feedback. I applied it and rebased :)

getdave · 2024-02-09T11:57:53Z

I restarted all the tests. We've had some trouble in trunk with test failures due to mismatched packages and things so let's check if these failures are legit.

getdave · 2024-02-09T12:14:57Z

@youknowriad Please could I get a confidence check here that:

e2e failures are the Synced Pattern problems and not related to this PR?
PHPUNit failures are Interactivity API failures that we've been seeing elsewhere

I don't think any of the tests are faling due to changes in this PR. It @gaambo can sync his fork and then rebase this PR the tests should pass but that might not be possible prior to the 17.7.0 RC today. If not then I'd like to force merge here.

gaambo · 2024-02-09T12:18:31Z

Just rebased the PR, so let's see if tests go through now :)

youknowriad · 2024-02-09T12:49:48Z

Aside the performance tests that are still failing on trunk, everything should be passing.

youknowriad · 2024-02-09T12:50:54Z

packages/block-library/src/navigation-link/index.php

+ * @since 6.5.0
+ * @deprecated 6.5.0 Use WP_Block_Type::get_variations / get_block_type_variations filter instead.
+ *
+ * TODO: After two WP versions (6.7), we can remove this.


If this is here only to ensure compatibility with previous versions in the Gutenberg plugin, can we define these functions outside the "block-library" package or no?

It's there for two reasons:

The functions have been in Gutenberg since November (but no core version)

They provide backwards-compatibility when the Gutenberg plugin is used with WP versions < 6.5

Do you mean, I could move them into the lib/compat/wordpress-6.5 directory?

Do you mean, I could move them into the lib/compat/wordpress-6.5 directory?

Yes, since these functions are "Gutenberg specific" and shouldn't "run" in Core. I was wondering if there's a way to keep the "navigation" block code "core only" and add the compat code from the compat folder. The reason is that the navigation block code will be copied as is into Core.

@youknowriad I understand.
I did that in dd6857c
I disabled the phpcs rules for these functions, since they were never in core and therefore imho shouldn't need a guarded clause. But if that thought was wrong, let me know.
Tests are passing locally. Only two other tests related to WP_Style_Engine_CSS_Rule_Gutenberg::set_at_rule are failing locally - but let's see what the GitHub tests say.

Maybe I'm missing something the functions are still in the navigation-link/index.php file?

getdave · 2024-02-09T13:47:12Z

PHPUnit failure:

Fatal error: Cannot redeclare block_core_navigation_link_unregister_taxonomy_variation() (previously declared in /var/www/html/wp-includes/blocks/navigation-link.php:488) in /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.5/navigation-block-variations.php on line 80

It's likely because the code already existed in trunk and was merged into WP Core trunk during the initial packages sync. However, I understand that it will be removed if/when we merge this PR and perform the final packages sync following the Gutenberg 17.7.0 RC today.

We could add a guard clause for safety? 🤷

@gaambo Not sure if you already did this but you might find it easier to:

update your fork with the upstream Gutenberg repo.
rebase this PR against the updated trunk.

That way we should bring in any fixes from trunk for the failing PHPUNit and e2e tests and that will give us a clearer picture of legit vs false failures 🙏

gaambo · 2024-02-09T13:52:41Z

Oh, because the code was already once synced into core in the previous packages update. Yeah, of course - I'll just add the guard clause.

youknowriad · 2024-02-09T13:53:32Z

@gaambo not really, we should not guard but just use prefixes functions gutenberg_ now that the code has moved to the 6.5 folder.

youknowriad

So I didn't test but the diff looks good to me.

lib/compat/wordpress-6.5/navigation-block-variations.php

getdave · 2024-02-09T15:00:11Z

🎉 🎉 🎉

draganescu · 2024-02-09T16:30:17Z

Huzzah!

jsnajdr · 2024-02-16T12:50:13Z

lib/compat/wordpress-6.5/navigation-block-variations.php

+	if ( 'core/navigation-link' !== $args['name'] || ! empty( $args['variation_callback'] ) ) {
+		return $args;
+	}
+	$args['variation_callback'] = 'build_navigation_link_block_variations';


This compat code doesn't work because the build_navigation_link_block_variations function does not exist.

The consequence is that in WordPress 6.4 the Navigation Link variations for post types (Post Link, Page Link) are not registered, we have only "Custom Link" available in the editor.

I was seeing strange Playwright e2e failures in my PR #59064 and I tracked them down to this. The e2e tests for the core/navigation block are failing because they expect "Page Link" components, but they see only "Custom Link" ones. See for example this code:

gutenberg/test/e2e/specs/editor/blocks/navigation-list-view.spec.js

Lines 103 to 112 in 237865f

await expect(

listView

.getByRole( 'gridcell', {

name: 'Page Link',

} )

.filter( {

hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.

} )

.getByText( 'Top Level Item 1' )

).toBeVisible();

Oh gosh, this PR (or my work on it) seems to be cursed 🙈
You are right, the functions name should be gutenberg_block_core_navigation_link_build_variations - since that's the prefixed name after building.
If I change it to that and use WP 6.4 locally, I get "Page Link" and "Post Link" variations correctly.
I wonder why no other tests caught that, but since the function is just passed as a callback it's not called directly and therefore doesn't trigger an error - probably that's why.

I've created a PR to fix that here: #59126

gaambo mentioned this pull request Jan 29, 2024

Navigation Link block: register variations on post type / taxonomy registration #54801

Merged

getdave added a commit to getdave/wordpress-develop that referenced this pull request Jan 29, 2024

Try code from WordPress/gutenberg#58389

0aae4bd

getdave requested review from draganescu, youknowriad and anton-vlasenko January 29, 2024 15:58

getdave added [Type] Bug An existing feature does not function as intended [Block] Navigation Link Affects the Navigation Link Block labels Jan 29, 2024

getdave assigned gaambo Jan 29, 2024

This was referenced Jan 29, 2024

Update packages 65 no nav changes WordPress/wordpress-develop#5967

Closed

Editor: Initial WordPress package updates for 6.5 WordPress/wordpress-develop#5922

Closed

gaambo marked this pull request as ready for review January 30, 2024 08:57

gaambo requested a review from tellthemachines as a code owner January 30, 2024 08:57

gaambo mentioned this pull request Jan 30, 2024

Fix/navigation link cs #58385

Closed

youknowriad requested a review from desrosj January 30, 2024 09:02

gaambo mentioned this pull request Feb 1, 2024

Register post terms variations on taxonomy registration #56080

Closed

gaambo force-pushed the try/navigation-link-variations-filter branch from b129d29 to 35585bd Compare February 1, 2024 16:13

getdave approved these changes Feb 8, 2024

View reviewed changes

draganescu reviewed Feb 8, 2024

View reviewed changes

packages/block-library/src/navigation-link/index.php Outdated Show resolved Hide resolved

packages/block-library/src/navigation-link/index.php Outdated Show resolved Hide resolved

gaambo force-pushed the try/navigation-link-variations-filter branch from 35585bd to a142a17 Compare February 8, 2024 12:32

gaambo force-pushed the try/navigation-link-variations-filter branch from 0471b9c to ab2afaf Compare February 9, 2024 12:17

youknowriad reviewed Feb 9, 2024

View reviewed changes

gaambo added 5 commits February 9, 2024 14:22

Nav Link: use get_block_type_variations to register variations

a0af054

wp <6.5 compat

62491c0

update code comments according to PR feedback

a6ebdda

remove todo

641db4e

moved deprecated functions into compat directory

dd6857c

gaambo force-pushed the try/navigation-link-variations-filter branch from ab2afaf to dd6857c Compare February 9, 2024 13:22

gaambo requested a review from spacedmonkey as a code owner February 9, 2024 13:22

guard deprecated functions against redeclaration in core

cca0b01

youknowriad reviewed Feb 9, 2024

View reviewed changes

getdave reviewed Feb 9, 2024

View reviewed changes

lib/compat/wordpress-6.5/navigation-block-variations.php Outdated Show resolved Hide resolved

getdave and others added 2 commits February 9, 2024 14:11

Fix typo

573ec7a

fix wrong function names

d1e9bc4

youknowriad merged commit 544b315 into WordPress:trunk Feb 9, 2024
53 of 54 checks passed

github-actions bot added this to the Gutenberg 17.7 milestone Feb 9, 2024

jsnajdr reviewed Feb 16, 2024

View reviewed changes

gaambo mentioned this pull request Feb 16, 2024

Fix wp 6.4/6.3 compat for navigation link variations #59126

Merged

Mamaduka mentioned this pull request Feb 23, 2024

Infinite number of core/navigation-link variations upon many locale switch #59300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation Link: use get_block_type_variations to register variations #58389

Navigation Link: use get_block_type_variations to register variations #58389

gaambo commented Jan 29, 2024 •

edited by getdave

Loading

getdave commented Jan 29, 2024

gaambo commented Jan 29, 2024 •

edited

Loading

gaambo commented Jan 29, 2024

youknowriad commented Jan 29, 2024

gaambo commented Jan 30, 2024

gaambo commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 •

edited

Loading

getdave commented Feb 6, 2024

getdave commented Feb 8, 2024

getdave commented Feb 8, 2024

getdave left a comment

draganescu left a comment

gaambo commented Feb 8, 2024 •

edited

Loading

getdave commented Feb 9, 2024

getdave commented Feb 9, 2024

gaambo commented Feb 9, 2024

youknowriad commented Feb 9, 2024

youknowriad Feb 9, 2024

gaambo Feb 9, 2024

youknowriad Feb 9, 2024

gaambo Feb 9, 2024

youknowriad Feb 9, 2024

getdave commented Feb 9, 2024 •

edited

Loading

gaambo commented Feb 9, 2024 •

edited

Loading

youknowriad commented Feb 9, 2024

youknowriad left a comment

getdave commented Feb 9, 2024

draganescu commented Feb 9, 2024

jsnajdr Feb 16, 2024

gaambo Feb 16, 2024

	await expect(
	listView
	.getByRole( 'gridcell', {
	name: 'Page Link',
	} )
	.filter( {
	hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
	} )
	.getByText( 'Top Level Item 1' )
	).toBeVisible();

Navigation Link: use get_block_type_variations to register variations #58389

Navigation Link: use get_block_type_variations to register variations #58389

Conversation

gaambo commented Jan 29, 2024 • edited by getdave Loading

Why?

How?

Testing Instructions

Automated testing

Manual Testing

Post Type

Taxonomy

Testing unregistering

Post Type

Taxonomy

getdave commented Jan 29, 2024

gaambo commented Jan 29, 2024 • edited Loading

gaambo commented Jan 29, 2024

youknowriad commented Jan 29, 2024

gaambo commented Jan 30, 2024

gaambo commented Feb 5, 2024

github-actions bot commented Feb 5, 2024 • edited Loading

getdave commented Feb 6, 2024

getdave commented Feb 8, 2024

getdave commented Feb 8, 2024

getdave left a comment

Choose a reason for hiding this comment

draganescu left a comment

Choose a reason for hiding this comment

gaambo commented Feb 8, 2024 • edited Loading

getdave commented Feb 9, 2024

getdave commented Feb 9, 2024

gaambo commented Feb 9, 2024

youknowriad commented Feb 9, 2024

youknowriad Feb 9, 2024

Choose a reason for hiding this comment

gaambo Feb 9, 2024

Choose a reason for hiding this comment

youknowriad Feb 9, 2024

Choose a reason for hiding this comment

gaambo Feb 9, 2024

Choose a reason for hiding this comment

youknowriad Feb 9, 2024

Choose a reason for hiding this comment

getdave commented Feb 9, 2024 • edited Loading

gaambo commented Feb 9, 2024 • edited Loading

youknowriad commented Feb 9, 2024

youknowriad left a comment

Choose a reason for hiding this comment

getdave commented Feb 9, 2024

draganescu commented Feb 9, 2024

jsnajdr Feb 16, 2024

Choose a reason for hiding this comment

gaambo Feb 16, 2024

Choose a reason for hiding this comment

gaambo commented Jan 29, 2024 •

edited by getdave

Loading

gaambo commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

gaambo commented Feb 8, 2024 •

edited

Loading

getdave commented Feb 9, 2024 •

edited

Loading

gaambo commented Feb 9, 2024 •

edited

Loading