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

Fix variations reference issue - Trac 60309 #58072

Closed
wants to merge 3 commits into from

Conversation

kt-12
Copy link
Member

@kt-12 kt-12 commented Jan 22, 2024

What?

Fixes
WordPress/wordpress-develop#5718 (comment)

Detailed issue in the description - https://core.trac.wordpress.org/ticket/60309#ticket

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention labels Jan 22, 2024
@@ -338,7 +338,7 @@ function block_core_navigation_link_register_variation( $variation ) {
return;
}

$navigation_block_type->variations[] = $variation;
$navigation_block_type->variations = array_merge( $navigation_block_type->variations, $variation );
Copy link
Member

Choose a reason for hiding this comment

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

This will not fixed the issue as

if ( $variation['name'] === $name ) {
unset( $navigation_block_type->variations[ $i ] );
break;
}
they check for variation name and remove it.

@Mamaduka
Copy link
Member

Mamaduka commented Jan 22, 2024

I think we can also revert changes introduced in #56100. The hook registration order is not important now that variations are lazily evaluated.

@spacedmonkey
Copy link
Member

@kt-12 Idea.

We add new function for adding / removing variants. wp_add_block_variation( $variation ) and wp_remove_block_variation( $name ). In core these functions can be implemented using the get_block_type_variations to add and remove elements from that array. In gutenberg these functions can be implement by detecting if the get_variations() method exists on block-type class. If it does, use the filter / overwise, use the variations property directly.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

The PHPUnit failures have been fixed via #58090 (which is the same approach being suggested here).

If we're going to address directly modifying registered variations as a valid back-compat concern, those PHPUnit tests should be added to the core tests suite that covers the main API for the block type registry, rather than only having tests in this repo for that behavior.

@kt-12 kt-12 closed this Jan 22, 2024
@kt-12 kt-12 reopened this Jan 22, 2024
@kt-12 kt-12 closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants