Skip to content

Fix variations reference issue - Trac 60309#58072

Closed
kt-12 wants to merge 3 commits intoWordPress:trunkfrom
kt-12:fix/trac-60309
Closed

Fix variations reference issue - Trac 60309#58072
kt-12 wants to merge 3 commits intoWordPress:trunkfrom
kt-12:fix/trac-60309

Conversation

@kt-12
Copy link
Copy Markdown
Member

@kt-12 kt-12 commented Jan 22, 2024 •
edited
Loading

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
mukeshpanchal27
mukeshpanchal27 reviewed Jan 22, 2024
View reviewed changes
packages/block-library/src/navigation-link/index.php Outdated
}

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

@mukeshpanchal27 mukeshpanchal27 Jan 22, 2024

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

gutenberg/packages/block-library/src/navigation-link/index.php

Lines 360 to 363 in 2bc59bd

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

@Mamaduka
Copy link
Copy Markdown
Member

Mamaduka commented Jan 22, 2024 •
edited
Loading

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

@t-hamano t-hamano mentioned this pull request Jan 22, 2024
Closed
@spacedmonkey
Copy link
Copy Markdown
Member

spacedmonkey commented Jan 22, 2024

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

@joemcgill joemcgill mentioned this pull request Jan 22, 2024
Merged
joemcgill
joemcgill reviewed Jan 22, 2024
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@joemcgill joemcgill joemcgill left review comments

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

+1 more reviewer

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kt-12 @Mamaduka @spacedmonkey @joemcgill @mukeshpanchal27