GlobalStyles: extract sanitize method #30809
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
added
[Type] Code Quality
|
Size Change: 0 B Total Size: 1.43 MB ℹ️ View Unchanged
|
gziolo
reviewed
Apr 14, 2021
gziolo
reviewed
Apr 14, 2021
| @@ -485,12 +436,14 @@ 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 ( is_array( $schema[ $key ] ) && is_array( $tree[ $key ] ) ) { | |||
There was a problem hiding this comment.
isset accounts also for the case when the $key in the $tree is not set. Is it safe to replace it with is_array?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is it possible that the env that runs phpunit has error reporting disabled?
oandregal
force-pushed
the
extract/sanitize-method
branch
from
03fe91e to
3959695
Compare
ockham
reviewed
Apr 14, 2021
Comment on lines
+297
to
+300
| foreach ( $valid_block_names as $block_name ) { | ||
| $schema['styles'][ $block_name ] = self::VALID_STYLES; | ||
| $schema['settings'][ $block_name ] = self::VALID_SETTINGS; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #30541 so it can land in pieces and make reviews easier/quicker.
This PR extracts the sanitization to its own method and simplifies its logic so that changing the structure at #30541 is easier.
How to test
npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.php