4.0 | Revise exit codes for PHPCS and PHPCBF commands

[jrfnl]

Repost from squizlabs/PHP_CodeSniffer#2898 by @gsherwood:

Version 4 is a good time to review the exit codes that both scripts produce to ensure that success cases all use 0 and that all failure cases have codes that make sense. Documentation in the wiki to describe the new exist codes should be an output of this work.

---

Additional info posted by @jrfnl:

Loosely related open issues:

• Disable report for phpcbf squizlabs/PHP_CodeSniffer#1818
• The "phpcs" doesn't use error exit code when invalid standard is given squizlabs/PHP_CodeSniffer#2412 (discoverability of availability of exit code related CLI arguments)

And some related closed issues:

• Return code of phpcbf on all errors fixed squizlabs/PHP_CodeSniffer#2954
• phpcbf returns exit-code 1 even if all errors were fixed squizlabs/PHP_CodeSniffer#3057
• When I run phpcbf with stdin, the exit code is "1" instead of 0 squizlabs/PHP_CodeSniffer#3367
• allow stdin fix to exit sucessfully squizlabs/PHP_CodeSniffer#3541

And the following open PR is also related:

• Add constants for exit return codes squizlabs/PHP_CodeSniffer#3696

---

Related remark posted by @nelson6e65:

Param --runtime-set ignore_warnings_on_exit 1 works on phpcs, but it does not for phpcbf.

---

Question by @dfelton:

Is there any documentation somewhere that lists all non 0 or 1 exit codes that exist, and the meaning of them?

Today was the first time I discovered an exit code of 3 is used for when and invalid standard is given. It'd be nice to be able to read a compiled list of what exit codes there are, and I cannot seem to find any information on the wiki on this topic.

Reply by @jrfnl:

I think this is what you are looking for: squizlabs/PHP_CodeSniffer#930 (comment)

[greg0ire]

0: no errors found
1: errors found
2: fixable errors found
3: processing error

It does not look like all situations are mutually exclusive. For instance, it's possible to have fixable and unfixable errors found during the same run. This should IMO be encoded as 1+2=3. If a processing error happens, 4 should be added, and each new situation should be associated with a power of 2, so that it's possible to tell more about what happened from the exit code alone.

[jrfnl]

@greg0ire I'm sure Greg had some idea in mind on where he wanted to go with this when he created the issue, but the above is all the info which is available.

[mbomb007]

Personally, I would like errors and warnings to have separate exit codes, so that a coding standards error returns 1, whereas warnings with no errors returns 2 or something similar. Otherwise you have to grep the output to determine what type of error it is.

This would be helpful for using PHPCS in GitLab pipelines, because exit code can be used to determine whether a pipeline is allowed to fail. GitLab allows pipelines to specify essentially a "pass with warning" state, and the pipeline stage shows as yellow (warning) instead of green (success) or red (error). I'd like an easy way to determine that from the exit code, such as 0 (success), 1 (error), 2 (warnings, no errors).

[paulshryock]

I'd love for PHPCBF to return exit code 0 when fixable issues are found and fixed and there are no unfixable issues found.

composer coding-standards:fix
> phpcbf
F... 4 / 4 (100%)



PHPCBF RESULT SUMMARY
-------------------------------------------------------------------------------
FILE                                                           FIXED  REMAINING
-------------------------------------------------------------------------------
/Users/User/Desktop/my-org/my-repo/src/php/MyClass.php         1      0
-------------------------------------------------------------------------------
A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE
-------------------------------------------------------------------------------

Time: 128ms; Memory: 6MB


Script phpcbf handling the coding-standards:fix event returned with error code 1

The above example shows 1 error found and fixed. So the outcome is a success; yet it exits 1 as though it failed.

[jrfnl]

Another situation which I believe should get its own exit code: "requirements to run not met". Also see #530

[jrfnl]

Some more input via issue #806:

@jrfnl posted:

For the record, there are currently four situations possible [when running phpcbf]:

