Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin Review Team: Output for WP.org form #441

Closed
bordoni opened this issue Mar 20, 2024 · 18 comments
Closed

Plugin Review Team: Output for WP.org form #441

bordoni opened this issue Mar 20, 2024 · 18 comments
Assignees
Labels
[Team] Plugin Review Issues owned by Plugin Review Team [Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@bordoni
Copy link
Member

bordoni commented Mar 20, 2024

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.

@bordoni bordoni added the [Type] Enhancement A suggestion for improvement of an existing feature label Mar 20, 2024
@bordoni bordoni added this to the 1.1.0 milestone Mar 20, 2024
@bordoni bordoni self-assigned this Mar 20, 2024
@felixarntz
Copy link
Member

@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
Copy link
Member

@davidperezgar davidperezgar added the [Team] Plugin Review Issues owned by Plugin Review Team label Jun 21, 2024
@davidperezgar
Copy link
Member

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

@davidperezgar davidperezgar self-assigned this Jun 26, 2024
@dd32
Copy link
Member

dd32 commented Jun 26, 2024

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
Copy link
Member

Thanks, we will take care of that.

@davidperezgar
Copy link
Member

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

@joemcgill
Copy link
Member

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
Copy link
Member

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

@davidperezgar
Copy link
Member

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

@joemcgill
Copy link
Member

@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
Copy link
Member

dd32 commented Jul 23, 2024

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
Copy link
Member

ernilambar commented Jul 23, 2024

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
Copy link
Member

davidperezgar commented Jul 23, 2024

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
Copy link
Member

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

@davidperezgar
Copy link
Member

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.

@davidperezgar davidperezgar removed this from the 1.1.0 milestone Aug 16, 2024
@davidperezgar davidperezgar added this to the 1.2.0 milestone Aug 28, 2024
@dd32
Copy link
Member

dd32 commented Sep 18, 2024

#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
Copy link
Member

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

@swissspidy
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Team] Plugin Review Issues owned by Plugin Review Team [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants