PSR12/OperatorSpacing: add perCompatible property to not check union type operator spacing in catch() statements

[klausi]

PSR12.Operators.OperatorSpacing: add support for PER 2.0 catch block spacing

Description

This PR introduces a new public property $perCompatible to the PSR12.Operators.OperatorSpacing sniff.

This property allows users to opt-in to PER Coding Style 3.0 behavior regarding union types in catch blocks.
According to PER-CS 3.0, the pipe operator | in catch statements should not be surrounded by spaces and should be treated as part of the type definition rather than a bitwise operator.

When $perCompatible is set to 3.0 (or higher), the sniff will enforce no spaces around the pipe operator within catch blocks. The default value remains 1.0, ensuring full backwards compatibility with PSR-12 behavior.

Suggested changelog entry

PSR12.Operators.OperatorSpacing: new perCompatible property to support PER-CS 3.0.

Related issues/external references

Fixes #660

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[jrfnl]

@klausi Thanks for setting up this PR.

First off, the change to treat multi-catch similar to union types for operator spacing was only made in PER 3.0, not 2.0. Please use the correct PER version.

Looking the PR over, one fundamental problem comes to mind: Operator spacing for multi-catch is now no longer checked at all for PER >= 3.0.

I don't think that's correct or desirable.

Now, to fix this, there are a number of options, all of which would impact the implementation of what you are doing in this PR.

1. Flag and fix spacing around the | operator for multi-catch in this sniff, but flag and fix to 0 spaces for PER >= 3.0.
   This would not break existing usage as this change in handling of multi-catch would be opt-in via the perCompatible property.
2. Create a separate sniff to handle spacing around the | operator for multi-catch.
   Such a separate sniff could either live in an external standard - in which case there will likely be a flurry of slightly different versions of such a sniff in multiple external standards.
   Or if the sniff would go into PHPCS itself, that sniff would also need to have the perCompatible property, so it would make more sense for this sniff to always bow out and defer to the separare sniff for multi-catch, but that would break existing behaviour.

There could even be more/other options. Happy to hear them.

Either way, I think we need to have a discussion about this first, before I comment on the code of the PR itself.

[jrfnl]

Oh and just noticed the LLM prompt section - please read the CONTRIBUTING GUIDE carefully. LLM generated PRs are NOT welcome at all in any form: https://github.com/PHPCSStandards/PHP_CodeSniffer?tab=contributing-ov-file#do-not-submit-ai-generated-prs

(also explains why my draft note of code changes to request is already way larger than it ever should be)

[klausi]

Sorry, totally missed the AI section in the contributing guidelines. I'll only submit pull requests I fully created myself in the future.

Thanks for pointing out the 2.0 vs. 3.0 version - I'll will update this PR accordingly (the summary description and the code).

As far as I understand it your proposed option 1 is exactly what I want to do here, the test case is demonstrating that. I will add one with setting the version to 4.0 as well to demonstrate code is flagged and fixed for higher versions as well.

[klausi]

Updated the code and the description above, ready for review!

[jrfnl]

@klausi Thanks for making those changes and sorry for making such a mess of the code view with all my remarks. Having worked with you before, I suspect this is largely due to the original setup having been created by AI... 🤷‍♀️

I hope the code review remarks help and are clear.

Also note that this PR needs a sister-PR in the documentation repo to document the new sniff property in the Customisable Sniff Properties wiki page.

[jrfnl]

See my remark about the fixer vs new lines in the test comments.

[klausi]

new lines are skipped now, as there is no standard defined for multi line catch() when you have 5+ exception types you don't want to compress in one line.

[jrfnl]

I understand the choice to leave handling of multi-line catch expressions out of this PR.

I do, however, believe clarification about this is needed from the FIG working group as - as things are at the moment -, the standard can also be interpreted as being defined and not allowing multi-line catch (as multi-line formatting is explicitly defined and mentioned for some of the other control structures and explicitly not mentioned as allowed for catch).

I suggest opening an issue in the PERCS repo.

[klausi]

For personal reasons I don't want to interact with the PERCS repository, but anybody feel free to go ahead and file an issue!

I assume the standards wants to be consistent with the veryComplex() example:

function veryComplex(
    array
    |(ArrayAccess&Traversable)
    |(Traversable&Countable) $input): ArrayAccess&Traversable
{
    // ...
}

and apply that the same way for catch() statements:

try {
    // nothing
} catch (
    Exception
    |RuntimeException
    |KlausiException
    |AnotherException
    |ItNeverEndsException $e
) {
}

[klausi]

Thanks a lot for your detailed review, I tried to address everything.

I will click resolve on the comments that I think are done, but let me know if anything is missing!

[klausi]

Sorry for all the notifications, but this should be ready for review again.

Also added documentation PR: PHPCSStandards/PHP_CodeSniffer-documentation#86

[jrfnl]

Hi @klausi, thank you for all those updates and changes. Looking good.

I've done another thorough review now and only found the following two small issues remaining:

1. To safeguard that the early return is in the right place (which it is), there should be a test with a bitwise or operator within parentheses, but not within a catch condition.
   This test is currently missing.
   Think: if ($error === E_ERROR | E_DEPRECATED) and/or function withDefault($param = MY_CONST_A | MY_CONST_B) {}.
   Obviously, this will need both a "valid spacing" and an "invalid spacing" variant of this to be added to the test file. This test may also need to be repeated for both the perCompatible 1.0 (default behaviour) as well as the perCompatible 3.0 block in the test file.
2. Below the new tests, the following still needs to be added to prevent confusion when contributors add new tests in future PRs:

// Reset property back to its default value.
// phpcs:set PSR12.Operators.OperatorSpacing perCompatible 1.0

Also note that I've earmarked this PR for the 4.1.0 release as it adds a new property. This means that I intend to merge it as soon as the 4.0.2 release has been published.

[klausi]

Thanks for the review, I expanded the test with 8 new test cases for valid/invalid combinations within parenthesis.

[jrfnl]

Thanks for that last update @klausi ! All good from me. Approved. Merge pending until the 4.0.2 release has been published.