4.0 | Removed feature support: error or warning ?

[jrfnl]

In light of #857 and #184 (comment)...

In PHPCS 4.0, support will be dropped for a number of previously (partially) supported features, like JS/CSS only sniffs, sniffs not complying with the naming conventions, setting array properties using the old comma-separated string format etc.

Some related issues: #689, #693, #694, #740, #799

Most of these features are already soft deprecated and in the last PHPCS 3.x major, these features will get deprecation notices, in as far as possible (hard deprecation).

Support for these features will be removed in PHPCS 4.0.

The question I'm running into now: how to handle this for end-users in PHPCS 4.0 ?

The choice is basically between throwing an error, which will stop the PHPCS run with a non-zero exit code, or to show a warning and continue running PHPCS, but the results may not be as expected.

Some examples:

• Use of a sniff which doesn't comply with the PHPCS naming conventions
  Should such a sniff be ignored by PHPCS with a warning ? Or should PHPCS stop running when such a sniff is included via the ruleset ? (error)
  A sniff being ignored, in practice means that some rules will not be enforced and that the results of the PHPCS run may not be as expected as some errors may not be reported.
• Use of a sniff which doesn't explicitly implement the Sniff interface
  Should such a sniff be ignored by PHPCS with a warning ? Or should PHPCS stop running when such a sniff is included via the ruleset ? (error)
• Use of the comma-separated string format for array properties
  Should the property setting be ignored with a warning ? Or should PHPCS stop running and throw an error ?
  An array property setting being ignored can lead to significant differences in the result of a sniff.
• Requesting a sniff which has been removed in PHPCS 4.0
  While quite some sniffs will be removed in PHPCS 4.0 (JS/CSS/MySource), in principle, this situation is already handled in the Ruleset and currently would throw an error.
  If the other situations above would throw a warning and ignore the sniff, should this "Referenced sniff %s does not exist." error be changed to warning and ignore the sniff instead ?

Opinions ?

Originally, I intended to make these warnings, but now I'm not so sure that's the right way to handle this and I'm currently leaning to letting these all be errors. However, I'd very much like some second opinions on this.

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

[michalbundyra]

@jrfnl I'd go with errors as well. It is a new major and better to fail hard than just let people get stuck with the old crap and 'only warnings'.
External libraries would need to do something anyway to support v4, so IMO it's better to hard fail, especially as all these cases were already deprecated before. But yeah, that's just my opinion and people might have a different one. Thanks

[GaryJones]

Hopefully the external sniff packages don't already claim to work with PHPCS ^4, which means they should already struggle to get it installed, but assuming they did, it would be good to make the user warning / error as descriptive as possible - make it clear that the issue lies with the sniff creator and not with the user's setup or something in their PHPCS config / code being tested.

[jrfnl]

Hopefully the external sniff packages don't already claim to work with PHPCS ^4, which means they should already struggle to get it installed, but assuming they did, it would be good to make the user warning / error as descriptive as possible - make it clear that the issue lies with the sniff creator and not with the user's setup or something in their PHPCS config / code being tested.

@GaryJones External standards which follow the PHPCS (naming) conventions should have no problems with these changes (which were all announced separately in their own issues). If you follow PRs in this repo, you will be able to give feedback on the messages which will be added soonish.

The question in this ticket is particularly about the error level/behaviour - should PHPCS stop running when it encounters use of a feature which is no longer supported ? Or should PHPCS continue to run, but with the risk of the results not being complete ?

[fredden]

I think all of these cases should have a non-zero exit code. It looks like 32 is the next number in the list to assign to a meaning, so that could be used here.

Use of a sniff which doesn't comply with the PHPCS naming conventions
Use of a sniff which doesn't explicitly implement the Sniff interface

Warning. Either because we are strict about the naming conventions and therefore throw a "the sniff does not exist" message and carry on, or because we are lenient to this problem. Changing the name of a sniff is a breaking change, and standard / sniff authors may not be ready for a breaking change at their end.

Use of the comma-separated string format for array properties

I think this should be a warning for phpcs and an error/blocker for phpcbf. Extra noise for a phpcs run is acceptable, as one of the lines of output would identify the problem. However, we cannot be sure that phpcbf would behave "correctly" according to user expectations. It's possible that some sniffs only fix a problem in one direction, so I think it's safer to stop phpcbf before making any code changes which might be difficult to undo.

Another option is to refuse to load the sniff and treat it the same as other cases where a sniff wasn't loaded: warning.

Requesting a sniff which has been removed in PHPCS 4.0

This should have the same behaviour as any other sniff that does not exist. I don't think we need to track a list of sniffs which used to exist at some point in the past. This situation is already something which might come up and tools like ComposerRequireChecker exist to solve this. Let's not create a maintenance burden for ourselves going forward. If we have a list of sniffs / standards which have been removed, then it follows that we should surface this functionality to downstream ruleset maintainers, which isn't a feature which sounds like fun to maintain.

[jrfnl]

In light of @fredden's response, let me try to make the background behind the original question a bit clearer.

At this moment, PHPCS has basically two states (in this context):

1. There are blocking errors in the CLI args, config and/or ruleset and PHPCS cannot run.
2. No problems found in CLI args, config and/or ruleset, PHPCS has all the information it needs to start a scan.

This would not change if use of any of the removed features in PHPCS 4.0 would become an error .

However, if use of a removed feature in PHPCS 4.0 would become a warning, we would now end up with three states:

1. There are blocking errors in the CLI args, config and/or ruleset and PHPCS cannot run.
2. No problems found in CLI args, config and/or ruleset, PHPCS has all the information it needs to start a scan.
3. Problems found in CLI args, config and/or ruleset, but we are ignoring those with a notice/warning to the user. PHPCS can start a scan, but the scan results may not be as expected (sniff not found => scan incomplete, string value setting of array property => scan may yield incorrect results)

It's this introduction of a third "state" which I'm concerned about.

I believe that if we introduce a third state "results may not be as expected", we also need to have a good think about how to handle this exit code-wise and whether or not a user should get the ability to influence the exit code for that state.

Think:

• If the default would be that config/ruleset notices/warnings do not influence the exit code, have a --strict option to make the build fail on config/ruleset warnings/notices.
• If the default would be that config/ruleset notices/warnings would influence the exit code, have a ignore-...-for-exitcode option (needs a good name) along the same lines as the existing ignore_errors_on_exit and ignore_warnings_on_exit` config options to prevent builds failing on config/ruleset warnings/notices and knowingly accept the risk of incorrect results.

All in all, I think introducing such a third state needs a good design and that now is not the time.

Let's for the first iteration in 4.0 go with errors. We can then revisit this in a later 4.x minor if necessary when the "three state" handling has been properly thought through.

The proposed update to the exit code proposal with the extra "reserved for future use" slots would ensure to allow for this.

[jrfnl]

I'm going to close this ticket now. The PHPCS 4.0 changes related to the changes in the Ruleset class, which inspired this question, have all been merged now.