Coding Standards: mark warnings that trigger CI failures as errors.

[peterwilsoncc]

The GitHub workflow for linting PHP currently fails for both warnings and errors, see this ​example run in which warnings error out.

As warnings trigger CI errors, I think it would be beneficial to set the phpcs.xml.dist file to bump the severity of all warnings to errors.

For developers using linting tools that default to showing errors only, this will ensure their editor/pre-merge script highlight anything that will cause the CI to fail.

https://core.trac.wordpress.org/ticket/57426

[peterwilsoncc]

I'm testing this in the linked pull request, currently in draft.

Warnings only trigger CI errors in the tests directory so this may be more complex than at first appearance.

I guess I want something like the following but I am not sure it is possible:

<rule ref="[as defined in the xml config file">
        <type>error</type>
        <exclude-pattern>/src/*</exclude-pattern> <!-- OR --> <file>tests/</file>
</rule>

[jrf]

@peterwilsoncc I'm not sure what you are trying to do here ? What is the problem you are trying to solve ?

PHCPS will exit with a non-zero exit code for both errors and warnings and, by default, all tooling integrating with this will recognize that correctly.

This behaviour can be changed (of course).

In the WP PHPCS workflow, it _is_ changed for the files in src by using the -n CLI parameter which effectively silences all warnings.

This was done for historic reasons: at the time the workflow was enabled, the src directory still contained over 6000 issues which needed developer attention (i.e. couldn't be auto-fixed without risk).
It was decided to give contributors time to go through these issues, but not let it block CI in the mean time.

Some of these issues are in actual fact errors, but have been downgraded in the WP Core phpcs.xml.dist file to warnings, precisely for this reason: to prevent CI failing (in combination with the -n), but to make sure that they still show up in IDEs.

For the test directory, it was easier, and much less risky, to clean it up, so that directory yields no warnings or errors, which is why the -n flag is not applied for the test run (and why there are two separate PHPCS runs instead of just one).

For those issues which were downgraded to warnings in the ruleset - if any of those are by now completely fixed in Core src, the downgrade in the ruleset should be removed.

There are other options to change the PHPCS behaviour, but unless I understand better what you are trying to achieve, it's hard to advise.

[peterwilsoncc]

Coding standards in Editor: alignment sniff as warning

[peterwilsoncc]

Coding standards in Editor: alignment sniff as error

[peterwilsoncc]

@jrf I've attached some images in the hope they'll be able to make it clearer.

If it is possible, I'd like to increase the severity of all coding standards failures in the test directory to error to match the behavior of the CI -- warnings cause the CI to error. Per tests-alignment-as-error.2.png​

I use VS Code with PHPCS set up via one extension or another. The default config is to display errors in the editor window, warnings are suppressed per tests-alignment-as-warning.png​.

While I use a custom config (tests-alignment-as-warning-with-custom-config.png​) to show warnings, they are quite understated with my extension so easily missed. However, for contributors not aware of the need to check for warnings nothing will show.

Reading through the annotated phpcs.xml file after posting this ticket, I am not sure it's possible so I'm happy enough if this ends up either a wontfix or a maybe later.

[jrf]

@peterwilsoncc

If it is possible, I'd like to increase the severity of all coding standards failures in the test directory to error to match the behavior of the CI -- warnings cause the CI to error.

That would require an adjustment to the Coding Standards handbook and Make posts to inform the decision.

The WordPress-Core ruleset is aligned with the handbook and throws errors for those things which are rules and warnings for (strong) suggestions.

Note: The last release of WPCS is not yet in line with recent updates to the handbook. This is being worked on and expected to be addressed in WPCS 3.0.0.

I use VS Code with PHPCS set up via one extension or another. The default config is to display errors in the editor window, warnings are suppressed per tests-alignment-as-warning.png

That sounds like a problem with the configuration of the VS Code editor. Not a problem with the ruleset.

However, for contributors not aware of the need to check for warnings nothing will show.

That sounds like a documentation issue about how to configure VS Code, not a problem with the ruleset.

The ​WPCS wiki contains links to tutorials about how to add WPCS to various editors correctly. If the existing (often external) tutorials aren't good enough or incomplete, I believe it would be a good idea to add new wiki pages with the correct information.
It could also be considered to add a section to the ​Coding Standards Handbook about configuring various IDEs.

Reading through the annotated phpcs.xml file after posting this ticket, I am not sure it's possible so I'm happy enough if this ends up either a wontfix or a maybe later.

To me, more than anything, this sounds like a dev education/documentation issue and not something which should be fixed in code/the ruleset.

[GaryJ]

PHPCS Errors and Warnings have different semantics. As well as Warnings being a strong suggestion, they can also be used when it's not possible to confidently determine if a bit of code is a violation or not i.e. it needs a human to verify (who can then either fix or add an ignore comment).

Nullifying those semantics by treating all violations as Errors, just because your editor setup doesn't show Warnings sufficiently well, doesn't seem beneficial for everyone else.

[peterwilsoncc]

To be clear, I'm not suggesting changes to WordPress-Core, rather the wordpress-develop xml config. That's why I am opening a ticket here rather than on the GitHub repo.

I understand errors and warnings are semantically different, however the CI treats warnings as errors in the tests directory so I think that needs to be reflected in the developer workflow. The semantic difference in the tests directory has been lost.