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

[gaambo]

What?

This is a different approach to solving #53826. Instead of using a later priority for the init hook (as in #54434), this PR directly hooks into registered_post_type and registered_taxonomy.

Closes #53826

Why?

See #53826 - server side function runs too early to make sure it gets all custom taxonomies/post types by plugins, themes etc..

How?

Variations for all post types and taxonomies registered until register_block_core_navigation_link (at init priority 10) are registered directly in here. For all post types and taxonomies registered after that, actions are added to registered_post_type and registered_taxonomy, which will register variations during post type/taxonomy registration.

We can't use the second approach for all post types/taxonomies, because some of them (eg builtin ones) are registered before the navigation link block is registered, therefore variations can't yet be added.

I'm directly adding the variation on the WP_Block_Type object from WP_Block_Type_Registry, because there's no API for registering variations in PHP (yet).

Testing Instructions

Manual Testing

Post Type

1. 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

1. 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

Automated testing

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

[getdave]

@gaambo Thank you so much for this PR. I will review this week and will attempt to solicit feedback from WP Core contributors.

[getdave]

Initial review. I will also need to manually test this.

I'll wait for others' thoughts (especially @ironprogrammer), but I'm wondering whether these variation functions should be grouped under a static utility class in the same way we did for Navigation Fallbacks.

There have been previous discussions about such an approach as a way of avoiding creation of global functions that then need to be maintained indefinitely.

[getdave]

This is look really good. Thank you.

A few things I noticed.

[getdave]

@ironprogrammer Could you confirm you would be happy with this approach?

[draganescu]

This is a really cool solution @gaambo - we're registering blocks to late for the built in cpt and taxonomies to reigster block variations, and too early for custom cpts and taxonomies to register variations 🤦🏻

I wonder if another option would be to prerender a list of these variations and then register them clientside with wp.blocks.registerBlockVariation?

[gaambo]

@draganescu

I wonder if another option would be to prerender a list of these variations and then register them clientside with wp.blocks.registerBlockVariation?

