Skip to content

Conversation

@tellthemachines
Copy link
Contributor

…tion

Trac ticket: https://core.trac.wordpress.org/ticket/64624

Syncs changes from WordPress/gutenberg#75360.

Use of AI Tools


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@tellthemachines tellthemachines self-assigned this Feb 12, 2026
@github-actions
Copy link

github-actions bot commented Feb 12, 2026

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props isabel_brison, mukesh27, westonruter, andrewserong.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Comment on lines +15 to +17
* @param array $registered_styles Currently registered block styles.
*
* @return string|null The name of the first registered variation, or null if none found.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param array $registered_styles Currently registered block styles.
*
* @return string|null The name of the first registered variation, or null if none found.
* @param array $registered_styles Currently registered block styles.
* @return string|null The name of the first registered variation, or null if none found.

Per document standard https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#1-functions-class-methods

)
);

// WP_Theme_JSON_Resolver::clean_cached_data();
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this code here, or was it added by mistake?

Copy link
Member

Choose a reason for hiding this comment

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

It was added in tear_down method so better to remove it from test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, well spotted! I was experimenting and forgot to remove it 😅

* Tests that block style variations with blockGap values are applied to layout styles.
*
* @ticket 64624
* @covers ::wp_render_layout_support_flag
Copy link
Member

Choose a reason for hiding this comment

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

Does this test also cover wp_get_variation_name_from_class()? Then it should be added here too.

If not, another test should be added for it.

'type' => 'grid',
'columnCount' => 3,
'minimumColumnWidth' => '12rem',

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 795 to 796
// Clean up.
remove_theme_support( 'appearance-tools' );
Copy link
Member

Choose a reason for hiding this comment

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

This should go in tear_down, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. I'm seeing adding/removing of theme supports per test in most places, and the support is added inside the test so I thought I'd remove it there too. Though for this one I guess it won't make any difference practically, because it's removed at the end of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually thinking better about it, we shouldn't need appearance-tools support here at all!

Comment on lines +14 to +15
* @param string $class_name CSS class string for a block.
* @param array $registered_styles Currently registered block styles.
Copy link
Member

Choose a reason for hiding this comment

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

Can the array param be more specific? For example (if this is accurate):

Suggested change
* @param string $class_name CSS class string for a block.
* @param array $registered_styles Currently registered block styles.
* @param string $class_name CSS class string for a block.
* @param array<string, array<string, mixed>> $registered_styles Currently registered block styles.

Comment on lines +26 to +31
$prefix = 'is-style-';
$len = strlen( $prefix );

foreach ( explode( ' ', $class_name ) as $class ) {
if ( str_starts_with( $class, $prefix ) ) {
$variation = substr( $class, $len );
Copy link
Member

Choose a reason for hiding this comment

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

Best to not abbreviate the variable names:

Suggested change
$prefix = 'is-style-';
$len = strlen( $prefix );
foreach ( explode( ' ', $class_name ) as $class ) {
if ( str_starts_with( $class, $prefix ) ) {
$variation = substr( $class, $len );
$prefix = 'is-style-';
$length = strlen( $prefix );
foreach ( explode( ' ', $class_name ) as $class ) {
if ( str_starts_with( $class, $prefix ) ) {
$variation = substr( $class, $length );

Copy link
Member

Choose a reason for hiding this comment

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

+1

*
* @return string|null The name of the first registered variation, or null if none found.
*/
function wp_get_variation_name_from_class( $class_name, $registered_styles = array() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function wp_get_variation_name_from_class( $class_name, $registered_styles = array() ) {
function wp_get_variation_name_from_class( string $class_name, $registered_styles = array() ): ?string {

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Just a couple of thoughts re: the new utility function (sorry I didn't catch this in the Gutenberg PR!):

  • Would wp_get_variation_name_from_class be better placed in block-style-variations.php?
  • There's an existing function wp_get_block_style_variation_name_from_class that accepts only a single param (class name). Would it be worth it to combine the logic into that existing function, with the optional second param? I.e. if the second param is empty, simply check against the classname, but if it's given a list of registered variations, then iterate over the list to find from the actual list of variations?
  • Alternately, still have this be a separate function, but give it consistent naming with the existing utility function, and co-locate it beside that function

Hope that all made sense, it's a useful function, though, and sounds like a good addition overall to me 👍

@tellthemachines tellthemachines force-pushed the fix/grid-with-style-vars branch from 6d3822f to 54f1554 Compare February 13, 2026 04:42
@tellthemachines
Copy link
Contributor Author

There's an existing function wp_get_block_style_variation_name_from_class that accepts only a single param (class name). Would it be worth it to combine the logic into that existing functions, with the optional second param?

Yeah that's a good question. Also, it looks like wp_render_block_style_variation_support_styles which uses that helper function is rendering styles without checking whether they're registered or not. Should layout do that too?

Otherwise, I think we could consolidate the two functions into one.

@andrewserong
Copy link
Contributor

Also, it looks like wp_render_block_style_variation_support_styles which uses that helper function is rendering styles without checking whether they're registered or not. Should layout do that too?

Oh, good point. Yeah, just as in wp_render_block_style_variation_support_styles, it looks like you're only using the $variation_name to access a key in the global styles data, so if there are any unregistered classnames that are a match, it'd return null anyway. So yeah, given that function works that way, it sounds like you could use the same approach here as the style engine will sanitize output, anyway.

Still, I don't mind the additional check (and the idea of adding it into the existing function via a second param) as it does give an additional polish of making sure that we're only looking up values that can exist.

Up to you, I think, if you prefer the clarity or "correctness" of the additional check, or the simplicity of re-using the existing function. (Personally I think it's a valid choice either way).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants