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

Implement exclusion of rules for the check results #823

Open
ernilambar opened this issue Dec 9, 2024 · 8 comments
Open

Implement exclusion of rules for the check results #823

ernilambar opened this issue Dec 9, 2024 · 8 comments
Labels
Infrastructure Issues for the overall plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@ernilambar
Copy link
Member

Related to #769

In actual this is more advanced form of above issue.

For application of PCP checks to the existing plugin in the directory, we need a mechanism for excluding certain error codes in certain files or directories. This is required because it is not possible to apply all PCP rules to the existing plugins without breaking those.

Few cases:

  1. Exclude single error code in general:

Example:

mismatched_plugin_name
  1. Exclude error code in certain files:

Example:

PluginCheck.CodeAnalysis.Offloading
-  /folder-one/file-1.php
-  /folder-one/file-2.php
-  /folder-two/sub-folder/file-2.php
  1. Exclude error code in certain folders:

Example:

PluginCheck.CodeAnalysis.Offloading
-  /folder-one/
-  /another-folder/sub-folder/
  1. Excluding could also be combination of 2 and 3.

Example:

PluginCheck.CodeAnalysis.Offloading
-  /folder-one/file-1.php
-  /folder-another/
-  /folder-two/sub-folder/

A set of rules would be assigned for each plugin which will be reviewed and approved by the reviewer manually.

Issue is here for tracking and gathering ideas and feedbacks regarding the implementation.

Questions:

  1. In what format will we keep these rules?
  2. How to make rules extendable and future-proof?
  3. How will CLI/admin take these rules in the processing of the output?
@ernilambar ernilambar added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall plugin infrastructure labels Dec 9, 2024
@swissspidy
Copy link
Member

Also related: #310

@davidperezgar
Copy link
Member

davidperezgar commented Dec 9, 2024

Hello,
What I see from your questions.
1-2. Implies infrastructure. Maybe @felixarntz, @swissspidy, @mukeshpanchal27 could give more info. Maybe we could make an XML as PHPCS does?
3. My proposal could be to do as ERROR_LOW_SEVERITY. We could make a new type called ERROR_EXCLUDED/WARNING_EXCLUDED that is shown as that.

I hope you it helps.

@davidperezgar
Copy link
Member

Or I'm thinking to make a JSON with a structure to exclude folders/checks. That's a more modern way.

@swissspidy
Copy link
Member

I'd like to first understand the problem better before jumping into implementation details.

Is this something you as the plugins team want to do for every plugin? If a plugin author themself can disable all rules for all files then PCP would become useless...

If you do it for all the plugins, how would you manage this information? Store it in the database?

Do you want to be able to disable whole checks or individual error/warning codes?

Also, if you disable some errors for a file today because of some false positive, then nothing stops the plugin author from introducing more errors in that file in the future. How would you handle that?

@ernilambar
Copy link
Member Author

ernilambar commented Dec 9, 2024

I'd like to first understand the problem better before jumping into implementation details.

Is this something you as the plugins team want to do for every plugin? If a plugin author themself can disable all rules for all files then PCP would become useless...

Rules would be reviewed and approved by the reviewer.

If you do it for all the plugins, how would you manage this information? Store it in the database?

We are discussing with the meta team regarding this (no concrete decision is made) but yah possibly in the database.

Do you want to be able to disable whole checks or individual error/warning codes?

Exclusion format with excluding error codes with possibility of doing it for whole would be ideal.

Also, if you disable some errors for a file today because of some false positive, then nothing stops the plugin author from introducing more errors in that file in the future. How would you handle that?

Security and other essential rules wont be allowed to be excluded. Only rules which could break the existing plugin will be allowed. Plugin review team will prepare guideline regarding the allowed rules and reviewer will approve based on that. Non-breaking rules would be required to be fixed whether plugin is already published or not.

For example:
WordPress.NamingConventions.PrefixAllGlobals.ShortPrefixPassed now allows prefix with minimum 4 chars. We cannot enforce this rule to existing plugin which has 3 chars prefix. So this type of rules should be excluded.

@felixarntz
Copy link
Member

Security and other essential rules wont be allowed to be excluded. Only rules which could break the existing plugin will be allowed.

This is very vague. There are probably some "essential" rules that were not considered essential in the past, so those would "break" existing plugins too. Without a clear place where we draw the line, I can see this lead to more problems than it solves.

Which leads me to: What real problem does this solve?

I understand existing plugins will violate certain rules that weren't enforced until now, but why is it a problem to highlight those? Of course, if those were to be enforced, there would need to be a long transition period given during which the developers could update their code. But "hiding" problems just to flag less I find questionable, also for the other reasons @swissspidy shared.

From a technical perspective, if you want to start with fewer rules, then why not go with the existing options to only run specific --checks etc.?

@ernilambar
Copy link
Member Author

Which leads me to: What real problem does this solve?

There is plan to implement PCP for existing plugins also and if there are ERRORs with --error-severity=7 in the plugin, system will block releasing new version of the plugin.

Lets say WordPress.NamingConventions.PrefixAllGlobals is a blocker rule. Almost all plugins with 3 chars prefix will be blocked to release new version. Eg: woo, edd, etc.
So this exclusion will help in this case. This exclusion will be implemented specific to the plugin, not overall for all plugins.

This is not about hiding the error message. It is about scanning plugin in each new release of the plugin enforcing PCP rules without breaking the plugin.

@davidperezgar
Copy link
Member

What about of having a severity for updates? We could ask Meta to use other error severity, for example 6-5 for that. Maybe it could be a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants