`phpcbf` returns exit code `0` when errors that can't be automatically fixed are found

Question

`phpcbf` returns exit code `0` when errors that can't be automatically fixed are found

johnbacon opened this issue a year ago · 2 comments

While configuring CI with GitHub Actions, we observed that duster fix --using="phpcs" always returns an exit code 0 even in case of failures, causing checks to pass incorrectly.

The root of this behavior lies in the fix method where phpcbf's return code is used, which also appears to be always 0. This creates confusion... for instance, checks pass when env() helpers are used in place of config().

We propose introducing a configuration option to govern the exit code or, alternatively, adjusting the default behavior to align with the linting response when used as a GitHub Action. This enhancement would provide a more intuitive and accurate CI behavior. Thoughts appreciated!

Answer 1 · 2023-07-20T21:53:54.000Z

I didn't know the best way to implement this. PHPCS uses separate tools for fixing/linting. When you run fix not all the rules are tried because it allows rules that cannot be fixed automatically. My thinking was that if you run fix you want it to only fix and report success/failure on if it was able to fix.

I might agree with you here though. It might be better to run fix and lint and fail if either of them fail. Rules that cannot be fixed automatically or you do not want to fix can be added to the config to be excluded.

Is this what you were thinking?

Thanks for reporting, I'll think about it for a bit.

Answer 2 · 2023-07-24T14:47:18.000Z

I might agree with you here though. It might be better to run fix and lint and fail if either of them fail. Rules that cannot be fixed automatically or you do not want to fix can be added to the config to be excluded.

Is this what you were thinking?

That's exactly what we were thinking!

I know PHP-CS-Fixer has long debated on GitHub about what constitutes a 0 exit code regarding fixing. Perhaps the debate around this means it would be better as an option, but I also know that too many options aren't a good thing.

It does seem that anyone using Duster for its intended purpose would want to know when the fix command returns unfixable errors, though, particularly regarding major problems like the env() example.