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.

Sign up for GitHub

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

Add blocks to the list of valid origins for theme.json #44363

Merged
merged 4 commits into from
Sep 22, 2022
Merged

Add blocks to the list of valid origins for theme.json #44363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d33d143
Fix name
oandregal Sep 22, 2022
1c31f8a
Remove comment
oandregal Sep 22, 2022
688ec41
Add missing blocks origin
oandregal Sep 22, 2022
1e8e56a
Do not use blocks origin data for variables either
oandregal Sep 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@ class WP_Theme_JSON_6_1 extends WP_Theme_JSON_6_0 {
'button' => array( ':hover', ':focus', ':active', ':visited' ),
);

/**
* The sources of data this object can represent.
*
* @var string[]
*/
const VALID_ORIGINS = array(
'default',
'blocks',
Copy link
Member Author

Choose a reason for hiding this comment

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

blocks was missing

'theme',
'custom',
);

/**
* Metadata for style properties.
*
Expand Down
18 changes: 16 additions & 2 deletions lib/compat/wordpress-6.1/get-global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,22 @@ function gutenberg_get_global_stylesheet( $types = array() ) {

/*
* If variables are part of the stylesheet,
* we add them for all origins (default, theme, user).
* we add them.
*
* This is so themes without a theme.json still work as before 5.9:
* they can override the default presets.
* See https://core.trac.wordpress.org/ticket/54782
*/
$styles_variables = '';
if ( in_array( 'variables', $types, true ) ) {
$styles_variables = $tree->get_stylesheet( array( 'variables' ) );
/*
* We only use the default, theme, and custom origins.
* This is because styles for blocks origin are added
* at a later phase (render cycle) so we only render the ones in use.
Copy link
Member

Choose a reason for hiding this comment

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

Given that the styles are output in other parts of code, isn't there the risk that priority 'default', 'block, 'theme', and 'custom' is not respected? How do we ensure this order?

Copy link
Member Author

@oandregal oandregal Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've looked for every usage of this function (including core) and all instances have been updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me expand this.

The $origins only affect presets, they do not affect the styles in the styles key. Because the blocks origin cannot declare any preset, not providing the origins (hence using all of them, including blocks) would not matter in practice. Though I still think it is good practice to be explicit. Maybe, in the future, blocks can declare presets by themselves.

Note that a filter gutenberg_theme_json_get_style_nodes was introduced to remove the styles entirely from the global styles stylesheet and process them separately.

* @see wp_add_global_styles_for_blocks
*/
$origins = array( 'default', 'theme', 'custom' );
$styles_variables = $tree->get_stylesheet( array( 'variables' ), $origins );
$types = array_diff( $types, array( 'variables' ) );
}

Expand All @@ -111,6 +119,12 @@ function gutenberg_get_global_stylesheet( $types = array() ) {
*/
$styles_rest = '';
if ( ! empty( $types ) ) {
/*
* We only use the default, theme, and custom origins.
* This is because styles for blocks origin are added
* at a later phase (render cycle) so we only render the ones in use.
* @see wp_add_global_styles_for_blocks
*/
$origins = array( 'default', 'theme', 'custom' );
if ( ! $supports_theme_json ) {
$origins = array( 'default' );
Expand Down
6 changes: 2 additions & 4 deletions lib/experimental/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,10 @@ public static function get_block_data() {
*
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'theme_json_blocks', new WP_Theme_JSON_Data_Gutenberg( $config, 'core' ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this name here does not have any effect because it was not being used anywhere. It has started to be used when it was added to the VALID_ORIGINS list.

Copy link
Member Author

@oandregal oandregal Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

By using an unregistered origin this is what was happening:

  • The WP_Theme_JSON_Resolver loaded the blocks origin (named core).
  • In the WP_Theme_JSON constructor, because it was not one of the origins registered it'll be changed to theme.
  • The WP_Theme_JSON_Resolved loaded then the theme origin, updating any instance.

This was working as expected. But only by coincidence. If the blocks origin was able to define presets this won't work. The reason is it'll work this way:

  • the presets of the blocks origin would be stored in the theme key
  • when the actual theme origin was loaded, its presets would be also loaded in the theme key, hence, overriding the previous blocks' presets that should have been stored elsewhere.

Essentially, this is the sort of tricky dormant bug waiting to happen.

$theme_json = apply_filters( 'theme_json_blocks', new WP_Theme_JSON_Data_Gutenberg( $config, 'blocks' ) );
$config = $theme_json->get_data();

// Core here means it's the lower level part of the styles chain.
// It can be a core or a third-party block.
return new WP_Theme_JSON_Gutenberg( $config, 'core' );
return new WP_Theme_JSON_Gutenberg( $config, 'blocks' );
}

/**
Expand Down