automattic / vip-governance-plugin Goto Github PK

Change allowedChildren to allowedBlocks as that makes more sense. Also, upgrade the version when this happens.

Describe the bug
When opening a post that has illegal blocks, the following warning appeared:

Seems to be related to this line:

To Reproduce

• Add a block, disallow it in governance.json, reload the post

Expected behavior
No warnings in console

Actual behavior
Warnings in console

Version of the plugin
1.0.6

Additional context

react-dom.js?ver=18.2.0:73 Warning: Unsupported style property background-color. Did you mean backgroundColor?
    at div
    at div
    at Disabled (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:61076:3)
    at https://testsite.lndo.site/wp-content/plugins/vip-governance/build/index.js?ver=289fda8f4f8dd70bdba8:1:3203
    at FilteredComponentRenderer (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:75975:9)
    at EditWithGeneratedProps (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:16549:5)
    at BlockEdit (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:16604:5)
    at BlockCrashBoundary (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:16932:5)
    at BlockListBlock (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:18190:10)
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:15699:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:32182:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:33391:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:34958:21
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:37068:112
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:37911:67
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:38389:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:38705:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:38779:5
    at FilteredComponentRenderer (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:75975:9)
    at https://testsite.lndo.site/wp-includes/js/dist/compose.js?ver=228e7d7fccaae67c220c:3232:10
    at https://testsite.lndo.site/wp-includes/js/dist/data.js?ver=2b5bb06caaeb5048ed96:4436:25
    at https://testsite.lndo.site/wp-includes/js/dist/data.js?ver=2b5bb06caaeb5048ed96:4286:22
    at shouldComponentUpdate (https://testsite.lndo.site/wp-includes/js/dist/compose.js?ver=228e7d7fccaae67c220c:3268:12)
    at Items (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:28529:3)
    at BlockListItems
    at div
    at Root (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:28447:3)
    at BlockList
    at RecursionProvider (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:56320:3)
    at div
    at WritingFlow (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:20399:3)
    at ExperimentalBlockCanvas (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:41821:3)
    at div
    at MotionComponent (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:18531:46)
    at div
    at MotionComponent (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:18531:46)
    at div
    at BlockTools (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:55870:3)
    at VisualEditor (https://testsite.lndo.site/wp-includes/js/dist/edit-post.js?ver=57aa460a64bb0e4ae963:3929:3)
    at div
    at NavigableRegion (https://testsite.lndo.site/wp-includes/js/dist/edit-post.js?ver=57aa460a64bb0e4ae963:1391:3)
    at div
    at div
    at div
    at InterfaceSkeleton (https://testsite.lndo.site/wp-includes/js/dist/edit-post.js?ver=57aa460a64bb0e4ae963:1457:3)
    at Layout (https://testsite.lndo.site/wp-includes/js/dist/edit-post.js?ver=57aa460a64bb0e4ae963:8679:63)
    at ErrorBoundary (https://testsite.lndo.site/wp-includes/js/dist/editor.js?ver=09660a848fa8379476ab:5911:5)
    at BlockRefsProvider (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:17776:3)
    at Provider (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:36533:3)
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:19222:5
    at https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:18775:5
    at WithRegistryProvider(Component)
    at BlockContextProvider (https://testsite.lndo.site/wp-includes/js/dist/block-editor.js?ver=0930e041f64667e446db:16486:3)
    at EntityProvider (https://testsite.lndo.site/wp-includes/js/dist/core-data.js?ver=77370c9a15a7db2ae084:6359:3)
    at EntityProvider (https://testsite.lndo.site/wp-includes/js/dist/core-data.js?ver=77370c9a15a7db2ae084:6359:3)
    at https://testsite.lndo.site/wp-includes/js/dist/editor.js?ver=09660a848fa8379476ab:12496:3
    at https://testsite.lndo.site/wp-includes/js/dist/editor.js?ver=09660a848fa8379476ab:12081:5
    at WithRegistryProvider(Component)
    at SlotFillProvider (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:36376:3)
    at provider_SlotFillProvider (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:36398:5)
    at Provider (https://testsite.lndo.site/wp-includes/js/dist/components.js?ver=9a812780592a1d78379b:36533:3)
    at Editor (https://testsite.lndo.site/wp-includes/js/dist/edit-post.js?ver=57aa460a64bb0e4ae963:9253:3)

The current approach to allowing critical core blocks is via a hardcoded map. There is a way to access the allowedBlocks property in the innerBlocks using the following command:

wp.data.select( 'core/block-editor' ).getAllowedBlocks('49abcad4-4be5-46d0-8db5-44760f15e0e7')

Using this in code causes a call stack exceeded. If this works, this would be a better solution long term.

Add more unit tests for all the code that has no tests

Since we have verified that duotone support is not there, this has been made to add that in. What needs to be done is that, CSS has to be injected onto the page to get duotone values as styles that can be applied.

Create a validate CLI command that can be used to quickly confirm if a rules file is correct or not. This could provide a list of core blocks that won't be supported and any potential pitfalls that could come from this.

Our development workflow atm is not the best when it comes to vendor changes. We need to ensure we run composer install --no-dev for most cases but for development we need to run composer install. This means that for husky to work we have to be mindful of which one we ran and ensure we do not accidentally check in the test pieces.

There has to be a better way to do this, and we should find that. Simplify the overall workflow.

This one applies to the Governance Plugin. We should switch to using Typescript so it's consistent with the rest of VIP and also because Typescript is better.

While reviewing the changes in #24, I ran into an issue allowing a user to edit a core/list block within another block. The screenshots in this issue reference PR 24, but that's likely not relevant to the problem.

core/list at the root level

First, let's start with a basic set of locked-down rules that allow an editor to edit a handful of root blocks including core/list and core/list-item:

{
    "$schema": "./governance-schema.json",
    "version": "0.2.0",
    "rules": [
        {
            "type": "default",
            "allowedFeatures": [],
            "allowedBlocks": [ "core/paragraph" ]
        },
        {
            "type": "role",
            "roles": [ "editor" ],
            "allowedBlocks": [ "core/media-text", "core/heading", "core/list", "core/list-item" ],
            "blockSettings": {}
        }
    ]
}

This works fine! Lists can be inserted easily at the root level:

core/list in a core/media-text

Next, let's change the editor role rule. We'll move core/list so that it's the only allowed block in a core/media-text instead. The goal is that we allow core/media-text blocks at the root level, and inside core/media-text blocks we only allow lists.

"rules": [
    {
        "type": "default",
        "allowedFeatures": [],
        "allowedBlocks": [ "core/paragraph" ]
    },
    {
        "type": "role",
        "roles": [ "editor" ],
        "allowedBlocks": [ "core/media-text", "core/heading" ],
        "blockSettings": {
            "core/media-text": {
                "allowedBlocks": [ "core/list" ]
            }
        }
    }
]

This unfortunately doesn't work because the content portion of core/media-text is a core/paragraph, and that's not editable:

It seems strange that the default rule core/paragraph doesn't apply here. It may be worth evaluating whether default blocks should be included automatically in allowedBlocks, since I'll need to re-add core/paragraph to any nested block settings. But for now, that's fine, I'll add the paragraph block it to core/media-text's allowedBlocks:

core/list in a core/media-text - Attempt 2

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/list", "core/paragraph" ]
        }
    }
}

This fixes the content paragraph issue but unfortunately I can't interact with the list item:

This appears to be because core/list-item isn't allowed. This may be another potential area of improvement. Some blocks like core/list or core/columns are only usable with their child blocks enabled. Potentially we can hardcode some core blocks to support necessary child items to remove this responsibility from the rules file creator.

Anyway, let's add core/list-item support to core/media-text and try that:

core/list in a core/media-text - Attempt 3

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list", "core/list-item" ]
        }
    }
}

Nope, this still won't let me interact with the list item:

This is surprising - this same set of allowedBlocks works fine at the root level for inserting core/list-items, but not in core/media-text. I'll try explicitly making a nested rule for core/list:

core/list in a core/media-text - Attempt 4

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list" ],
            "core/list": {
                "allowedBlocks": [ "core/list-item" ]
            }
        }
    }
}

Still doesn't work, same result as above. Maybe I need to add core/paragraph to the core/list rules too?

core/list in a core/media-text - Attempt 5

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list" ],
            "core/list": {
                "allowedBlocks": [ "core/paragraph", "core/list-item" ]
            }
        }
    }
}

