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

GlobalStyles: extract sanitize method #30809

Merged
merged 2 commits into from
Apr 15, 2021
Merged

GlobalStyles: extract sanitize method #30809

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae3faa6
Extract sanitize method
oandregal Apr 14, 2021
3959695
Prevent against accessing a non-existing index
oandregal Apr 14, 2021
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
257 changes: 109 additions & 148 deletions lib/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,99 +56,76 @@ class WP_Theme_JSON {
*/
const ROOT_BLOCK_SELECTOR = ':root';

/**
* Data schema of each block within a theme.json.
*
* Example:
*
* {
* 'block-one': {
* 'styles': {
* 'color': {
* 'background': 'color'
* }
* },
* 'settings': {
* 'color': {
* 'custom': true
* }
* }
* },
* 'block-two': {
* 'styles': {
* 'color': {
* 'link': 'color'
* }
* }
* }
* }
*/
const SCHEMA = array(
'customTemplates' => null,
'templateParts' => null,
'styles' => array(
'border' => array(
'radius' => null,
'color' => null,
'style' => null,
'width' => null,
),
'color' => array(
'background' => null,
'gradient' => null,
'link' => null,
'text' => null,
),
'spacing' => array(
'padding' => array(
'top' => null,
'right' => null,
'bottom' => null,
'left' => null,
),
),
'typography' => array(
'fontFamily' => null,
'fontSize' => null,
'fontStyle' => null,
'fontWeight' => null,
'lineHeight' => null,
'textDecoration' => null,
'textTransform' => null,
),
const VALID_TOP_LEVEL_KEYS = array(
'customTemplates',
'templateParts',
'styles',
'settings',
);

const VALID_STYLES = array(
'border' => array(
'radius' => null,
'color' => null,
'style' => null,
'width' => null,
),
'settings' => array(
'border' => array(
'customRadius' => null,
'customColor' => null,
'customStyle' => null,
'customWidth' => null,
),
'color' => array(
'custom' => null,
'customGradient' => null,
'gradients' => null,
'link' => null,
'palette' => null,
),
'spacing' => array(
'customPadding' => null,
'units' => null,
),
'typography' => array(
'customFontSize' => null,
'customLineHeight' => null,
'dropCap' => null,
'fontFamilies' => null,
'fontSizes' => null,
'customFontStyle' => null,
'customFontWeight' => null,
'customTextDecorations' => null,
'customTextTransforms' => null,
'color' => array(
'background' => null,
'gradient' => null,
'link' => null,
'text' => null,
),
'spacing' => array(
'padding' => array(
'top' => null,
'right' => null,
'bottom' => null,
'left' => null,
),
'custom' => null,
'layout' => null,
),
'typography' => array(
'fontFamily' => null,
'fontSize' => null,
'fontStyle' => null,
'fontWeight' => null,
'lineHeight' => null,
'textDecoration' => null,
'textTransform' => null,
),
);

const VALID_SETTINGS = array(
'border' => array(
'customRadius' => null,
'customColor' => null,
'customStyle' => null,
'customWidth' => null,
),
'color' => array(
'custom' => null,
'customGradient' => null,
'gradients' => null,
'link' => null,
'palette' => null,
),
'spacing' => array(
'customPadding' => null,
'units' => null,
),
'typography' => array(
'customFontSize' => null,
'customLineHeight' => null,
'dropCap' => null,
'fontFamilies' => null,
'fontSizes' => null,
'customFontStyle' => null,
'customFontWeight' => null,
'customTextDecorations' => null,
'customTextTransforms' => null,
),
'custom' => null,
'layout' => null,
);

/**
Expand Down Expand Up @@ -295,75 +272,53 @@ class WP_Theme_JSON {
* @param array $theme_json A structure that follows the theme.json schema.
*/
public function __construct( $theme_json = array() ) {
$this->theme_json = array();

if ( ! is_array( $theme_json ) ) {
return;
}

// Remove top-level keys that aren't present in the schema.
$this->theme_json = array_intersect_key( $theme_json, self::SCHEMA );
$valid_block_names = array_keys( self::get_blocks_metadata() );
$this->theme_json = self::sanitize( $theme_json, $valid_block_names );
}

$block_metadata = self::get_blocks_metadata();
foreach ( array( 'settings', 'styles' ) as $subtree ) {
// Remove settings & styles subtrees if they aren't arrays.
if ( isset( $this->theme_json[ $subtree ] ) && ! is_array( $this->theme_json[ $subtree ] ) ) {
unset( $this->theme_json[ $subtree ] );
}
/**
* Sanitizes the input according to the schemas.
*
* @param array $input Structure to sanitize.
* @param array $valid_block_names List of valid block names.
*
* @return array The sanitized output.
*/
private static function sanitize( $input, $valid_block_names ) {
$output = array();

// Remove block selectors subtrees declared within settings & styles if that aren't registered.
if ( isset( $this->theme_json[ $subtree ] ) ) {
$this->theme_json[ $subtree ] = array_intersect_key( $this->theme_json[ $subtree ], $block_metadata );
}
if ( ! is_array( $input ) ) {
return $output;
}

foreach ( $block_metadata as $block_selector => $metadata ) {
if ( isset( $this->theme_json['styles'][ $block_selector ] ) ) {
// Remove the block selector subtree if it's not an array.
if ( ! is_array( $this->theme_json['styles'][ $block_selector ] ) ) {
unset( $this->theme_json['styles'][ $block_selector ] );
continue;
}
$output = array_intersect_key( $input, array_flip( self::VALID_TOP_LEVEL_KEYS ) );

$styles_schema = self::SCHEMA['styles'];
$this->theme_json['styles'][ $block_selector ] = self::remove_keys_not_in_schema(
$this->theme_json['styles'][ $block_selector ],
$styles_schema
);
$schema = array();
foreach ( $valid_block_names as $block_name ) {
$schema['styles'][ $block_name ] = self::VALID_STYLES;
$schema['settings'][ $block_name ] = self::VALID_SETTINGS;
}
Comment on lines +297 to +300
Copy link
Contributor

Choose a reason for hiding this comment

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

For each $block_name, we're creating an identical copy of self::VALID_STYLES and self::VALID_SETTINGS, respectively. Would it be worth considering having just

Suggested change
foreach ( $valid_block_names as $block_name ) {
$schema['styles'][ $block_name ] = self::VALID_STYLES;
$schema['settings'][ $block_name ] = self::VALID_SETTINGS;
}
$schema['styles'] = self::VALID_STYLES;
$schema['settings'] = self::VALID_SETTINGS;

and changing remove_keys_not_in_schema() accordingly?

Copy link
Member Author

@oandregal oandregal Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

One of the struggles of this class has been how to make it adaptable to theme.json shape changes. The idea I explored in the latest rounds is to separate the logic that iterates over the tree from the logic to do something with a particular set of nodes. A visitor pattern of sorts. The hope is that, by centralizing the iteration logic, changes can be made easily in the future.

For example, in this case, remove_keys_not_in_schema is agnostic about the shape of theme.json, it just cares about removing nodes in the given tree that don't follow the given schema. The same logic applies to some other methods in this class after the latest changes landed in master: most of them become "visitors" of nodes, they know how to operate on a node or set of nodes (take styles out of them, process presets, etc) but are theme.json-shape agnostic. Then there're a couple of methods that are focused on building the paths to iterate over, so those are the only ones that need updating when theme.json changes.


// Remove the block selector subtree if it is empty after having processed it.
if ( empty( $this->theme_json['styles'][ $block_selector ] ) ) {
unset( $this->theme_json['styles'][ $block_selector ] );
}
foreach ( array( 'styles', 'settings' ) as $subtree ) {
if ( ! isset( $input[ $subtree ] ) ) {
continue;
}

if ( isset( $this->theme_json['settings'][ $block_selector ] ) ) {
// Remove the block selector subtree if it's not an array.
if ( ! is_array( $this->theme_json['settings'][ $block_selector ] ) ) {
unset( $this->theme_json['settings'][ $block_selector ] );
continue;
}

// Remove the properties that aren't present in the schema.
$this->theme_json['settings'][ $block_selector ] = self::remove_keys_not_in_schema(
$this->theme_json['settings'][ $block_selector ],
self::SCHEMA['settings']
);

// Remove the block selector subtree if it is empty after having processed it.
if ( empty( $this->theme_json['settings'][ $block_selector ] ) ) {
unset( $this->theme_json['settings'][ $block_selector ] );
}
if ( ! is_array( $input[ $subtree ] ) ) {
unset( $output[ $subtree ] );
continue;
}
}

// Remove the settings & styles subtrees if they're empty after having processed them.
foreach ( array( 'settings', 'styles' ) as $subtree ) {
if ( empty( $this->theme_json[ $subtree ] ) ) {
unset( $this->theme_json[ $subtree ] );
$result = self::remove_keys_not_in_schema( $input[ $subtree ], $schema[ $subtree ] );

if ( empty( $result ) ) {
unset( $output[ $subtree ] );
} else {
$output[ $subtree ] = $result;
}
}

return $output;
}

/**
Expand Down Expand Up @@ -485,12 +440,18 @@ private static function remove_keys_not_in_schema( $tree, $schema ) {
$tree = array_intersect_key( $tree, $schema );

foreach ( $schema as $key => $data ) {
if ( is_array( $schema[ $key ] ) && isset( $tree[ $key ] ) ) {
if ( ! isset( $tree[ $key ] ) ) {
continue;
}

if ( is_array( $schema[ $key ] ) && is_array( $tree[ $key ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

isset accounts also for the case when the $key in the $tree is not set. Is it safe to replace it with is_array?

Copy link
Member Author

@oandregal oandregal Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's not, good catch! Pushed another check above as well.

Would you know how to make unit tests fail if there are warnings? Somehow they're not reported for me when running npm run test-unit-php.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that the env that runs phpunit has error reporting disabled?

$tree[ $key ] = self::remove_keys_not_in_schema( $tree[ $key ], $schema[ $key ] );

if ( empty( $tree[ $key ] ) ) {
unset( $tree[ $key ] );
}
} elseif ( is_array( $schema[ $key ] ) && ! is_array( $tree[ $key ] ) ) {
unset( $tree[ $key ] );
}
}

Expand Down Expand Up @@ -630,7 +591,7 @@ private static function compute_style_properties( $declarations, $styles ) {
foreach ( self::PROPERTIES_METADATA as $name => $metadata ) {
// Some properties can be shorthand properties, meaning that
// they contain multiple values instead of a single one.
// An example of this is the padding property, see self::SCHEMA.
// An example of this is the padding property.
if ( self::has_properties( $metadata ) ) {
foreach ( $metadata['properties'] as $property ) {
$properties[] = array(
Expand Down