Plugin Review Team: Output for WP.org form

[bordoni]

We need a way to output a simplified version of the table of checks, by passing a couple of params so we can hook into the WP.org output with an error.

No false positives are permitted here, we will only output things that are required to pass. Which will be handled on #440 by @barrykooij.

---

More details to come on this.

[felixarntz]

@bordoni Similar to #440, I'd like to better understand what this issue is about so we can support the effort. Are you referring to the tables with the check results in WP Admin? or WP-CLI?

[barrykooij]

Related to this https://meta.trac.wordpress.org/ticket/7556#ticket

[davidperezgar]

We have asked Meta and @dd32 has replied us with the necessary information needed for them:

A single PHP function that can be called
Return value of said function should be 100% reliable as a “This submission should be blocked, 100%“.
Return value of said function can return structured data, such as array( 'pass' => true, 'warnings' => [ "Looks like you're doing XYZ, that's not great." ] );
The plugin can…

• run external tools if needed
• shouldn’t be opinionated
• should run within 5-10 seconds absolute max
• must 100% be only bundling trusted and vetted code

[dd32]

Additional requirements:

• The "single PHP function" can be a WP-CLI command if needed. The return value would then be the exit code and/or a parsable (json?) CLI output.
• It must accept an arbitrary path, such as /tmp/plugin-to-test. The plugin will not exist within WP_CONTENT_DIR.
• It cannot:
  • modify the filesystem in any way - modifications in /tmp/ is fine.
  • modify the database in any way - plugin check itself can save/store data if needed, but it cannot modify existing data like users, alter the table prefix, install WP, etc all that fun stuff.
  • execute the plugin-to-test's PHP code at all
  • I believe ^ means it'd need to fully opt-out of any "runtime" tests.

I'll also add, the above 5-10 seconds max is negotiable :) We mostly need consistent runtime, and as fast as possible obviously. Larger plugins will take long, that's fine, we just need to be mindful that it's running in a HTTP request and not a async job.

edit: clarified the modify database request.

[davidperezgar]

Thanks, we will take care of that.

[davidperezgar]

@joemcgill how do you suggest best approach from requests asked by Dion?

[joemcgill]

I think the best approach would be to extend the WP CLI command that is already included in the plugin.

Currently, the plugin only supports running tests against installed plugins, so we'll need to make it possible to run checks against a plugin folder in an arbitrary path. Given that requirement, @dd32 is correct that only Static Checks will be able to be run in this context, which I expect will be fine.

The CLI Command accepts a --format argument to control the output that is returned as either table, CSV, or JSON formats. You can also pass --ignore-warnings so only errors will be returned. If the list of checks categorized for the plugin repo only include static checks, which only report errors for issues that should block submission, I'm curious if the presence of any errors returned by the CLI command would be sufficient as a pass/fail signal, or if the command needs to also return a specific error code as well?

[joemcgill]

I've created #478 to address the arbitrary location requirement.

[davidperezgar]

Ok, I've made changes. What do you think about this PR?

[joemcgill]

@davidperezgar I'm not sure what changes you're referring to, can you provide more details of what you want feedback on in the comments of the PR you're referencing?

[dd32]

I'm curious if the presence of any errors returned by the CLI command would be sufficient as a pass/fail signal, or if the command needs to also return a specific error code as well?

Error codes aren't needed, we just need a solid way of saying "This submission should be rejected, because X".

If that's parsing the JSON / CLI output, that's fine, it just needs to be stated what it is.

If --ignore-warnings means that any errors returned are a 100% "Do not allow submission" that's totally fine.

[ernilambar]

Hmm, then may be we don't need to introduce new --format in the command. We can use like this:

wp plugin check plugin-slug --severity=8 --ignore-warnings

Severity 8 used here as an example of 100% sure checks only.

Related #479

[davidperezgar]

Hello,
But our main objective is to help developers to develop better and more secure. We actually give two kind of results and pass. Errors and Warnings. I think that we should show warnings, that the developer could work on that, and obviously it won't count as a pass fail.

If that's parsing the JSON / CLI output, that's fine, it just needs to be stated what it is.

Yes, actually in this branch is does.

[davidperezgar]

We're waiting some feedback from meta team. CC: @dd32

[davidperezgar]

Hello,
I've updated the code with the severity level and wporg format. I've made within iterative for all formats, but creating an array for errors and warning after severity filter.

I've removed pass as is useless.

We can discuss what's your opinion.

[dd32]

#478 is useful, however it appears that all checks assume the folder name from which the plugin is loaded into is the plugin slug.

This causes warnings when the folder doesn't match the path, for example, an uploaded ZIP might extract into phpkvyfGo8TAssp if it doesn't contain a folder matching the slug within the archive.

Action needed: WordPress.org needs a way to tell Plugin Check what the plugin slug is.

A dedicated format as in #480 isn't exactly useful IMHO, and I've expressed that previously:
#480 (comment) The JSON format works just fine.

Readme parser warning detected: low_usage_tags
Looks like the plugin-check plugin is using an older Readme Parser which included that warning, which is no longer valid. As it's part of the Readme Validator instead.

Ideally we'd not run checks that duplicate WordPress.org checks here.. but as long as it duplicates the plugin directory exactly it's not much of an issue.

That however brings up the next point:

vendor/afragen/wordpress-plugin-readme-parser/class-parser.php

That's being loaded by plugin-check even on WordPress.org, that shouldn't be happening. The WordPress.org file should take precedence. I guess the plugin-check auto-loader is simply being hit first.

I'm going to go as far as to say; Please don't use that composer package at all. It's not namespaced, that's a non-starter for deployment on WordPress.org

[davidperezgar]

Thanks @dd32 . We'll take care all of it...

[swissspidy]

Ideally we'd not run checks that duplicate WordPress.org checks here.. but as long as it duplicates the plugin directory exactly it's not much of an issue.

That however brings up the next point:

vendor/afragen/wordpress-plugin-readme-parser/class-parser.php

That's being loaded by plugin-check even on WordPress.org, that shouldn't be happening. The WordPress.org file should take precedence. I guess the plugin-check auto-loader is simply being hit first.

I'm going to go as far as to say; Please don't use that composer package at all. It's not namespaced, that's a non-starter for deployment on WordPress.org

We can handle that in #328 👍

Use the existing class from WordPress.org if running on dotorg, else use a prefixed/namespaced version.