That didn't do it either, same result as before:

I can't find a way to make the original rule idea work:

The goal is that we allow core/media-text blocks at the root level, and inside core/media-text blocks we only allow lists.

Conclusion

There are a few potential issues here:

1. First and most importantly, I'm not sure how to make a ruleset work where I allow core/list nested in a block and can interact with it's children core/list-item blocks. I can only apply these rules at the root level, but not in a nested block. This seems like a bug, or I'm missing a configuration step.

2. From "Attempt 1" above:

   It seems strange that the default rule core/paragraph doesn't apply here. It may be worth evaluating whether default blocks should be included automatically in allowedBlocks, since I'll need to re-add core/paragraph to any nested block settings.

   Our default ruleset makes it easy to avoid repeating rules like core/paragraph everywhere, but it only applies to the root set of blocks. Once we start making inner allowedBlocks, we're still required to repeat that. We should consider solutions where that isn't necessary.

3. From "Attempt 2":

   This may be another potential area of improvement. Some blocks like core/list or core/columns are only usable with their child blocks enabled. Potentially we can hardcode some core blocks to support necessary child items to remove this responsibility from the rules file creator.

   This would ease some of the difficulty of dealing with fiddly inner allowedBlocks rules for some block types.

4. From "Attempt 3":

   This is surprising - this same set of allowedBlocks works fine at the root level for inserting core/list-items, but not in core/media-text.

   The surprising bit here is that this set of rules works fine, and allows me to insert core/list-items in core/media-text:

   {
       "type": "role",
       "roles": [ "editor" ],
       "allowedBlocks": [ "core/media-text", "core/list", "core/list-item" ]
   }

   However, this set doesn't work, with the same rules applied at the core/media-text block level:

   {
       "type": "role",
       "roles": [ "editor" ],
       "allowedBlocks": [ "core/media-text" ],
       "blockSettings": {
           "core/media-text": {
               "allowedBlocks": [ "core/paragraph", "core/list", "core/list-item" ]
           }
       }
   }

   Root-level allowedBlocks apply to children blocks, but inner allowedBlocks appear to not. This feels inconsistent.

Currently, we don't hide the non-theme colours shown to the user when we override what style option is available. We should look into customizing that so that only our option is available, or we mention as a README note to follow.

Duotone support seems to be done via another function that we don't cover at the moment. This is a property that we haven't tested afaik, so we should verify this and add it to the limitations in the README.

Use this mirroring action to ensure the two repos stay updated.

Due to lodash not being enqueued by default anymore in WordPress 6.4, there are block recovery errors happening when trying to select some nested settings in the editor.

The solution is to either enqueue lodash, or use a native alternative to get that we use to fetch the setting at any depth based on the path provided.

Currently, the plugin just customizes the settings that could be applied to a block. It doesn't automatically apply them when the editor is loaded via styles. This is meant to explore that, so that we can enable post/role based styling in the same way that we can do it via settings. This would open the door to nested styling via role/post type rules.

Before the plugins' release we need to make sure the docs have been improved.

In governance error handling code, we have two phases:

1. Verify that the JSON passes validation
2. Verify that the JSON matches the schema

While the schema validation is helpful, it still can miss logic errors or produce hard-to-understand errors. For example, if we have a malformed rule like:

{
    "type": "role"
}

We'll show this error:

Schema validation failed: {
    "/rules/2/type": "The data should match one item from enum",
    "/rules/2": "The required properties (roles) are missing"
}

It's not very clear. When we previously used min/max properties, we'd show this instead:

Schema validation failed: {
    "/rules/2": "Object must have at least 2 properties, 1 found"
}

This catches a wider group of error types, but does not explaining what's needed or missing clearly.

I propose we add a layer of rule checking between steps 1 & 2 outlined at the top, where we check rules for logic errors and return clear error messages. This would allow us to produce better error messages than our schema checker can make, and also catch logic errors that might not be possible to encode via schema.

The javascript code is currently not tested, we should add tests for that. This issue is meant to get the framework in place and add some starter tests.

We can start with block-utils as that's simple.

