Skip to content

#53826 register variations in a later init hook#54434

Closed
gaambo wants to merge 2 commits intoWordPress:trunkfrom
gaambo:try/navigation-link-block-cpt-variations-1
Closed

#53826 register variations in a later init hook#54434
gaambo wants to merge 2 commits intoWordPress:trunkfrom
gaambo:try/navigation-link-block-cpt-variations-1

Conversation

@gaambo
Copy link
Copy Markdown
Contributor

@gaambo gaambo commented Sep 13, 2023

What?

This is a first try at fixing #53826.
I try to create the variations of the navigation link block for all taxonomies and post types in a seperate function hooked to init on priority 1000.

Why?

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

How?

I just chose 1000 because I'd assume then most other plugins have registered their post types. I've seen plugins use 99, 100 etc - but I guess 1000 should be save.
I'm using a seperate function and directly modify the registered WP_Block_Type instance. I'm not sure if that is really an intended way, but the properties of the class are public - so I guessed "yes".

Alternative solutions are described in the issue.

Testing Instructions

Custom 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
        )
    );
}, 9);

Important: Set the item_link label or the block variation is called Post Link.
Important: change add_action priority to something lower than 10 (eg 9)
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

Testing Instructions for Keyboard

Screenshots or screencast

@getdave getdave added [Type] Bug An existing feature does not function as intended [Block] Navigation Link Affects the Navigation Link Block labels Sep 13, 2023
@gaambo gaambo mentioned this pull request Sep 18, 2023
Closed
@getdave
Copy link
Copy Markdown
Contributor

getdave commented Sep 22, 2023

I've reached out to some Core folks for advice on altering the priority here.

@getdave
Copy link
Copy Markdown
Contributor

getdave commented Sep 22, 2023

I feel that we should probably add a PHPUnit test for this as well. Register some CPTs and Taxonomies and then see if the Nav block variations get registered.

@gaambo
Copy link
Copy Markdown
Contributor Author

gaambo commented Sep 22, 2023

@getdave

I feel that we should probably add a PHPUnit test for this as well. Register some CPTs and Taxonomies and then see if the Nav block variations get registered.

Would that be just PHPUnit tests which check the existence of the variations (e.g. shortly before loading the editor), or should they be 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.

getdave
getdave reviewed Sep 25, 2023
View reviewed changes
packages/block-library/src/navigation-link/index.php
$navigation_block_type->variations = array_merge( $navigation_block_type->variations, $built_ins, $variations );
}
add_action( 'init', 'register_block_core_navigation_link' );
add_action( 'init', 'register_block_core_navigation_link_variations', 1000 );
Copy link
Copy Markdown
Contributor

@getdave getdave Sep 25, 2023

Choose a reason for hiding this comment

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

Speaking away from Github with @ironprogrammer we discussed avoiding using 1000 here. This is because - as you noted - we cannot assume extenders will have done their work by this point.

Another option might be moving this to a later action hook?

@ironprogrammer may be able to advise on a suitable candidate but you could experiment in the meantime to see what works 🙏

Copy link
Copy Markdown
Contributor Author

@gaambo gaambo Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking, that maybe we could run the initial registration on 10 like it is in core atm but additionally add an action hook to registered_post_type and then add custom variations in that callback. This way we can be sure, to catch all post types that will be registered.

@getdave
Copy link
Copy Markdown
Contributor

getdave commented Sep 25, 2023 •
edited
Loading

Would that be just PHPUnit tests which check the existence of the variations (e.g. shortly before loading the editor), or should they be e2e tests?

I would opt for PHPUNit tests which live in the phpunit directory.. Create a new file under blocks/ directory called something like block-navigation-link-variations-test.php.

Then for the test itself I'm thinking something like:

  • register some post types and taxonomies (being sure to de-register them after the tests)
  • get the core/navigation-link block from the block registry. Maybe something like this?
$registry = WP_Block_Type_Registry::get_instance();
$nav_link_block = $registry->get_registered( 'core/navigation-link` );

To run the unit tests from the Gutenberg repo you can use wp-env if you have that running. It requires Docker.

npm run wp-env start
npm run test:unit:php:base -- --filter YOUR_TEST_CLASS_NAME

@gaambo gaambo mentioned this pull request Sep 25, 2023
Merged
@gaambo
Copy link
Copy Markdown
Contributor Author

gaambo commented Sep 25, 2023

@getdave I've added a second PR with a different approach (as outlined in my comment above) and also adding PHPUnit tests - which run successfully.
I think the method in the second PR is better, safer and also handles taxonomies. So we could close this PR.
#54801

@gaambo
Copy link
Copy Markdown
Contributor Author

gaambo commented Nov 10, 2023

#54801 seems to be a better approach, therefore closing this PR.

@gaambo gaambo closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@getdave getdave getdave left review comments

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@anton-vlasenko anton-vlasenko Awaiting requested review from anton-vlasenko

@ironprogrammer ironprogrammer Awaiting requested review from ironprogrammer

Assignees

@gaambo gaambo

Labels

[Block] Navigation Link Affects the Navigation Link Block [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gaambo @getdave