[3.2.3 BC break] New error reporting added in MultipleStatementAlignment

[Majkl578]

This code worked fine in 3.2.2:

<?php

$indentation = str_repeat(self::INDENTATION, $indentationLevel);
$longestKey  = array_reduce(array_keys($value), function ($k, $v) {
    return (string) (strlen((string) $k) > strlen((string) $v) ? $k : $v);
});
$maxKeyLength = strlen($longestKey) + (is_numeric($longestKey) ? 0 : 2);

Now reports an error in 3.2.3:

 3 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space (Generic.Formatting.MultipleStatementAlignment.NotSame)
 4 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 3 spaces but found 2 spaces (Generic.Formatting.MultipleStatementAlignment.NotSame)

As much as I understand that it could be considered as a one assignment group, introducing new error for this in a bugfix release is a BC break.

[jrfnl]

The fact that it previously wasn't being reported was a bug - see #1870, so why shouldn't it have been in a bugfix release ?

[Majkl578]

Because new errors being reported are BC breaks and i.e. break CI builds.

[gsherwood]

Because new errors being reported are BC breaks and i.e. break CI builds.

That's not how PHPCS works. The main job of PHPCS is to detect and report coding standard violations in your code. So when something needs to be fixed, the reason is because PHPCS is either:

1. Not reporting an error that it should be
2. Reporting an error that it should not be

There are cases where internal code changes are made to solve issues that are not outwardly visible, but most bug fixes solve one of the two points above.

That means that almost every bug fix either:

1. Removes an error that was previously being reported, as it was not valid
2. Adds a new error for code that violates a standard, but PHPCS was not reporting it

Bug fix #1870 falls into category 2, where the sniff was incorrectly seeing your example code as two different code blocks due to the semi-colon in your closure. It was a pretty clear bug in the end, so it's been fixed in a bug fix release.

Does that explain this issue a bit better?

[Ocramius]

@gsherwood this makes a lot of sense, so I think we'll just have to deal with this minor instability.

[Majkl578]

@gsherwood Thanks for your reply.
It makes sense, but I think there should be a warning banner somewhere in README or something saying basically what you explained here.
Currently your README suggests to install either squizlabs/php_codesniffer=* (unbound) or squizlabs/php_codesniffer=3.*. Formally speaking, 3.* is semver constraint, so any typical consumers will asssume semantic versioning so that 3.123 won't break things uses on 3.0, that is not the case here.

Although I still think #1870 could've been fixed without introducing new error reporting in bugfix release and leaving it at least to next minor (i.e. remove incorrect reporting, leave out the fixed behavior for next minor). Tracking breaking changes in bugfix releases is nightmare for consumers.

[gsherwood]

Formally speaking, 3.* is semver constraint, so any typical consumers will asssume semantic versioning so that 3.123 won't break things uses on 3.0, that is not the case here.

That's exactly what I'm going for. I see this as a pretty clear bug fix. The sniff was written before closures and anon classes existed and it just never got support for them.

If I didn't fix bugs, PHPCS would obviously report the exact same errors for the exact same code forever, but then it would never improve its error detection or future PHP version support.

Although I still think #1870 could've been fixed without introducing new error reporting in bugfix release

In this particular case, it looks to me like the originally reported code is basically the same structure as the code you've posted here, so I don't know how I can report an error for one and not the other. I think the only thing I could have done is add a config option to allow people to tell PHPCS to ignore closures and anon classes, but you still would have had to change your ruleset to enable this option if you wanted that code to remain valid.