Add a new e2e test that creates a heading, and a heading within a media text. It then verifies the custom colour yellow for a heading at the root level, and red within a media text.

The following update needs to be made to the wp-env.json to ensure the right rules file is loaded. There is a bug which needs to be solved which is the js/css code we want to load doesn't load. Its likely because the path is off for the plugin dir.

{
	"plugins": [ "." ],
	"mappings": {
		"wp-content/plugins/vip-governance": ".",
		"wp-content/private": "./tests/private"
	},
	"config": {
		"WPCOM_VIP_PRIVATE_DIR": "/var/www/html/wp-content/private"
	}
}

Since useblockEditingMode() has been made public, we should incorporate this in as a replacement for our block locking code in block-locking.jsx.

The strategy would be that:

• If this method exists, then we should make a call to useBlockEditingMode( 'disabled' ) prior to wrapping the block in the disabled tag.
• We may need to see if our existing block-locking.php code is still needed or not.

It's already in Gutenberg 16.5 and will be in WordPress 6.4.

Typography properties are not supported in the plugin and we need to fix that.

Currently, it's not possible to customize the rules per network site as it's the same private directory for all of them. We should offer a way to customize this, and this could potentially be used then by those not using the plugin on VIP as well.

Unlike Gutenberg, we don't currently skip over insecure settings and css values. We should be doing that to ensure we don't create a security problem.

This would be really doing what class-wp-theme-json-gutenberg#remove_insecure_settings does.

Implement the request in this Gutenberg issue, but via governance.json by adding an editCSS property to allowedFeatures

This is the TODO from block-utils.js code that wasn't documented in the limitations in the README.

For block searches within a child we only go one level in the parent-child hierarchy in the governance rules.

This is not interpreted correctly that means.

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list" ],
            "core/list": {
                "allowedBlocks": [ "core/paragraph", "core/list-item" ]
            }
        }
    }
}

We have too many manual steps for creating a release in RELEAE.md, and we should automate that. Gutenberg can be used as an example for how to do this.

Hi Gopal—Thanks so much for all your work on this plugin!

I'm not sure whether this is the best place for these questions, but I also wasn't sure where else to ask them.

In terms of supporting non-WordPress VIP usage, do you have any plans to:

• Host the plugin on the WordPress.org Plugin Directory?
• Allow users to customize a location for the governance-rules.json file (or to create a default location outside of the plugin itself for non-VIP users)?

In terms of the location for the governance-rules.json file: Do you feel the file needs to be kept private for security reasons, or is the WordPress VIP private folder just the most straightforward place for WordPress VIP users to put their own copy?

Thank you again!

Update the analytics to be for VIP sites only, and add PHPDocs wherever it's possible

Using this PR as inspiration, drop the use of wp_array_get as much as possible.

Currently, all rules are loaded via a file. This prevents more dynamic rule generation.

Could a filter be added so that rules loaded via the file could be augmented by programmatically generated JSON? Or file based loading skipped entirely if programmatic generation is in use?

This comes from feedback provided by a customer.

We should allow restricting different sets of allowed blocks based on the post type in addition to what we allow right now.

Drop all the ported WP code for CSS injection, in favour of a single method.

Describe the bug
Frontend assets are enqueued with a path that can result in a 404 (.e.g http://localhost:8080/var/www/html/wp-content/plugins/vip-governance/css/vip-governance.css?ver=1.0.5)

To Reproduce
Install the plugin on a docker container running wordpress:6.4-php8.2.

Expected behavior
Assets get pushed as wp-content/plugins/vip-governance/css/vip-governance.css?ver=1.0.5

Actual behavior
Assets get pushed as http://localhost:8080/var/www/html/wp-content/plugins/vip-governance/css/vip-governance.css?ver=1.0.5

Version of the plugin
1.0.5

Additional context
Replacing some uses of WPCOMVIP_GOVERNANCE_ROOT_PLUGIN_DIR with plugin_dir_url(WPCOMVIP_GOVERNANCE_ROOT_PLUGIN_FILE) resolved the issue.

This has been created to write e2e tests using playwright, that will allow us to actually verify that the plugin works within the editor.

Integrate tracks into the plugin so we can record metrics and other data points that we consider important.

Open Question - since this is a beta, should opt out be offered?