Block plugin validation tool UI improvements

[tellyworth]

The UI for the forthcoming block validator tool is very rough and needs improvement.

[ryelle]

Initial UI improvements— some error message cleanup, restructured the flow of the results page, and removed the icon links for text links

[tellyworth]

In 10003:

Block directory: initial UI improvements for block validator.

Props ryelle.
See #5303.

[tellyworth]

In 10006:

Plugin directory: link validator messages to sample help page.

The docs probably belong somewhere else, but this will serve as an example.

See #5303

[tellyworth]

In 10007:

Plugin directory: make it easier to validate block plugins by name or URL.

This improves the input handler to accept either a plugin slug, or a URL like ​https://wordpress.org/plugins/the-slug/.

See #5303.

[tellyworth]

In 10011:

Plugin directory: add a pass/fail message to the block validator.

See #5303

[ryelle]

Allow passing in github branches, ex: ​https://github.com/ryelle/rmb-recipe-block/tree/try/block-json

[ryelle]

Check that the plugin only exports a single parent-level block

[ryelle]

single-parent-block.diff​ This check is just an idea, I don't think we've officially made it a rule — there should be only one "top level" block per block-plugin. Having a single block ID'd as the "main block" would be helpful for the block dir API too, make sure we're showing the right block name.

Currently this check only works with block.json files, since we can't/don't get the rest of the config settings from the JS or PHP calls. So for now, it's just a warning, and it could be incorrect.

[ryelle]

style-plugin.diff​ & style-theme.diff​ are a pair, some markup & CSS fixes to clean up the page a bit. I'm not sure I made the change in the right place in the theme, so I can move that around if needed.

[tellyworth]

Replying to ryelle:

single-parent-block.diff​ This check is just an idea, I don't think we've officially made it a rule — there should be only one "top level" block per block-plugin. Having a single block ID'd as the "main block" would be helpful for the block dir API too, make sure we're showing the right block name.

Yep this is great. At the moment there's a much lazier check which blindly assumes that < 5 blocks might be parent/child blocks but 5+ is too many. It definitely should be a rule that there's only one top level block. I did have an earlier idea that we could enforce only one block.json file per plugin, with that file representing the top-level block. But this is much more comprehensive. It seems reasonable to say "if you want to include multiple parent/child blocks, you need to use block.json files for all of them with the correct parent values."

Let's put this in, and update the 5 block rule in check_for_blocks to be a higher number. At some point if there are, I dunno, 15 or 20 blocks in there, even if the parent/child relationship is configured correctly it might be a sign that something's gone wrong.

[ryelle]

In 10035:

Plugin Directory: Fix PHP warnings in block validator

See #5303

[ryelle]

In 10037:

Plugin Directory: Add github branch support to the block validator

See #5303

[ryelle]

In 10038:

Plugin Directory: Clean up block validation form & result messages.

See #5303

[ryelle]

In 10039:

Plugin Directory: First pass of style updates for block validator

• Fix label alignment
• Fix URL input alignment on smaller screens
• Fix font sizes of messages in notices

Bonus plugin upload fix: add focus style to the file upload button

See #5303

[ryelle]

In 10040:

Plugin Directory: Add a check for one top-level block.

Multiple blocks are allowed, but there should be only one "main" block. This checks the block.json file for each block (if exists), and flags if there is more than 1 block without a parent property. This also fixes the check_for_blocks check to warn if more than 15 blocks are found.

See #5303

[Ipstenu]

I think the change in r10040 (checking for one parent) may be too aggro. For example, listicles gets flagged:

More than one top level block was found: Listicles, Listicle Item Title, List Item, List Item Content More about this.

But the reason is there are multiple children (grandchildren)

• Listicles - parent block
• List Item - individual item, child of Listicle
• List title & List Content -- children of List Item, grandchildren of Listicles

Is there a way to account for that? I'm not the only person who uses that methodology :)

[ryelle]

If you add a block.json for each block, and specify that ​parent relationship it should understand that — so List Item should have a property parent: [listicle]; list title & list content should have parent: [list item] (using the block names).

We can't get that info out of the JS or PHP files, unfortunately.

[ryelle]

