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

Better navigation link variations for post types / taxonomies #56100

Merged

Conversation

gaambo
Copy link
Contributor

@gaambo gaambo commented Nov 14, 2023

What?

This is a follow up to #54801
It adds the following things:

  • Adds handling of unregistering post types / taxonomies and their variations.
  • Adds unit tests for private post types / taxonomies (those with show_in_nav_menus set to false)
  • Fixes some code documentation
  • Fixes: Renames functions to be in line with the naming conventions for block php files and also consistency in this file itself

Since the original PR was just merged into trunk and not released yet, I think renaming those functions is not a breaking change.

Why?

#54801 worked, but I missed some minor errors in code documentation and function naming.
Also @obache pointed out that unregistering of post types/taxonomies was not handled.

How?

Similar to registering the variations, it hooks into unregistered_post_type and unregistered_taxonomy to unregister the variations. Since there is no server-side API for (un)registering variations, this is done directly by searching through the variations array of the registered block type - not ideal, but I see no other way atm.

Testing Instructions

Testing unregistering

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. 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);
  1. Go into site-editor and add a navigation block
  2. try to search for a block named like the custom taxonomy (eg "Product Link").
  3. Block variation should not be 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.
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);
  1. Go into site-editor and add a navigation block
  2. try to search for a block named like the custom taxonomy (eg "Course Link").
  3. Block variation should not be available

Automated testing

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

Testing Instructions for Keyboard

Screenshots or screencast

@gaambo
Copy link
Contributor Author

gaambo commented Nov 14, 2023

@draganescu This is a follow up to #54801 which fixes some code documentation but also handles post-type/taxonomy unregistration. Maybe you could have a look at this as well (sorry for spamming you 😅).
I'd be happy to hear your opinion on how the unregistering is handled. Since there's no server-side API for block variations, I think it's not ideal. But I think the other way would be to just send all registered post types/taxonomies to the editor (as suggested by you here).

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Good follow-up. Given how we add the variations this seems to be the logical way to remove them 👍🏻

@draganescu draganescu enabled auto-merge (squash) December 1, 2023 11:28
@draganescu draganescu enabled auto-merge (squash) January 11, 2024 17:34
@draganescu
Copy link
Contributor

@gaambo can you rebase and push this again, maybe we convince the tests to be green. If after doing this (I can't push to your repo) the auto merge still doesnt kick in I'll manually merge it.

auto-merge was automatically disabled January 11, 2024 19:58

Head branch was pushed to by a user without write access

@gaambo gaambo force-pushed the 54801-follow-up-navigation-link-variations branch from 58fa98d to bf6cc96 Compare January 11, 2024 19:58
@gaambo
Copy link
Contributor Author

gaambo commented Jan 11, 2024

@draganescu thank you 🙏 I just rebased and pushed.
I don't know why the JS unit test now fails - since the PR only contains PHP changes 😅 I also can't run any tests locally since npm run build triggers a FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory atm in my local dev environment. Need to look into that seperately.

@draganescu draganescu merged commit 295ebf5 into WordPress:trunk Jan 12, 2024
51 of 53 checks passed
@draganescu
Copy link
Contributor

Thank you @gaambo I merged the PR.

@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

@youknowriad this PR changes phpunit/blocks. Unless I'm wrong I can't see these types of testing being backported to WP Core and thus we shouldn't need any backports here. Am I right?

@youknowriad
Copy link
Contributor

For me if the code is maintained in Gutenberg, the test should be as well and this is the case for the block library code so yeah, no need for backport.

@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

Added phpunit/blocks to exclusions in #58064

@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

@draganescu This PR will not require a manual backport. Thank you.

@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

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

@jonnynews
Copy link
Contributor

Changing the function name of register_block_core_navigation_link_variation is a breaking change. This should be reverted ASAP. @draganescu @getdave

@gaambo
Copy link
Contributor Author

gaambo commented Jan 22, 2024

I renamed the function to be more in line with the newly added block_core_navigation_link_unregister_variation from this PR.
The function was just added in the original PR of this (#54801) which was merged for release 17.1. - so it wasn't in any Core version yet and I don't find any plugins or themes using it.
But I don't know what the rule about backwards-compatibility for such functions in Gutenberg is. Theoretically the function is not really meant for public use, but I didn't document that - so yeah, I guess it can be seen as public "api"?

@draganescu
Copy link
Contributor

@jonnynews I think what @gaambo explained is good, are we wrong? The function was barely added in a previous PR.

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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants