4.0 | Proposal: more consistent type handling of ruleset-set custom properties

[jrfnl]

Current Situation

Follow up on/Inspired by #704.

End-users can set the values for public properties on sniffs via the XML ruleset.

PHPCS receives these values as strings and includes special handling for a few situations:

Value in XML ruleset	Value set for the sniff property
'' (empty string)	null (null)
'null'	null (null)
'false'	false (boolean)
'true'	true (boolean)

As things are, this special handling:

• Is not being applied when 'null', 'true' or 'false' are not received in lowercase.
  I.e. a property value of 'False' will remain the string 'False'.
• Is not applied to the values of array elements being set on an array property.

It is also not applied to the value of array keys.

Proposal

Proposal - part 1

I'd like to propose to apply the special handling of property values more consistently.

Value in XML ruleset	Value set for the sniff property in PHPCS 3.x	Value set for the sniff property as of PHPCS 4.0
'' (empty string)	null (null)	null (null)
'null'	null (null)	null (null)
'false'	false (boolean)	false (boolean)
'true'	true (boolean)	true (boolean)
'NULL' (and all other case variations)	'NULL' (string)	null (null)
'FALSE' (and all other case variations)	'FALSE' (string)	false (boolean)
'TRUE' (and all other case variations)	'TRUE' (string)	true (boolean)

Proposal - part 2

I'd also like to propose to apply the same special handling to array element values as of PHPCS 4.0.

What will not change ?

I do NOT intend to change the handling of array keys. These will remain strings at all times.
Changing that could have far bigger breaking impact due to how PHP casts all array keys to integers/strings and duplicate keys are not allowed.

I also do NOT intend to add special handling for integer or float property values at this time.

Impact and timeline

Sniffs which explicitly expect string values for all array elements in a public property may break due to this change if the sniff only takes string values for array elements into account.

Sniffs may also contain special handling to work around the above listed short-comings of the property handling. Especially for array values, where PHPCS currently has no special handling, I can imagine that this sniff-specific special handling could break with this change.

So, with the above in mind, I'm proposing to make this change in the 4.0 release and not before.

Opinions ?

Am I overlooking anything ? Are there more things which could break with this change ? Or do you think this will more likely fix some obscure bugs for end-users ?

Please leave a comment with your thoughts.

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

[greg0ire]

What's the audience for part 1? People who mistakenly type False, or people who expect it to work? I think it wouldn't cross my mind to do that, so I'm a bit skeptical about part 1.

For part 2 however, I think it is a good idea.

By the way, is there currently a syntax that allows escaping that special handling? If there is, then it would be possible to warn users about this, and recommend them to migrate to the escaping syntax ahead of time.

[jrfnl]

What's the audience for part 1? People who mistakenly type False, or people who expect it to work? I think it wouldn't cross my mind to do that, so I'm a bit skeptical about part 1.

Both. While it may be uncommon, there are people who are used to using uppercase null/true/false everywhere (there's even a sniff to enforce it, though I'm not sure how much it is used nowadays, what with PSR2/12 recommending lowercase).

I can't remember having seen bug reports about this, but why wait for one ? More than anything, this is about the "principle of least surprise", why would one work while the other doesn't ?
If given the choice between documenting that only lowercase values work or fixing this, I think fixing this is the better option and will lead to fewer support requests.

For part 2 however, I think it is a good idea.

👀

By the way, is there currently a syntax that allows escaping that special handling? If there is, then it would be possible to warn users about this, and recommend them to migrate to the escaping syntax ahead of time.

There is no escaping syntax available and never has been AFAICS.

[greg0ire]

While it may be uncommon, there are people who are used to using uppercase null/true/false everywhere

Ok, I didn't know about that, but just because I haven't encountered them does not mean they don't exist I suppose. In that case, it seems fair to fix rather than document indeed 👍