I tried this approach for a similar issue (#52569 about post term block variations for taxonomies) in this PR #52576

I see the following disadvantages:

• There have to be two additional REST API calls (all post type objects, all taxonomy objects). Maybe we can preload all the data (like the navigation block does/did), but that can be a lot of data on large sites.
• Because of the REST API call, this only gets taxonomies/post types which have "show_in_rest" enabled (show_in_rest is a typical way to prevent a post type from using the block editor. A developer may want to use the classic editor for a post type, but still allow using it for navigation links, I guess).
• I don't know of the performance impacts of subscribe/unsubscribe, but could imagine it's kind of large?
• The _builtin property of taxonomies is not output via REST API. But by default the builtin taxonomies are registered first and therefore output first (and therefore added as a variation first).

Therefore, I went with the other solution and register them on the server side. We have all the information we need (because taxonomies and post types have to be registerd on the server, there are no js-only registration calls) and don't have to offload work to the client.

[draganescu]

Aweosme, thank you for all the details @gaambo 🙏🏻

Yes. I was more leaning on prerendering serverside a big object with all this data and then register these variations in the editor boot up or when the block is init - no rest calls. So still serverside to get all the advantages you mention but without the three callbacks, all of them executed all the time.

Hm, now that I wrote the above could just enabling everything be overwhelming? How could one opt out of not showing their tax or cpt in the nav block? Maybe since today the implementation doesn't work we have not had the chance to be overwhelmed with 40 different variations?

[gaambo]

Thank you for your input @draganescu!

Yes. I was more leaning on prerendering serverside a big object with all this data and then register these variations in the editor boot up or when the block is init - no rest calls. So still serverside to get all the advantages you mention but without the three callbacks, all of them executed all the time.

Yes, that could work. Do you happen to know if there's any other place where such data is passed to the editor and where to put that date? I found that the site editor loads the block categories and (still experimental) the server side registered block defintions. Which is both done via wp_add_inline_script to the wp-blocks. That code runs way after init so all post types are registered already.
I guess we could use that here as well. But maybe you/someone else has a better idea if that is the right way to add server-side generated data to the editor?

Hm, now that I wrote the above could just enabling everything be overwhelming? How could one opt out of not showing their tax or cpt in the nav block? Maybe since today the implementation doesn't work we have not had the chance to be overwhelmed with 40 different variations?

I'm checking on the show_in_nav_menus flag for taxonomies/post types, which is on par with the classic navigation editor. So internal object types will not be used for variations. I think, working with this flag will keep the existing expectations of developers as well as users.

[draganescu]

The more I thought about this the more it made sense so 🤷🏻 . My one question left is why not use the add_action calls at line 397 after the init one, is there a need for them to be in the register_block_core_navigation_link block registration function?

One reason may be that if the reigistration doesnt go through we should have the other hooks, but the registration seems to never exit early and register_block_core_navigation_link_variation will exit early if the block is not registered.

When porting on core these filters would maybe all be in the same spot so it seems all the more a reason to not set them in the registration function of the init callback?

[gaambo]

Thank you, @draganescu .

My one question left is why not use the add_action calls at line 397 after the init one, is there a need for them to be in the register_block_core_navigation_link block registration function?

One reason may be that if the reigistration doesnt go through we should have the other hooks, but the registration seems to never exit early and register_block_core_navigation_link_variation will exit early if the block is not registered.

When porting on core these filters would maybe all be in the same spot so it seems all the more a reason to not set them in the registration function of the init callback?

The reason is stated in the comment directly above the add_action calls in register_block_core_navigation_link:

This needs to happen in this function, because otherwise any post types/taxonomies registered before this block will attempt to register on a block that does not yet exist.

Meaning: Third parties could register a post type or a taxonomy before init priority 10. If we did the add_action calls outside register_block_core_navigation_link, those would be called. But there would not be a navigation block yet registered to which we could add the variation. With the current implemented method, all post types/taxonomies - the ones registered before init priority 10 and the ones after that - will have a variation generated.

Another way would be to do the two add_action calls outside register_block_core_navigation_link, in register_block_core_navigation_link_variation somehow (eg static variable) store all post types/taxonomies that are registered when there is no core/navigation block (yet) and then in register_block_core_navigation_link use that static variable to generate the first variations. But imho that seems worse then doing add_action calls in register_block_core_navigation_link.

[draganescu]

Ugh, I think I read that five times in past reviews 🤦🏻 Sorry for asking the obvious @gaambo

Third parties could register a post type or a taxonomy before init priority 10.

Wouldn't these be available to register_block_core_navigation_link at init priority 10 via get_post_types and get_taxonomies?

The functions register_block_core_navigation_link_taxonomy_variation and register_block_core_navigation_link_post_type_variation exit early if the block doesn't exist.

What am I missing?

[gaambo]

Oh no, you are absolutely right. I completely missed the obvious thing🤦.
You are right, every post type/taxonomy registered before register_block_core_navigation_link, will trigger the hooks but return early because the block is not registered yet. Then when registering the block they are collected.

I changed that and tried to document that as well.

[gaambo]

@draganescu Is there anything for me to do or do we just wait for reviewers?

Should I add e2e tests? I've searched in the repo and found that for example in e2e-tests/plugins/ there are some e2e tests which register post types and taxonomies and then - so I guess - test for their existence in the editor.
I've never written e2e tests and have only limited experience with PHPUnit tests, but be happy to try writing those, if I have some guidance on where they should go.

If this apporach gets merged, I also would suggest to use it for #52569 (missing post terms block variations), which is pretty similar to this.

[draganescu]

@gaambo I saw the tests were not green but did not pay attention that the failing one was optional. Nevertheless, I'll be merging. Would you be willing to open a core ticket and corresponding PR porting this change to core? (that was what the optional test was about, editing PHP changes requires a core porting PR).

[gaambo]

@draganescu Thank you 🥳
I couldn't find any documentation about backporting or what to do exactly (only this about the release process). I guess should just be a trac ticket with the reference to this PR/issue and a PR that puts the PHP file changes into wordpress core. Is that correct?

[draganescu]

I guess should just be a trac ticket with the reference to this PR/issue and a PR that puts the PHP file changes into wordpress core. Is that correct?

Actually, the changes go in the package update since we only updated the block. It's worth opening a ticket, referencing this PR and asking if leaving the code in the block is good enough.

Generally, the PHP updates of blocks are landing in core via package update. But in this case given we add new functions and filters we may want to move these out of the block. Maybe 🤷🏻

[getdave]

✅ I updated the PHP Sync Tracking Issue to note this PR does not require a backport.

[getdave]

I think these two lines maybe causing the breaking PHPUNit tests for the PHP package sync process for WordPress 6.5

WordPress/wordpress-develop#5922

[gaambo]

Maybe after #56952 was merged, we can just use the variations_callback function and don't need to hook into the registered_post_type and registered_taxonomy actions anymore. I can test locally.

But do you have any idea as to why those hooks may cause phpunit timeouts?

[gaambo]

@getdave I've drafted up a PR which uses the now in core available get_block_type_variations filter instead of the hooks mentioned above. This should give us performance benefits, but also hopefully remove the problems with the unit tests you mentioned.

#58389

The PR is just a draft/exploration, because it will only work when that filter is available (which is core 6.5+). So it definitely has some BC pitfalls. Happy for any feedback or if you think, that this PR should just be reverted so gutenberg can successfully be merged into core.

[getdave]

This should give us performance benefits, but also hopefully remove the problems with the unit tests you mentioned.

Lets bring that in. I'll try it manually here.

why those hooks may cause phpunit timeouts?

PHPUnit probably registers/unregisters post types hundreds of times. That could be it? Still only a guess.

The PR is just a draft/exploration, because it will only work when that filter is available (which is core 6.5+). So it definitely has some BC pitfalls.

These Gutenberg changes will be for 6.5 so it should be ok. The only issue is that the Plugin should run with the last x2 versions of WP (I think) so we'd need to gate it based on the availability of the new variations_callback API in Core. Can we do that?