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

Try: Render variation alias blocks #6555

Draft
wants to merge 12 commits into
base: trunk
Choose a base branch
from

Conversation

tjcafferkey
Copy link

Trac ticket:


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.

@ockham
Copy link
Contributor

ockham commented May 16, 2024

As @tjcafferkey mentioned in a DM, variations are still missing a class name on the frontend. E.g. replacing wp:social-link with wp:core/social-link/wordpress in the markup, we get this on the frontend:

image

The reason is that the block is missing the wp-block-social-link class.

@ockham
Copy link
Contributor

ockham commented May 16, 2024

Per WordPress/gutenberg#43743 (comment):

The next layer where we need to decide how to represent aliases is WP_Block instances. This layer is a lot more semantic that the parsed block format; among other things, it validates the block name based on what blocks are registered. For this reason (and others), I think that the WP_Block instance's $name field should reflect the canonical block name, and that we need to introduce additional fields and/or methods to reflect the current variation.

In the vein of that comment, I tried to apply the following patch to this PR:

diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index 960bee9260..9f3a3a14ce 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -128,7 +128,7 @@ class WP_Block {
         */
        public function __construct( $block, $available_context = array(), $registry = null ) {
                $this->parsed_block = $block;
-               $this->name         = $block['blockName'];
+               $this->name         = $this->get_canonical_block_name( $block['blockName'] );
 
                if ( is_null( $registry ) ) {
                        $registry = WP_Block_Type_Registry::get_instance();
@@ -136,8 +136,7 @@ class WP_Block {
 
                $this->registry = $registry;
 
-               $block_name       = $this->get_canonical_block_name( $this->name );
-               $this->block_type = $registry->get_registered( $block_name );
+               $this->block_type = $registry->get_registered( $this->name );
 
                $this->available_context = $available_context;
 

Since this would set $this->name to the canonical block name pretty much at the start of the lifecycle of any WP_Block instance, I thought it would fix the missing classname issue. However, to my surprise, that issue still persists.

This is quite puzzling to me. One potential explanation I could see was that the class name was generated based on the parsed block's blockName field rather than the WP_Block instance's name field, but this code seems to indicate otherwise.

We'll need to investigate this a bit more.

@tjcafferkey
Copy link
Author

We'll need to investigate this a bit more.

This is happening because when we apply block support functionality we pass in the $parsed_block which fetches the matching WP_Block_Type and we haven't yet registered a WP_Block_Type for the variation block (only a WP_Block).

Fetching the WP_Block_Type of the canonical block fixes this issue (480b354), however the WP_Block_Type isn't aware of whether it is a variation of itself or not (because we've not registered it).

We may need to either a) register variations as a WP_Block_Type b) Provide the ability to pass in the variation name to generate class names for.

At the moment I'm leaning towards option B because I don't think we should register variations as WP_Block_Types but instead use the canonical blocks instance since that's what variations are.

…ff off on off off off on on off off off off on off off off off off off off on off off off off on off off on off off off on on off on on off off off on off off on off off on off off on off on off off on off on off off off off on off off off on off off on off off off off off off off off on off on off off on off off off on off off off on off off off off on off on off on off on off off on off on off off on on on off on on off on on on on off off off off on on off off on off off off off off on off off on off off on on off off off off off on off off off off on on off on off off on off off on off on off off off off off off off off off off on on off on off off off arg to appy_block_supports function
@tjcafferkey
Copy link
Author

tjcafferkey commented May 17, 2024

@ockham I don't necessarily think these are the correct solutions but here are two approaches I experimented with to demonstrate the problem

Edit: Keep scrolling

@tjcafferkey
Copy link
Author

tjcafferkey commented May 20, 2024

In this PR currently when we retrieve the WP_Block_Type we do so by inferring the canonical block name, looking that up in the registry and then setting ->name as the alias block name as seen here.

Whereas in the WP_Block instance we again infer the canonical block name of an alias, and set that its ->name as the canonical name, as seen here.

What this means is that we will have a WP_Block instance and a WP_Block_Type instance of the same alias block. The former with a name property of namespace/block-name and the latter with a name property of namespace/block-name/alias-name

This is a mismatch I believe we should fix.

@tjcafferkey
Copy link
Author

tjcafferkey commented May 20, 2024

Following on from my previous comment, it feels as though we need to always return the instance of the canonical block. Here are two other approaches for this same problem.

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.

2 participants