1. Clean code base, no errors at all.
2. Dirty code base, all errors/warnings are auto-fixable.
3. Dirty code base, some errors/warnings are auto-fixable.
4. Dirty code base, no errors/warnings are auto-fixable.

These four situations are currently handled as follows:

Situation	Auto-fixable	Shows	Exit code	End result
1. Clean	N/A	"No fixable errors were found"	0	Clean code base
2. Dirty	All	Report with "Remaining 0" for each file	1	Clean code base
3. Dirty	Some	Report with "Remaining > 0" for some files	1	Dirty code base, non auto-fixable
4. Dirty	None	"No fixable errors were found"	0	Dirty code base, non auto-fixable

As part of #184, the exit code for situation 2 should change to 0.
And I'm not sure yet, but I can imagine that the exit code for situation 4, should possibly change to 1 ?

@peterwilson responded to this with

I think both of these changes make sense:

• exit 1: the code base is dirty
• exit 0: the code base is clean

[jrfnl]

Some interim thoughts on the exit code changes - nothing set in stone yet:

Interim proposal

Code scan, phpcs:

• 0 = clean code base
• 1 = issues found, auto-fixable
• 2 = issues found, non-auto-fixable
• 8 = processing error - blocking the actual run of PHP_CodeSniffer, like a parse error in an XML ruleset
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

As the exit codes would be cumulative, this includes:

• 3 = issues found, mix of auto-fixable and non-auto-fixable (1+2 = 3)

Fixer run, phpcbf:

• 0 = clean code base
• 1 = issues found, auto-fixable (only in combination with 4/failed to fix)
• 2 = issues remaining, non-auto-fixable
• 4 = failure to fix some files/fixer conflict
• 8 = processing error - blocking the actual run of PHP_CodeSniffer, like a parse error in an XML ruleset
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

As the exit codes would be cumulative, this includes:

• 5 = issues found, auto-fixable, but some failed to fix (1+4 = 5)
• 7 = issues found, mix of auto-fixable and non-auto-fixable, but some failed to fix (1+2+4 = 7)

Note: exit code 6 (2+4) cannot occur as you can't have a fixer conflict when there are no auto-fixable issues.

Explain phpcs -e:

• 0 = success
• 8 = processing error - blocking the actual run of PHP_CodeSniffer, like a parse error in an XML ruleset
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

Documentation phpcs --generator=...:

• 0 = success
• 8 = processing error - blocking the actual run of PHP_CodeSniffer, like a parse error in an XML ruleset (or in an XML doc file ?)
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

Config, i.e. --config-show, --config-set, --config-delete:

• 0 = success
• 8 = processing error: failure to set or delete (--config-set, --config-delete only)
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

Miscellaneous, like running -h (help), -i (info) or --version:

• 0 = success
• 16 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

As things are, exit code 8 and 16 are (currently) mutually exclusive with the other exit codes as PHPCS will hard exit early if the requirements are not met, same if a processing error happens.

How this addresses remarks left in this thread

I believe the above would address @greg0ire's concerns as per #184 (comment), as well as @paulshryock's remark: #184 (comment)

---

It does not address separate exit codes for errors and warnings as requested by @mbomb007 in #184 (comment)

I'm not sure distinguishing between errors and warnings for the exit code has more use cases than the single use case outlined in the comment.

In all other cases I've seen, this can sufficiently be handled via the existing ignore_errors_on_exit and ignore_warnings_on_exit options, which can either be set permanently via --config-set or for the current run only using --runtime-set. More info in the wiki

A separate issue should be opened to investigate @nelson6e65's remark:

Param --runtime-set ignore_warnings_on_exit 1 works on phpcs, but it does not for phpcbf.

If that claim turns out to be true, this should be fixed anyhow, but IMO such a fix would not necessarily need to be tied to PHPCS 4.0.

Opinions ?

I'd love to hear your thoughts on this, including whether I'm overlooking something/forgetting about a situation which needs to be handled via an additional exit code.

