PHP_CodeSniffer icon indicating copy to clipboard operation
PHP_CodeSniffer copied to clipboard

PSR12.Operators.OperatorSpacing interprets pipe character in catch declaration as binary operator

Open pschultz opened this issue 1 year ago • 2 comments

Describe the bug

PSR12.Operators.OperatorSpacing produces false positives on type alternatives in catch clause.

Code sample

<?php

try {
} catch (A|B $exception) {
}

To reproduce Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs -s --standard=PSR12 test.php
  3. See error message displayed
FILE: /tmp/tmp.0QHXuAapol/foo.php
---------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------
 4 | ERROR | [x] Expected at least 1 space before "|"; 0 found
   |       |     (PSR12.Operators.OperatorSpacing.NoSpaceBefore)
 4 | ERROR | [x] Expected at least 1 space after "|"; 0 found
   |       |     (PSR12.Operators.OperatorSpacing.NoSpaceAfter)
---------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------

Expected behavior

No error, since the pipe character in the code is not an operator. This already works as expected for function declarations, i.e. this is fine:

<?php

function f(A|B $x)
{
}

Versions (please complete the following information):

  • OS: Fedora 53
  • PHP: 8.0
  • PHPCS: 3.7.1
  • Standard: PSR12

pschultz avatar Sep 12 '22 12:09 pschultz

This one is tricky to fix due to PSR12 and PHP itself.

The try/catch/finally example in PSR12 is this (https://www.php-fig.org/psr/psr-12/#56-try-catch-finally):

<?php

try {
    // try body
} catch (FirstThrowableType $e) {
    // catch body
} catch (OtherThrowableType | AnotherThrowableType $e) {
    // catch body
} finally {
    // finally body
}

Note the spaces around the pipe character.

This obviously pre-dates PHP8 union types, so the pipe character here has been treated as a bitwise OR in PHPCS because a union types concept didn't exist. PSR12 wants spaces around it, so PHPCS is functioning correctly as per PSR12.

With union types making it into PHP8, it would probably be more accurate to re-tokenize the pipe character in a catch to T_TYPE_UNION, but this breaks existing sniffs, including the PSR12 sniff that checks for whitespace around the pipe character.

So where I've landed on this is

  1. PHPCS is working correctly as per the PSR12 spec
  2. I'd like to reconsider this token in PHP 4, where backwards compatibility can be broken

Keen to hear people's thoughts on that.

gsherwood avatar Sep 12 '22 22:09 gsherwood

That's my bad, I guess. We encountered this issue after updating FriendsOfPHP/PHP-CS-Fixer, which removed the spaces, but I haven't double checked the standard.

As far as I'm concerned this issue can be closed.

pschultz avatar Sep 13 '22 10:09 pschultz