collapsible-details-plugin.diff​ & collapsible-details-theme.diff​ switch away from the external help page, and add inline expandable help text to each check type. I think this gives a better dev experience, and will be translatable. The downside is needing a code change to update docs, but it seems that's how the other pages work.

There are still todo notes & other questions in this patch, but it's a first pass at adding descriptions to each check. I also collapsed all the check_block_json_is_valid issues into one section, since it can get wordy :)

Speaking of check_block_json_is_valid, I think it's a little aggressive — my test block uses script handles for editorScript, and it's flagging every one of them since they don't end in .js. Is that a requirement for finding the assets in the API?

I also had the idea to make a check for a well-formed block name— it should be alphanumeric, start with a letter, and contain one slash for namespace. If we ​agree that this should be the plugin slug, it could also check that. Otherwise a basic check that it isn't one of [ cbg, example, block, core ] seems good to me (other example prefixes I should add?).

[Ipstenu]

If you add a block.json for each block, and specify that ​parent relationship it should understand that — so List Item should have a property parent: [listicle]; list title & list content should have parent: [list item] (using the block names).

Yeah that's exactly what I did.

• listitem.js has parent: [ 'lez-library/listicles' ],
• listdt.js has parent: [ 'lez-library/listitem' ],

So all it's doing is saying "Hey, you have multiple items picked as parents" which ... yes? Could it not check if the item it picked as parent _also_ has a parent?

If not, it's no big, but we should have a note in the docs that says "grandchildren items may cause a false positive"

[ryelle]

Hmmmm, it should only be counting blocks that don't have parent set. And you've got these properties set in the block.json files? What plugin URL are you using to check this?

[Ipstenu]

Ah. I have the properties in the JS but NOT the block.json (we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out -- it needs to be like the readme.txt example)

I was checking ​https://plugins.svn.wordpress.org/listicles/trunk and NOW with whatever changes just went through it stopped telling me I had too many parents :) (all families are valid, Block checker!)

Which reminds me...

​https://wordpress.org/plugins/developers/block-plugin-validator/help/#check_block_tag should clearly explain "Your plugin needs the tag block" people had been using blocks (plural) because ... who knows.

[ryelle]

we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out

I agree, and I think it's finally stable enough that we can do that :) I added an example & more info to ​these docs, which we can link to from the error messages.

[tellyworth]

Replying to Ipstenu:

Ah. I have the properties in the JS but NOT the block.json (we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out -- it needs to be like the readme.txt example)

This is a great point. Where does the sample block.json belong?

people had been using blocks (plural) because ... who knows.

The plural blocks tag makes sense for plugins that contain multiple blocks. I added a check for the singular tag more as a suggestion.

[tellyworth]

In 10048:

Plugin directory: styles for collapsible details in block validator.

Props ryelle.
See #5303.

[tellyworth]

In 10049:

Plugin directory: show expandable details for validator messages.

Props ryelle.
See #5303.

[JeffPaul]

block plugin validator

[JeffPaul]

block validator help

[JeffPaul]

I've been testing the ​Block Plugin Validator the last two days with our ​Block for Apple Maps plugin and wanted to share the following feedback (note that I'm testing on Firefox 78.0.1.):

NICE TO HAVE

1. Widen the text field for "Plugin repo URL" so the entire helper text displays.
2. Link to plugin source for the specific revision (in the case of my sample plugin that would be to ​https://plugins.trac.wordpress.org/browser/maps-block-apple?rev=2338279) as this will allow inspection of the plugin code in comparison to the block validator results.

DESIRED

1. I see SUCCESS and a nice green-highlighted message so it looks like I'm all set, except the copy makes it sound like there are additional steps I need to take ("No problems were found. Your plugin has passed the first step towards being included in the Block Directory."). Only its not clear what those steps are. Having details on what happens next is helpful to understand if there's more I need to do or if its now sitting with some other team (PRT perhaps?).
2. There are WARNINGS and NOTES sections each with items called out with "More about this." links that in some cases don't hyper/anchor link to anything in particular. If there's help needed on copy for those links I'm happy to help draft those, but having something that is linked to for those would be helpful.
3. Its unclear if the WARNINGS are things I need to resolve in order for our block to appear in the Block Directory, but I'd assume things are ok as there are no "ERRORS" which would tell me I still have things to fix. If the WARNINGS need to be resolved, it would be helpful to know.
4. On the ​Block Validator Help page, the "Block scripts" section has copy that stops mid-sentence. Getting that clarified will be helpful.
5. In trying to find more details on what is needed for our block plugin to appear in the Block Directory I've searched for and found several different markdown pages on GitHub (​Share your Block with the World, ​Block Type Registration RFC, and ​Block Registration) each of which provide information that would ideally live in the ​Plugin Handbook as the canonical source of information where developers go to learn about crafting their plugins (in this case a block plugin). I suspect that for most plugin developers that it is not immediately obvious to find the right markdown file in the right branch in the Gutenberg GitHub repo to figure out what's needed for their block plugin to appear in the Block Directory.

[JeffPaul]

Oof, looks like updates happened while I was documenting my previous testing feedback. In the latest revision, I see another issue likely related to my testing in Firefox. Namely that the expand/collapse actions are not obvious and I only stumbled onto them by clicking on a row in the WARNINGS section. There is no visual queue in Firefox that those rows are clickable and that the feedback for each warning row appears once you click on each row.

[tellyworth]

Allow plugin committers to add or remove their plugins from the block directory

[tellyworth]

In 10051:

Plugin directory: add self-service support to the block plugin validator.

This lets plugin committers add or remove their plugins from the Block Directory.

See #5303.

[tellyworth]

In 10052:

Plugin directory: change button text from Validate to Check

See #5303

[tellyworth]

In 10053:

Plugin directory: send an email when plugin is added to the block directory.

See #5303

[tellyworth]

In 10054:

Plugin directory: queue an import when adding plugins to the block directory.

See #5303

[dd32]

In 10055:

Plugin directory: When queueing a plugin for import after being added to the Block Directory, pass the tags_touched property as an array.

See [10054].
See #5303.

[ryelle]

In 10057:

Plugin Directory: Improve block checker management section

Add a link to plugin code for committers, and change button style to use primary for "Add" buttons.

See #5303

[ryelle]

In 10058:

Plugin Directory: Update block.json file checks in block checker

Remove the json file check, since the same items are checked for in check_block_json_is_valid. Improve messages in check_for_block_script_files.

See #5303

[ryelle]

In 10059:

Plugin Directory: Skip info notices when gathering check_block_json_is_valid warnings.

See #5303

[ryelle]

@jeffpaul thanks for running through this! as you've noticed, most of the copy is still in progress. I added a link to the plugin code in r10057. It should always run the checker on trunk, but if you think linking to the exact rev would be helpful I can add that in. The "more about this" links have been removed in favor of the inline comments.

We definitely need better guidance for "what's next" if there are errors & warnings (only errors prevent adding to the directory). Where would you want to see that info?

[tellyworth]

That's odd about the lack of visual cue in FireFox @JeffPaul. I did notice the lack of triangle icons in FF, and thought I'd fixed that with the summary style addition in r10048. Perhaps I didn't build the CSS correctly when I committed that - can confirm that the triangles are missing again.

[tellyworth]

In 10062:

Plugin dir theme: bust cache for r10048.

See #5303.

[tellyworth]

I forgot to bust the cache, that's why. Triangles should appear now on expandable items in Firefox. It still could use a better visual cue, but that's something at least.

[JeffPaul]

I can confirm that Firefox mobile Version 27.0 (18428) correctly shows the triangle icons.

[ryelle]

In 10065:

Plugin Directory: Update duplicate block name check

Switches the check to flag an error if we find the block name in a different repo plugin, or a warning if the found block has the same author.

See #5303

[ryelle]

In 10066:

Plugin Directory: Add a check for block name standards

This will flag as errors any block names that would not register in the editor (using the same regex). It also checks for some common block starter content namespaces, to ensure names are unique across the repo (as warnings).

See #5303

[annezazu]

Came here to provide feedback about the language and general experience of the block validator. JeffPaul pointed out much of what was on my mind above though! In any case, here's what's on my mind as general enhancements:

• It would be great to have more links to documentation or places to get help. Right now, the experience can feel a bit like a dead end otherwise.
• Put priority on any "warnings" found so they show above any success message. It's odd to me how often there's both a message with warnings yet a "success" message. It's not clear to me if it's actually okay to ignore the warnings (more on this later)
• Make the distinction between separate warnings clearer. Right now, it's a bit confusing that some of the warnings have an arrow to read more and others.
• When reviewing block plugins with fatal errors, warnings, and the "problems were encountered" language it becomes really hard to know what to focus on. I'd recommend changing the visual of "problems were encountered" to make it more distinct from Fatal Errors. Right now, it looks like I need to resolve all three sections to proceed (warnings, problems were encountered, fatal errors) when in actuality I just need to focus in on the fatal errors to start.
• I might consider renaming "Warnings" to "Recommended Improvements" and "Fatal Errors" to "Blockers". This depends on whether warnings are actually blockers though!

This tool will help determine if a WordPress block plugin is suitable for inclusion in the Block Directory. Enter a SVN repository URL, Github URL, or plugin slug to check. Please note this is a work in progress.

To call out a specific example, it would be great to link to some wider context from the start. Perhaps even linking to this recent post for now would be helpful:

​https://make.wordpress.org/plugins/2020/07/11/you-can-now-add-your-own-plugins-to-the-block-directory/

[ryelle]

Here's an attempt at moving stuff around & adding more context: ​https://cloudup.com/cKNeb_LcIQj (nothing's committed yet, so it's just some result screenshots)

• Moved the overall result to the top of the "Result" section, so it's a little more obvious this is the outcome, and the rest are individual issues
• Tweaked the language on the "success" message since there might be warnings, and switched it to info-style if the plugin is already in the block directory.
• Renamed Warnings to "Recommendations"
• Added descriptions to error & recommendation sections

The pattern of having the individual check results use the error/warning/info notice style could also be updated if that seems overwhelming, thoughts?

Make the distinction between separate warnings clearer.

Do you have any suggestions? I thought about having all these details default to open, but I don't know if that would be too much information.

It would be great to have more links to documentation or places to get help.

I think these efforts are scattered & in-progress right now, but as they come together I agree, we'll want a better intro here.

[annezazu]

This looks a ton better already! Thanks for digging right in. Much easier to understand what actions need to be taken. Some additional feedback...

• In this screenshot ​https://cloudup.com/iSeHAh8aemA The language of "Your plugin has passed the first step towards being included in the block directory" is a bit confusing as my brain immediately goes to "What's the next step?". Without the UI having corresponding "step" related direction, it can feel like one's missing something. I might change this to something like "Your plugin has passed the checker tests and is ready to be included in the block directory."
• In this screenshot ​https://cloudup.com/iXgZw_lNR12 I might change the language from "They need to be addressed before your plugin will work in the Block Directory" to "They need to be addressed in order for your plugin to be included in the Block Directory" to make it clearer that the plugin cannot be added since the checker is meant to be part of the flow for adding to the directory.
• The language around recommendations can be a bit clearer. Perhaps "These are suggestions to improve your block. While they are not required for your block plugin to be added to the Block Directory, addressing them will help people discover and use your block."

Do you have any suggestions? I thought about having all these details default to open, but I don't know if that would be too much information.

I think what might help is that if each warning has more information so there is consistency in every warning having the arrow in front of it. That weirdly makes it easier to distinguish and creates a nice visual pattern. Agreed on default to open being too much.

[ryelle]

In this screenshot ​https://cloudup.com/iSeHAh8aemA The language of "Your plugin has passed the first step towards being included in the block directory" is a bit confusing as my brain immediately goes to "What's the next step?".

The section right under that notice is the next step— the "Add … to Block Directory" button (block authors will see this section as "Committer Tools", without the edit link). How do you think we can make that more clear?

[annezazu]

I think that button is quite clear. I just think the language around "steps" is going to cause confusion. Usually when there's mention of steps in a flow there's a nice list or a clear continuation of language (ie "Your final step is to.."). Without it, I just think that part might be confusing. This is a bit nitpicky ;) but clarity doesn't hurt.

