Add support for wildcard rules in block settings by ingeniumed · Pull Request #85 · Automattic/vip-governance-plugin

ingeniumed · 2024-09-10T06:47:03Z

Description

There are 3 big changes in this PR:

Add support for regex block rules in the block settings, similar to what's allowed in the allowedBlocks. This will solve Add support for wildcard block names when specifying block-specific settings rules #82. See this issue for an example in action.
Allow programmatic rule generation for the governance rules via a filter, which will mean no more file necessary outside the plugin itself. This will solve Add filter to enable for programmatic JSON generation. #62.
Remove the need for a blank CSS file, by registering a dummy style sheet. This will solve Governance style sheet being loaded on the front end #84.

There's two small changes as well:

Enable webpack, and use the existing rule set taken from our vip-workflow plugin so the overall configuration is simple.
Update all the packages

…PHP code

alecgeatches · 2024-09-10T19:38:19Z

From an initial runthrough of the code, this looks good!

My only thought prior to the full PR would be to change "regex" to "wildcard" in the PR title and internal functions. We don't actually support regular expressions here (which I think is a good thing), just wildcards. I think we should use the term "wildcard" consistently to avoid confusion.

….md as well

ingeniumed · 2024-09-11T03:14:03Z

-
-			( { value: result } = getNestedSetting( blockNamePath, path, nestedSettings ) );
+			// iterate through the nestedSettingPaths to find the blockName
+			for ( const nestedBlockName in nestedSettingPaths ) {


@alecgeatches - just need to pay special attention to this block as I touched it up a touch. I'd like to see if we could do some more pre-processing on the PHP side but that's for another PR.

The main thing to keep in mind for this filter is that the blockEditor.useSetting.before filter runs a lot of times. In a quick test locally on a post with a moderately complex post, it's easy to hit this filter 10k+ times in a few seconds by just hovering around the page.

You may already have the fastest option here that's flexible, but if there's anything we can do to return early for non-matched blocks, we want to do so as quickly as possible.

I've got one idea for optimization:

In JS prior to setting up this hook, we could split nestedSettingPaths into nestedSettingWildcards for names we need to evaluate like core/*, and a set of non-wildcards like nestedSettingNonWildcards (or a better name). Then we could keep the linear-time check that we had before using hasCustomSetting when matching non-wildcards, and only do a loop through any wildcards. If there are no wildcards, then there's no loop necessary.

Overall I think this is pretty fast. I would split up wildcard and non-wildcard checks though.

My thought was to try and do that on the PHP side so that we take advantage of server side rendering actually. That way we minimize the pre-processing necessary on the JS side 🤔

ingeniumed · 2024-09-11T03:23:41Z

From an initial runthrough of the code, this looks good!

My only thought prior to the full PR would be to change "regex" to "wildcard" in the PR title and internal functions. We don't actually support regular expressions here (which I think is a good thing), just wildcards. I think we should use the term "wildcard" consistently to avoid confusion.

This is a good point, and something that I've now fixed. Lemme know if I missed any occurrence.

ingeniumed · 2024-09-11T04:05:19Z

I also added in support for 6.0 and PHP 8.0/8.3 in tests btw.

Co-authored-by: Alec Geatches <alec.geatches@automattic.com>

…Automattic/vip-governance-plugin into add/support-for-regex-in-block-settings

alecgeatches · 2024-09-12T19:05:34Z

@ingeniumed I'm seeing an issue with a custom palette under a core/* property. Here's my rules:

{
	"$schema": "https://api.wpvip.com/schemas/plugins/governance.json",
	"version": "1.0.0",
	"rules": [
		{
			"type": "role",
			"roles": [ "administrator" ],
			"allowedFeatures": [ "codeEditor", "lockBlocks" ],
			"allowedBlocks": [ "core/media-text" ],
			"blockSettings": {
				"core/media-text": {
					"core/*": {
						"color": {
							"text": true,
							"palette": [
								{
									"color": "#ff00ff",
									"name": "Custom purple",
									"slug": "custom-purple"
								}
							]
						}
					}
				}
			}
		},
		{
			"type": "default",
			"allowedBlocks": [ "core/heading", "core/paragraph", "core/image" ]
		}
	]
}

In the editor, this works correctly. I made a core/media-text block containing a heading with the custom color:

However, the custom color doesn't appear on the frontend:

This is the frontend CSS getting injected into the page:

.wp-block-media-text .wp-block {
    --wp--preset--color--custom-purple: #ff00ff;
}

.wp-block-media-text .wp-block.has-custom-purple-color {
    color: var(--wp--preset--color--custom-purple) !important;
}

.wp-block-media-text .wp-block.has-custom-purple-background-color {
    background-color: var(--wp--preset--color--custom-purple) !important;
}

.wp-block-media-text .wp-block.has-custom-purple-border-color {
    border-color: var(--wp--preset--color--custom-purple) !important;
}

The .wp-block selector doesn't seem to work. Here's the media-text HTML used on the frontend:

<div class="wp-block-media-text is-stacked-on-mobile">
    <figure class="wp-block-media-text__media">
        <img <!--...--> class="wp-image-273 size-full">
    </figure>

    <div class="wp-block-media-text__content">
        <h2
            class="wp-block-heading has-custom-purple-color has-text-color has-link-color wp-elements-f7bf55209eb97c294140446761d1d65d"
            >Heading</h2>
    </div>
</div>

Above, the h2 has a .wp-block-heading class but not a .wp-block, so the .has-custom-purple-color class isn't applied.

This makes me wonder if we need to generate a matching rule for all possible subblocks, although would probably get exponential quickly with a handful of nested wildcards. Any idea on how to solve this?

alecgeatches

Left some comments! The main issue I found is here: #85 (comment), where the .wp-block trick doesn't work for headings. I'm not sure if this is something that'll be easy to get around given .wp-block doesn't seem to be consistent.

Great job on this feature otherwise! There's a lot of complexity figured out in here.

alecgeatches · 2024-09-12T18:06:49Z

+
+For an example of this feature, refer [here](#default-wildcard-rule-set).
+
+Note: `allowedBlocks` are not respected under a wildcard block within `blockSettings`. This will only be respected under a targeted block such as `core/quote`.


I don't really follow this. Could you maybe add an example of a improper ruleset ❌, and then maybe an example of a working one ✅ to explain the issue?

Good point, I'll do that for an improper one. I've already linked an existing example for a proper one

I added another update to this documentation. It still wasn't clear enough to me for a couple of reasons:

We have an example of a bad schema, but it wasn't obvious to me what a fix looked like. I added a second example to show how a customer might work around the issue and still make use of wildcards.

I don't like having only an incorrect example under the "Wildcards" header. If I was trying to work quickly, I might search for "wildcard" on the README page and just copy the first example I found without realizing it was meant to be a counterexample. I made that more clear with ❌/ ✅ right above both examples.

I added these as headings at the fifth level (#####), which I've purposefully excluded from the table of contents. Typically TOC generators have a set level of headings (e.g. 1-4), and subheadings below that level are excluded. I think that makes sense for the TOC here in this README as well.

alecgeatches · 2024-09-12T18:24:27Z

 							"text": false
 						}
 					},
+					"core/*": {


Good thinking adding this as an example! I think you can remove the prior core/paragraph example though, as this covers that case. For me in VSCode, the whole set of examples fills in when I tab-in a rule:

I think this is redundant to have two rules to set color.text to false, as technically the second rule would cover the case of the first. We could either have the core/paragraph example show something different, or just remove it here.

Good point, I'll replace it with a gradient rule

alecgeatches · 2024-09-12T18:50:44Z

-
-			( { value: result } = getNestedSetting( blockNamePath, path, nestedSettings ) );
+			// iterate through the nestedSettingPaths to find the blockName
+			for ( const nestedBlockName in nestedSettingPaths ) {


The main thing to keep in mind for this filter is that the blockEditor.useSetting.before filter runs a lot of times. In a quick test locally on a post with a moderately complex post, it's easy to hit this filter 10k+ times in a few seconds by just hovering around the page.

You may already have the fastest option here that's flexible, but if there's anything we can do to return early for non-matched blocks, we want to do so as quickly as possible.

alecgeatches · 2024-09-12T19:12:05Z

-
-			( { value: result } = getNestedSetting( blockNamePath, path, nestedSettings ) );
+			// iterate through the nestedSettingPaths to find the blockName
+			for ( const nestedBlockName in nestedSettingPaths ) {


I've got one idea for optimization:

In JS prior to setting up this hook, we could split nestedSettingPaths into nestedSettingWildcards for names we need to evaluate like core/*, and a set of non-wildcards like nestedSettingNonWildcards (or a better name). Then we could keep the linear-time check that we had before using hasCustomSetting when matching non-wildcards, and only do a loop through any wildcards. If there are no wildcards, then there's no loop necessary.

alecgeatches · 2024-09-12T19:25:38Z

-
-			( { value: result } = getNestedSetting( blockNamePath, path, nestedSettings ) );
+			// iterate through the nestedSettingPaths to find the blockName
+			for ( const nestedBlockName in nestedSettingPaths ) {


Overall I think this is pretty fast. I would split up wildcard and non-wildcard checks though.

ingeniumed · 2024-09-13T04:00:53Z

 	const nestedSettingPaths = getNestedSettingPaths( nestedSettings );

+	// pull out all the wildcard block names.
+	const nestedWildcardPaths = Object.keys( nestedSettingPaths ).reduce( ( acc, blockName ) => {


Optimized it based on your suggestion and it def feels a touch faster

Thank you! This is what I was picturing. I pushed up a small update to split these in one pass imperatively, a tiny optimization since we're going for speed here:

vip-governance-plugin/src/index.js

Lines 79 to 90 in 9b80bba

const nestedWildcardPaths = {};

const nestedNonWildcardPaths = {};

for ( const blockName in nestedSettingPaths ) {

if ( blockName.indexOf( '*' ) === -1 ) {

// eslint-disable-next-line security/detect-object-injection

nestedNonWildcardPaths[ blockName ] = nestedSettingPaths[ blockName ];

} else {

// eslint-disable-next-line security/detect-object-injection

nestedWildcardPaths[ blockName ] = nestedSettingPaths[ blockName ];

}

}

…Automattic/vip-governance-plugin into add/support-for-regex-in-block-settings

alecgeatches · 2024-09-13T18:36:46Z

@ingeniumed Made some minor changes, going to merge this!

ingeniumed added 5 commits September 9, 2024 15:52

Upgrade packages, and add support for regex in block settings in the …

ad2f788

…PHP code

Simplify the build process by using webpack

157c62f

Simplify the code a lot more

52ac9e4

Delete the blank css file

a0fd2f3

Up the version

c79898a

ingeniumed self-assigned this Sep 10, 2024

ingeniumed added 3 commits September 10, 2024 16:48

Update the package-lock

fa3b716

Attempting to add a test for the nested-governance changes

9c316d1

Fix the test css to match against

41baa82

alecgeatches reviewed Sep 10, 2024

View reviewed changes

Comment thread governance/nested-governance-processing.php Outdated

Update the tests, add the new filter to the README and add a security…

3a5c03c

….md as well

ingeniumed commented Sep 11, 2024

View reviewed changes

Comment thread webpack.config.js

ingeniumed commented Sep 11, 2024

View reviewed changes

ingeniumed changed the title ~~Add support for regex rules in block settings~~ Add support for wildcard rules in block settings Sep 11, 2024

Change the occurrence of regex to wildcard

eaad20b

ingeniumed added 2 commits September 11, 2024 13:27

Run the ci on all php versions

da9b146

Adjust the settings and css for WP 6.0

246fc74

ingeniumed marked this pull request as ready for review September 11, 2024 04:04

ingeniumed requested a review from a team as a code owner September 11, 2024 04:04

ingeniumed requested a review from alecgeatches September 11, 2024 04:05

ingeniumed and others added 3 commits September 12, 2024 11:11

Update governance/nested-governance-processing.php

ae8d70f

Co-authored-by: Alec Geatches <alec.geatches@automattic.com>

Simplify the webpack config

0fac393

Merge branch 'add/support-for-regex-in-block-settings' of github.com:…

57ca4ab

…Automattic/vip-governance-plugin into add/support-for-regex-in-block-settings

alecgeatches reviewed Sep 12, 2024

View reviewed changes

Comment thread package.json Outdated

alecgeatches requested changes Sep 12, 2024

View reviewed changes

Update package.json

8724dcd

ingeniumed added 3 commits September 13, 2024 11:52

Fix the bug where the CSS wasn't applied on published posts correctly

d7418c5

Update the examples provided for wildcards

fdb8bcb

Optimize the useSetting hook to be a lot faster

4a808e2

ingeniumed commented Sep 13, 2024

View reviewed changes

Comment thread governance/nested-governance-processing.php

ingeniumed commented Sep 13, 2024

View reviewed changes

Comment thread src/nested-governance-loader.js

ingeniumed requested a review from alecgeatches September 13, 2024 04:01

alecgeatches added 5 commits September 13, 2024 11:32

Merge branch 'trunk' into add/support-for-regex-in-block-settings

fc2c121

Add second example for parent block nesting wildcard limitations

5c9bc28

Merge branch 'add/support-for-regex-in-block-settings' of github.com:…

b15a5ae

…Automattic/vip-governance-plugin into add/support-for-regex-in-block-settings

Split wildcard paths in one pass

a0366b2

Reverse if condition order for readibility

9b80bba

alecgeatches added 2 commits September 13, 2024 12:40

Merge branch 'trunk' into add/support-for-regex-in-block-settings

3a6b901

Fix indentation

a99337b

alecgeatches approved these changes Sep 13, 2024

View reviewed changes

alecgeatches merged commit 8be16bd into trunk Sep 13, 2024

alecgeatches deleted the add/support-for-regex-in-block-settings branch September 13, 2024 18:43

This was referenced Sep 13, 2024

Bump webpack from 5.89.0 to 5.94.0 #83

Closed

Governance style sheet being loaded on the front end #84

Closed

Add filter to enable for programmatic JSON generation. #62

Closed


		For an example of this feature, refer [here](#default-wildcard-rule-set).

		Note: `allowedBlocks` are not respected under a wildcard block within `blockSettings`. This will only be respected under a targeted block such as `core/quote`.

	const nestedWildcardPaths = {};
	const nestedNonWildcardPaths = {};

	for ( const blockName in nestedSettingPaths ) {
	if ( blockName.indexOf( '*' ) === -1 ) {
	// eslint-disable-next-line security/detect-object-injection
	nestedNonWildcardPaths[ blockName ] = nestedSettingPaths[ blockName ];
	} else {
	// eslint-disable-next-line security/detect-object-injection
	nestedWildcardPaths[ blockName ] = nestedSettingPaths[ blockName ];
	}
	}

Conversation

ingeniumed commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

alecgeatches commented Sep 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingeniumed commented Sep 11, 2024

Uh oh!

ingeniumed commented Sep 11, 2024

Uh oh!

Uh oh!

alecgeatches commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecgeatches left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingeniumed Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alecgeatches commented Sep 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ingeniumed commented Sep 10, 2024 •

edited

Loading

alecgeatches commented Sep 12, 2024 •

edited

Loading

ingeniumed Sep 13, 2024 •

edited

Loading