And while I have your attention, I'd also like to draw attention to open PR #807, which doesn't change the exit code, but does change the exit message when phpcbf succeeds and there are no violations left (at all). Please leave a comment on that PR if there's concerns about that change.

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

[jrfnl]

Just thinking: should there be a separate exit code for when a sniff throws an error/warning/notice/deprecation ?

At this moment, that would result in an error being added to the report for a file and the exit code being non-zero.

However issue #30 proposes to change this by making those non-blocking in a lot of cases, but that could mean that PHPCS would finish successfully, but the scan results may be incomplete.

So, what I'm really saying is: should there be a separate exit code for an incomplete run ?

[jrfnl]

Another thought/consideration which may need discussion:

PHPCS 4.0 will contain quite some changes in what is supported.
Some examples:

• The old manner of setting an array property on a sniff via a comma-separated string is deprecated and support will be removed in PHPCS 4.0.
• Sniffs not complying with the naming conventions will no longer be supported (4.0 | Formally stop support for sniffs not complying with the PHPCS naming conventions #689).

In practice, that means that in PHPCS 4.0:

• A warning will be thrown when an old-school setting of array property is seen and the property will not be applied.
• A warning will be thrown when a sniff not complying is requested to be run and the sniff will be ignored.

In other words: there will be situations where a PHPCS/PHPCBF run can succeed, but the results may not be as expected as certain directives were ignored.

What exit code should be used for this ? Should this exit with 0 and display the above mentioned warnings ?
Or should an extra exit code be introduced (probably 1 with everything else moving up), which indicates exactly that "success, but results may not be as expected/intended" ?

[schlessera]

Looks good to me for consistency and generally makes sense! 👍

What I would love to see added is a flag that allows me to decide whether remaining non-auto-fixable issues after a phpcbf run would stop the CI system. So, basically have a flag like --ignore-non-auto-fixable that would turn the exit code 2 on a phpcbf run to an exit code 0.

This way, the CI run using phpcbf could only focus solely on whether "auto-fixing" worked as expected and then go on and assemble a pull request or similar, even if the code base is not 100% clean still.

[schlessera]

Regarding the following:

there will be situations where a PHPCS/PHPCBF run can succeed, but the results may not be as expected as certain directives were ignored.
What exit code should be used for this ?

In this case, I think something like a --strict flag (or with better naming if available ;) ) would make sense, to distinguish between: "succeed if it is probably fine" versus "succeed only when it is guaranteed to be 100% correct".

[fredden]