[ryelle]

In 10071:

Plugin Directory: Remove *.asset.php check from block checker

This check only applies to a specific build process, and having more than one asset file is not inherently a bad thing.

See #5303

[ryelle]

More updates: I removed most of the "next step" language entirely, so that it's more clear that the actual next step is to click the button (and added a message to upload the plugin to wp.org if it's a github scan); added descriptions to all the errors. ​see screenshots here.

[JeffPaul]

@webfactory ​reported on the make/plugins post:

Block Plugin Checker has a broken link. When a check fails due to “No blocks found in plugin.” it links to ​https://wordpress.org/plugins/developers/block-plugin-validator/TUTORIAL which redirects to ​https://wordpress.org/plugins/search/developers/ which is probably not intended to happen

I think the intended link was ​https://wordpress.org/plugins/developers/block-plugin-validator/help/

If I test the checker for ​https://plugins.svn.wordpress.org/autoshare-for-twitter, in the "Fatal Errors:" section I see "No blocks found in plugin." and "Learn how to create a block." that links to ​https://wordpress.org/plugins/developers/block-plugin-validator/TUTORIAL/. I'm not certain that I know of an exact link for a tutorial at this point, but the ​make/plugins post is the closest I know of for now so maybe we use that link instead?

[Ipstenu]

I would say TUTORIAL should link to ​https://developer.wordpress.org/block-editor/ or ​https://developer.wordpress.org/block-editor/tutorials/create-block/

[ryelle]

In 10078:

Plugin Directory: Improve feedback on block checker page

This restructures the page so the flow is more clear, adds clarity to what counts as a blocker, and adds more details to all flagged issues.

See #5303

[tellyworth]

In 10079:

Plugin dir: refactor asset discovery code to be reusable.

Needed for block checker; see #5303.

[tellyworth]

In 10085:

Plugin directory: better asset detection in block checker.

This should make the block checker use identical guesswork to the importer when looking for block assets.

See #5303.

[tellyworth]

In 10088:

Plugin directory: add some helper functions to the block validator class.

See #5303.

[tellyworth]

In 10089:

Plugin directory: catch known problem PHP function calls in block validator.

Some PHP calls are likely indicators of a plugin that is not compatible with the Block Directory. This will produce a warning or error if they are found in a plugin.

See #5303.

[ryelle]

In 10090:

Plugin Directory: Add wp_add_inline_script to warning function list in block checker

See #5303

[ryelle]

I've uploaded a patch (5303-translations.diff​) that checks for wp_set_script_translations & warns if it's not found. It's not a block directory requirement, but I've come across a bunch of block directory plugins that have translations but don't add it— so the translations don't work. I'm just not sure if the block checker is the right place for a check like this, or if we should stick to ​improving documentation.

[ryelle]

r10101 & r10102 should be attached to this ticket (my mistake in the commits).

r10102 commits the 5303-translations.diff​ patch.

[tellyworth]

In 10104:

Plugin dir: catch PHP syntax errors when checking block plugins.

See #5303.

[tellyworth]

An incomplete stab at adding support for zip file uploads

[ryelle]

In 10109:

Plugin Directory: Skip composer framework files when checking size of block plugins

See #5303

[ryelle]

In 10110:

Plugin Directory: Fix spelling & grammar issues on block checker

See #5303

[JeffPaul]

I caught this question in the #core-coding-standards channel and wasn’t sure if this was an odd case we should consider with the checker?
​https://wordpress.slack.com/archives/C5VCTJGH3/p1596239699173000

I just ran my plugin through the new Block Checker and it's giving a Fatal Error with this reason: Found PHP call header(). This is likely to prevent your plugin from working as expected. It's referring to this code at the top of the plugin:

if (!defined('ABSPATH')) {

header('Status: 403 Forbidden');
header('HTTP/1.1 403 Forbidden');
exit();

}

It was in my understanding that this is always a good thing to put at the top of a plugin or theme in order to avoid direct access. Why would this be considered a Fatal Error ?

[dd32]

Replying to JeffPaul:

I caught this question in the #core-coding-standards channel and wasn’t sure if this was an odd case we should consider with the checker?

Just noting that this was handled in #meta ​https://wordpress.slack.com/archives/C02QB8GMM/p1596240707221700

The suggestion was to not use the header() calls but to keep the exit() if wanted.
Allowing header() to be used for other reasons would likely signify that a plugin really wasn't a Block Plugin so it really seems like banning it's use entirely is probably best.

[ryelle]

In 10111:

Plugin Directory: Add a total size check for block plugins

This will trigger a warning if the total size of the plugin exceeds 1MB. A larger plugin will cause a longer wait when inserting into the editor, which degrades the user experience.

See #5303

[tellyworth]

In 10118:

Plugin directory: check block plugins for multiple namespaces.

See #5303.

[ryelle]

In 10128:

Plugin Directory: Fix textdomain on block namespace check

See #5303

[tellyworth]

In 10171:

Plugin dir: catch more multi-block plugins.

This makes the block plugin checker more strict about plugins that contain many top-level blocks, which won't work in the block directory.

See #5303.

[tellyworth]

In 10172:

Block dir: make invalid namespaces an error.

See #5303.

[tellyworth]

In 10173:

Plugin dir: check for unique namespaces in the block checker.

This should permit block namespaces to be reused by the same author, but otherwise require that the namespace be unique.

See #5303.

[tellyworth]

In 10174:

Plugin dir: block plugins shouldn't register shortcodes.

See #5303.

[tellyworth]

In 10176:

Plugin dir: improve block checker handling of json errors.

Links to the json file when displaying an error.

See #5303.

[tellyworth]

In 10180:

Plugin dir: support zip uploads in block validator.

This also refactors the code to be a little less messy.

See #5303.

[tellyworth]

In 10181:

Plugin dir: add test button for plugin reviewers.

See #5303.

[tellyworth]

In 10182:

Plugin dir theme: update block checker styles.

See #5303.

[tellyworth]

In 10189:

Plugin dir: prevent false trigger of the Test button.

See #5303.

[tellyworth]

In 10203:

Plugin dir: only show test results when available.

See #5303.

[tellyworth]

In 10204:

Plugin dir: fix the Add/Remove buttons.

The Test button was interfering with admin users.

See #5303.

[tellyworth]

In 10210:

Plugin dir: filter out broken blocks from block search API.

See #5303.

[tellyworth]

In 10212:

Plugin dir: revert r10210.

Seems like it broke block search.

See #5303.

[tellyworth]

In 10213:

Plugin dir: add an email to alert block developers about test failures.

See #5303.

[tobifjellner]

@tellyworth typo in https://meta.trac.wordpress.org/changeset/10213
...that attempts to insert the block %3%s into a post...

[dd32]

In 10225:

Plugin Directory: Block Validator: Fix a string typo.

Props tobifjellner.
See #5303.

[JeffPaul]

TLDR: Can someone help check and see if there's a fix needed for the Block Plugin Checker for the apiVersion field?

Details:
Note that @richtabor ​may have found a logic error on the apiVersion check on the ​Block Plugin Checker as its throwing a block.json file error for his ​Markdown Comment Block plugin that the apiVersion property must contain a string value. However the recent ​Block API Enhancements post seems to show this as an integer. I'm guessing this was a code copy mistake mimicking the same as the other version (string|false|null) check.

[gziolo]

I opened PR against the Gutenberg repository that clarifies that apiVersion in block.json should be a number:

​https://github.com/WordPress/gutenberg/pull/33249

All core blocks define apiVersion with the number 2. We also used number in all examples in documentation and in the initial dev note ​https://make.wordpress.org/core/2020/11/18/block-api-version-2/.

[ryelle]

Yesterday, in r11097, I increased the error level of block.json issues, because most issues in that category will prevent block-plugins from appearing in the directory. The apiVersion was always a string, but it was considered a recommended change before. I agree this is incorrect and should be updated to a number.

We need to define the block type schema to validate against it - is there a core-defined schema that we could use to avoid these kinds of mistakes? (I also ​asked this in slack)

[ryelle]

Update apiVersion to number, use rest_validate_value_from_schema to validate json object

[ryelle]

It looks like I'd need to write a validate_number function to add number support to the validator, so I've tried an different approach in 5303.diff​ - removing all our custom validation in favor of rest_validate_value_from_schema. I think this does everything we want (and some, since it would support all the property definitions that the core schema validates), the only drawback is that it returns on the first invalid value, rather than listing them all.

[welcher]

I like to submit that the schema no longer be in the plugin but rather pulled from ​https://json.schemastore.org/block.json or, once it's ready, the new package being proposed to manage the schemas for Gutenberg - ​https://github.com/WordPress/gutenberg/issues/35927.

I'd be happy to open a new ticket if that makes more sense.

[ryelle]

Thanks for sharing that issue, a @wordpress/schemas package would be a great help here. Once that's released, we should use it here.