4.x PHP_CodeSniffer\Util\Common::$allowedTypes structure changed without documentation

[InvisibleSmiley]

Describe the bug

4.x PHP_CodeSniffer\Util\Common::$allowedTypes used to be list<string>, now it's array<string, string> (due to it being a copy of the new ALLOWED_TYPES constant). In other words, the key type changed from non-negative-int to non-empty-string and of course all key values changed as well.

The change is not documented in current version of the developer upgrade guide.

Since the property is public static, this potentially affects usages anywhere, not just in subclasses and read contexts.

Context analysis

The change was probably made with good intentions, to micro-tune performance (replace in_array by isset).

The fact that it is a BC break (that either needs to be documented or avoided) was probably missed.

Expected behavior

The change is either (partly) reverted or properly documented.

Versions

PHP_CodeSniffer version	4.x branch

Confirmations

• I have searched the issue list and am not opening a duplicate issue.
• I have read the Contribution Guidelines and this is not a support question.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer. (does not apply)

[jrfnl]

@InvisibleSmiley Could you please explain why you believe this is a BC-break ? I mean, the values was the only useful thing before and those values are still available as values, so any looping through the values or use of in_array() will still work, just like before, so I cannot see the BC-break (only an enhancement).

[InvisibleSmiley]

The problem is not with usages similar to those that were in place before in PHPCS code (like in_array and loops) but with many other kind of usages one could imagine. The property is public static so anyone can access it, even without an object reference - which on one side is of course the reason why it has been defined as such (to be referenced by any PHPCS class), but also allows any outside code to use it in ways that neither of us can fully predict.

Just some ways this could result in breaking external code:

• direct access of items by (numeric) ID
• using list() or array destructuring
• using call_user_func_array()
• array unpacking (...) in combination with function/method calls (where string keys are interpreted as targeting named parameters)

You may say those will hardly be used but it takes only one person to be affected to receive negative feedback. Having lots of experience with changelog and migration documentation I'm really only trying to save you from that here.

As I said, you don't have to revert anything. Adding a note to the documentation in the proper place might be all that needs to be done here.

[jrfnl]

@InvisibleSmiley Well, yes, reverting is not on the table anyhow. I do appreciate you opening an issue about this though and I'm quite happy to add an entry to either the changelog or the upgrade guide, even though my imagination is clearly coming up short as I do find the examples given pretty contrived and I can't think of any legitimate situation in which the usages you mention would come into play. Either way, I'm on the road at the moment. When I'm back, I'll add something to the upgrade guide.

If you have a textual suggestion, please drop it in this issue.

[InvisibleSmiley]

I'd suggest to simply add the following to the text under "Replace with":

(note: both arrays are now associative)
or
(note: keys are now strings instead of integers)