I like the idea of consistency between the two invocations of this tool (ie, phpcs and phpcbf). I think that the current list of suggested exit codes (and the fact that they're additive, so can communicate a number of issues simultaneously) seems sensible. I particularly like that exit codes 1 & 2 & 3 are for code quality problems, and exit codes 4+ are for PHP_CodeSniffer/sniff problems; this makes it clear whose responsibility it might be to fix certain issues.

I like the idea that we will be able to identify if a code-base is clean or not based on the exit code for phpcbf. (The current exit codes there are... less useful.)

Thinking through the use-cases for phpcbf, I get the following list. This isn't intended to be an exhaustive list, but instead it's what I could come up with today while writing this down.

• git pre-commit hook - Being able to determine if the code is "clean" or not (after fixing whatever if fixable) makes sense. A pre-commit hook could optionally run phpcs to list out the remaining problems, but wouldn't need to like it does right now.
• quality control in CI/CD - Either phpcs is used to determine if there are any problems, leaving a human to do something about them; or phpcbf is used and its changes (if any) are committed automatically back to the repository. The exit code for phpcbf in this case can usually be ignored, as the logic is more interested in any changed files.
• during development - Running phpcbf as part of the development flow (either directly or via a scripted process) can be quite helpful. The exit code in this case can often be ignored, but if it shows if the code-base is "clean" or not, then a construct like phpcbf --cache || phpcs --cache --colors -s would return much quicker for codebase which has been auto-fixed as it would only need to be scanned once.

[jrfnl]

I've just had a discussion with @fredden about this proposal in combination with the question I posed in #858 (errors vs warnings).

To prevent future changes which may affect the exit codes having to wait until the next major release, it may be a good idea to leave some room for additional exit codes.

With that in mind, I propose to change the original proposal for the exit codes to this:

• 0 = clean code base
• 1 = issues found, auto-fixable (only in combination with 4/failed to fix)
• 2 = issues remaining, non-auto-fixable
• 4 = failure to fix some files/fixer conflict
• 8 = reserved for future use
• 16 = processing error - blocking the actual run of PHP_CodeSniffer, like a parse error in an XML ruleset
• 32 = reserved for future use
• 64 = requirements for running not met (i.e. minimum PHP version doesn't comply, missing required extensions)

This would allow for making potential additional changes while in the 4.x major and would prevent those changes having to be deferred to the 5.0 major.

[paulshryock]

• 0 = clean code base

Does this mean if fixable issues are fixed, we'll still get a non-zero exit code? Or does this mean that the code base is clean after any fixing has completed?

Since the purpose of code beautifier is to make changes to the code (while the purpose of code sniffer is to check for issues), it feels like code beautifier succeeds if it makes changes, hence my reasoning for wanting it to exit 0.

And to explain a little further, I like to use code beautifier in a pre-commit git hook. So if I'm about to commit some changes, I'd like it to fix any fixable issues for me automatically and exit 0 so that the commit can complete.

[zdziejowski]

Does this mean that with version 4.0, it will be possible to use the ignore_warnings_on_exit 1 parameter for phpcbf, since the proposed exit code 1 would interrupt a pre-commit hook or pipeline on Git?

[jrfnl]

• 0 = clean code base

Does this mean if fixable issues are fixed, we'll still get a non-zero exit code? Or does this mean that the code base is clean after any fixing has completed?

@paulshryock That depends. Per the above proposal, if all fixable issues are fixed and there are no non auto-fixable issues left, you'd get exit code 0.
If there are non-auto-fixable issues left, you'd get exit code 2.

I'm inclined to implement @schlessera's suggestion for having a --ignore-non-auto-fixable option though, which - when used - would in that case still give you exit code 0.

And to explain a little further, I like to use code beautifier in a pre-commit git hook. So if I'm about to commit some changes, I'd like it to fix any fixable issues for me automatically and exit 0 so that the commit can complete.

I understand the desire for a zero exit code in that situation, but there will be others who will want phpcs to exit with a non-zero code if there are still non-auto-fixable things left to prevent those getting into a code base.

Having the exit codes as proposed + the flag would allow to satisfy both use-cases.

Does that answer your question ?

[jrfnl]

Does this mean that with version 4.0, it will be possible to use the ignore_warnings_on_exit 1 parameter for phpcbf, since the proposed exit code 1 would interrupt a pre-commit hook or pipeline on Git?

@zdziejowski Your question is unclear to me.

[paulshryock]

if all fixable issues are fixed and there are no non auto-fixable issues left, you'd get exit code 0.

Amazing, that's exactly what I hoped for. Thank you!

Having the exit codes as proposed + the flag would allow to satisfy both use-cases.

Does that answer your question ?

Yes, 100%. Everything described sounds great to me.

[zdziejowski]

@jrfnl Now everything is clear to me:

if all fixable issues are fixed and there are no non auto-fixable issues left, you'd get exit code 0.

That's fine with me. That's why I asked about the parameter ignore_warnings_on_exit 1. In that case, we don't need it. Am I right?

[jrfnl]

if all fixable issues are fixed and there are no non auto-fixable issues left, you'd get exit code 0.

That's fine with me. That's why I asked about the parameter ignore_warnings_on_exit 1. In that case, we don't need it. Am I right?

@zdziejowski You may or may not need it. Non auto-fixable issues can be either errors or warnings. That behaviour is unchanged.