Open bordoni opened 3 months ago
@bordoni @barrykooij Curious to learn more about this, can you provide a bit more context on what this is about? How could we possibly ensure there are no false positives? Maybe I'm misunderstanding the issue description.
@felixarntz This issue is about identifying what phpcs rules are 100% blocking for new submission. These should not have any false positives.
An example of a 100% blocking check is DisallowShortOpenTag
. We never accept this, no exceptions and no false positives. This issue is to identify all rules that are like this so we can use these rules as a first step in the plugin upload process.
@barrykooij Thanks, makes sense!
Is the idea to put those rules into a new PHPCS config once they're identified? Or a new check group? How would those rules technically be annotated / grouped so that only those would run in the new submission process?
@felixarntz I'm not sure yet, whatever works best. I'm currently leaning towards creating a set (check group?) of "blocking" PHPCS rules and if any of these fails, a submission is blocked. Another thing we need to in mind is that we're going to expand the checks to other form of checks than PHPCS, like a readme parser.
I think it would be good to also show this in the "regular" PCP wp-admin result, so a developer can see that the current version of the plugin contains blocking issues.
I'm currently creating a meta trac ticket to discuss how to run only these checks in a new plugin submission. Related to this is also #441
@barrykooij
I'm currently leaning towards creating a set (check group?) of "blocking" PHPCS rules and if any of these fails, a submission is blocked. Another thing we need to in mind is that we're going to expand the checks to other form of checks than PHPCS, like a readme parser.
In that case a "check category" probably makes most sense. For instance, we could add a "Plugin Submission" or "Plugin Repo (blocking)" category to https://github.com/WordPress/plugin-check/blob/trunk/includes/Checker/Check_Categories.php#L48 and assign the relevant checks to that category.
Alright. I've created a list of checks and PHPCS rules that I think should be included in the first version.
Checks
Code_Obfuscation_Check
File_Type_Check
Localhost_Check
No_Unfiltered_Uploads_Check
Plugin_Header_Text_Domain_Check
PHPCS RULES based on phpcs-rulesets/plugin-review.xml
Generic.PHP.BacktickOperator.Found
Generic.PHP.DiscourageGoto.Found
Generic.PHP.DisallowShortOpenTag
Generic.PHP.DisallowAlternativePHPTags
WordPress.Security.PluginMenuSlug
WordPress.DB.RestrictedClasses
WordPress.DB.RestrictedFunctions
Generic.PHP.ForbiddenFunctions
WordPress.WP.DeprecatedClasses
WordPress.WP.DeprecatedFunctions
WordPress.WP.DeprecatedParameters
WordPress.DateTime.RestrictedFunctions
WordPress.WP.DiscouragedConstants
WordPress.WP.DeprecatedParameterValues
I'm still in doubt about WordPress.WP.AlternativeFunctions
.
In that case a "check category" probably makes most sense. For instance, we could add a "Plugin Submission" or "Plugin Repo (blocking)" category to https://github.com/WordPress/plugin-check/blob/trunk/includes/Checker/Check_Categories.php#L48 and assign the relevant checks to that category. I've taken a look and I agree this would be a clean way to add this. I can create a new ruleset xml file and create a new check that uses this new ruleset.
The only thing I'm not happy about in this solution is that we don't want to have this be a publicly new check category. Ideally, I see this included in the Plugin Repo category, perhaps as a new Type of check result. Now Error is the highest find type, maybe we can introduce a level above this like Blocking. This way we can make it clear to people using the plugin that the submission will be blocked in the current state.
@felixarntz
One way we could address this problem is by implementing a "severity" system, as discussed in https://github.com/WordPress/plugin-check/issues/479.
With that in place, we could set some guidelines for what would cause a check to meet different severity levels and make sure all blocking checks are configured with a severity level above a certain threshold (e.g. 8). WP VIP has a page dedicated to interpreting severity levels for their WordPress-VIP-Go sniffs that could be referenced for inspiration.
As we talked last week @barrykooij will own this task to making sure that we have either:
That is important to enables us to have it be part of the submission on WordPress.org, which should enable us to reduce the Queue and maintain it at a good level.
Currently there is no way to appeal specific items that might be a false positive so this is a must.