Extract theme json processor #26803
Extract theme json processor #26803
Conversation
|
Size Change: -92 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
gziolo
added
the
Global Styles
oandregal
force-pushed
the
try/extract-theme-json-processor
branch
from
da1e259 to
c0478f5
Compare
oandregal
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
I still need to fix some PHP linting issues, but it's now ready for review! |
oandregal
force-pushed
the
try/extract-theme-json-processor
branch
from
05d4dc9 to
b43d928
Compare
| * } | ||
| */ | ||
| const SCHEMA = array( | ||
| 'selector' => null, |
There was a problem hiding this comment.
Specifying the schema pointing things can contain strings like selector to null seems strange.
What if specify in the same format to specify inputs for the rest API:
Example:
/**
* Get our sample schema for comments.
*/
function prefix_get_comment_schema() {
$schema = array(
// This tells the spec of JSON Schema we are using which is draft 4.
'$schema' => 'http://json-schema.org/draft-04/schema#',
// The title property marks the identity of the resource.
'title' => 'comment',
'type' => 'object',
// In JSON Schema you can specify object properties in the properties attribute.
'properties' => array(
'id' => array(
'description' => esc_html__( 'Unique identifier for the object.', 'my-textdomain' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
'author' => array(
'description' => esc_html__( 'The id of the user object, if author was a user.', 'my-textdomain' ),
'type' => 'integer',
),
'content' => array(
'description' => esc_html__( 'The content for the object.', 'my-textdomain' ),
'type' => 'string',
),
),
);
return $schema;
}
There was a problem hiding this comment.
You mean, adding type validation at the leaf node level?
It crossed my mind, and I think it'd be potentially helpful if connected with the logging mechanism for theme authors to see, which is something they have suggested they'd want. It can be useful, yes, but perhaps can be added in a subsequent PR? Would love to land this one quickly to unblock some work, let other people use this, and avoid many rebases (which are going to be / already were quite painful and time-consuming).
There was a problem hiding this comment.
Yah having a schema would allow validation at the node level, and I guess developers could even use validation tools to make sure their theme.json is correct.
I guess the first version of a schema could just validate what we have currently but specifying things in schema.json would be better than "'selector' => null," as the current syntax is a little confusing, selector is not null it is a string. It seems we are inventing our own schema.
I agree it can be done as a follow up 👍
There was a problem hiding this comment.
Created #26955 to track this.
lib/class-wp-theme-json.php
Outdated
| * @param array $input Whole tree to normalize. | ||
| * @param array $schema Schema to use for normalization. | ||
| */ | ||
| private function process_key( $key, &$input, $schema ) { |
There was a problem hiding this comment.
Why do we need to remove the nodes that aren't valid per the schema? When outputting CSS could we simply ignore these nodes?
There was a problem hiding this comment.
Right, we can do validation at entering-time or at outputting-time. I chose to do it on entering because that simplifies all the internal code for processing the tree (stylesheet processing but also settings, etc). It's also safer, as we are strict on which things we allow and which things we don't, which is important in case attackers are able to hook into the theme.json in the server.
lib/class-wp-theme-json.php
Outdated
| * @param array $declarations Holds the existing declarations. | ||
| * @param array $context Input context to process. | ||
| */ | ||
| private function compute_style_properties( &$declarations, $context ) { |
There was a problem hiding this comment.
Should this function be static?
There was a problem hiding this comment.
I don't have a strong opinion on this, I'm fine marking them as static. I'm curious why it'd be preferred, though!
There was a problem hiding this comment.
I think it is a good practice to have all the functions that don't depend on the instance of a class marked as static.
oandregal
force-pushed
the
try/extract-theme-json-processor
branch
2 times, most recently
from
38f89d2 to
b591b11
Compare
oandregal
force-pushed
the
try/extract-theme-json-processor
branch
from
aee8a21 to
77d626a
Compare
This PR does a few things:
Fixes Protect against bad data in processing theme.json #26804
Fixes block.json key retrieval: it's called
linkand not linkColor.Fixes one of the issues noted at Reduce the amount of data Global Styles sends to the client #26802 by sending through
__experimentalFeaturesand__experimentalGlobalStylesBaseStylesonly the relevant data to the client. It fixes the root cause: how we normalize the schema.Extracts the first piece out of
global-styles.php, so that it has a structure that helps us maintain, grow, test, and merge it into WP core. More to come in subsequent PRs. It was increasingly difficult to land a big PR with so many features and people working in parallel, so I think it's best to go step by step.How to test
Test that the link color works as expected:
Test that it only passes the necessary data: #26802 for details
core/edit-sitestore only holds the necessary data withinsettings.__experimentalData. Example:Test that an invalid context doesn't break the site #26804
lib/global-styles.phpand add a new context that doesn't represent any block's selector: