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

Navigation Link Block: Variations for post types may not contain all post types #53826

Closed
gaambo opened this issue Aug 19, 2023 · 5 comments · Fixed by #54801
Closed

Navigation Link Block: Variations for post types may not contain all post types #53826

gaambo opened this issue Aug 19, 2023 · 5 comments · Fixed by #54801
Labels
[Block] Navigation Link Affects the Navigation Link Block [Feature] Navigation Menus Any issue relating to Navigation Menus Needs Testing Needs further testing to be confirmed. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended

Comments

@gaambo
Copy link
Contributor

gaambo commented Aug 19, 2023

Description

Similar to #52569 the navigiation link block creates a variation for all post types. But the hook to check for public post types (especially those with show_in_nav_menus set to true) is run on init with a default priority of 10.
It's safe to asume, that this will run before many plugins will have registered their custom post types, because the core init hook is added earlier and 10 is the default priority (so many developers will register it on 10 as well).

File:

add_action( 'init', 'register_block_core_navigation_link' );

Proposed solution:

  1. Run the complete register_block_core_navigation_link on a priority later than 10 (eg 100)
  2. Split the function and add variations for all public post types in the editor in JavaScript (performance for fetching all post types?)
  3. Split the function and add variations add a later point via PHP (not possible because server side version to register block variations is missing)

All other core blocks are registered on priority 10, so I think it would be a bit odd to run the register function for only this block on a later priority. Also of course we can't be 100% sure if on priority 100 all custom post types are already registered (since developers also can use later priorities). So I guess I actually favor solution 2. Is there precedent for any solution like this?

See also discussion in #52569

My attention was brought to this bug by @kyleakelly@cosocial.ca.

Step-by-step reproduction instructions

Negative test:

  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.
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 not available, only the default "Post Link" and "Pages Link" will be availab.e

Positive test:

  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

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 5.2

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jordesign jordesign added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Block] Navigation Link Affects the Navigation Link Block [Feature] Navigation Menus Any issue relating to Navigation Menus labels Aug 20, 2023
@getdave
Copy link
Contributor

getdave commented Sep 13, 2023

As mentioned in the recentl Core Editor chat (sign up required) I would be happy to support @gaambo with contributing PRs to resolve these issues.

@gaambo
Copy link
Contributor Author

gaambo commented Sep 13, 2023

@getdave I created a PR. Looking forward to your feedback.

I went with the first solution, which registers the variations on the server side in an additional init hooked function. It runs on priority 1000.
I did not want to move the registering of the block to such a late priority. Therefore I worked around the missing register_block_variation API in PHP by directly modifying the WP_Block_Type instance. The properties are public, so I thought I'll just go with it. Allthough that means, that those variations are not filterable in all the hook normally running in register_block_type_from_metadata. That would require moving the complete block registration to the later priority.
What do you think? If we decide on a way to go, we can probably to the same for #52569

Also, I'm not sure how to add e2e tests - never written something like that and looking at the others I'm not sure how to add the custom post type into the snapshot.

@github-actions
Copy link

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Oct 14, 2023
@getdave
Copy link
Contributor

getdave commented Nov 13, 2023

@gaambo Thank you for all your work on this. It's a great addition to WordPress.

@gaambo
Copy link
Contributor Author

gaambo commented Nov 13, 2023

@gaambo Thank you for all your work on this. It's a great addition to WordPress.

@getdave Thank you very much for helping me get there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Link Affects the Navigation Link Block [Feature] Navigation Menus Any issue relating to Navigation Menus Needs Testing Needs further testing to be confirmed. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants