Test version logic

[jrfnl]

Before I start debugging/changing things around, I'd like to check what the intended behaviour is of the testVersion logic.

It was always my understanding that if you provided a specific version or version range, you'd only get the notices which were relevant for those versions.
And if you didn't provide a testVersion, you'd get all the notices no matter what, i.e. this would currently translate to the range 5.0-7.0.

In practice, that is not how it currently works.

The below comment is in the PHPCompatibility_Sniff abstract base class. The explanation in the comment is not in line with what my understanding was, but also not in line with how it currently works.

/* The testVersion configuration variable may be in any of the following formats:
 * 1) Omitted/empty, in which case no version is specified.  This effectively
 *    disables all the checks provided by this standard.
 * 2) A single PHP version number, e.g. "5.4" in which case the standard checks that
 *    the code will run on that version of PHP (no deprecated features or newer
 *    features being used).
 * 3) A range, e.g. "5.0-5.5", in which case the standard checks the code will run
 *    on all PHP versions in that range, and that it doesn't use any features that
 *    were deprecated by the final version in the list, or which were not available
 *    for the first version in the list.
 * PHP version numbers should always be in Major.Minor format.  Both "5", "5.3.2"
 * would be treated as invalid, and ignored.
 * This standard doesn't support checking against PHP4, so the minimum version that
 * is recognised is "5.0".
 */

This is what the functions currently return:

So my question specifically is what the intended behaviour is when no testVersion has been provided.

[MarkMaldaba]

If I recall, it was me who added this option and the related code, and therefore probably me who documented it too.

I don't have much time this week, but I'll try and look at it and respond properly as soon as I can.

[MarkMaldaba]

... however, to answer your specific question off the top of my head, the intended behaviour was probably "don't break backwards-compatibility for existing deployments which don't specify a value".

:-)

[jrfnl]

The thing what gets me is that it is not clear at all from the readme/user facing documentation that if you don't specify a testVersion you will:

• according to the documentation - not get any benefit from using the sniffs in the library at all "This effectively disables all the checks provided by this standard."
• in practice - only get notices when the check uses the supportsAbove() method.

[MarkMaldaba]

Sorry about the delay replying to this. I'm not sure if this reply is going to answer all your questions, but I've dug through the history and discovered this comment from my original commit of these functions, in revision 752feb0:

Previously, each class was repeating the same rather lengthy version check, to decide whether a sniff error should be reported (based on the specified testVersion), which makes the code hard to read, increases the risk of errors when editing (particularly when new sniffs are added) and makes the logic harder to change in the future (e.g. to specify a range of versions that you need your code to support).

I have therefore created a base class, called PHPCompatibility_Sniff, which all the other sniffs in the project now extend. This implements PHP_CodeSniffer_Sniff, so child classes no longer need to mention this. It adds two helper methods, easily available to all sniffs:

• supportsAbove($phpVersion) - returns true if the version being tested against is >= $phpVersion or if no testVersion was specified.
• supportsBelow($phpVersion) - returns true if the version being tested against is <= $phpVersion (returns false if no testVersion was specified).

The logic is identical to the two tests that were previously duplicated all over the place, but it is now encapsulated in a much more easy-to-manage way.

As you pointed out from the code, and as confirmed by the above commit message, the supportsAbove() test returns true when the testVersion is null, whilst the supportsBelow() test returns false.

This was intentional, in order to ensure no change in behaviour for installations that don't specify a testVersion, and it is therefore the documentation that was incorrect when I came to document the testVersion setting in commit 5834c92.

In effect, it means that if no testVersion is specified (and assuming the sniffs are implemented consistently) we report any and all deprecated/removed features (backwards-compatibility issues) but we don't report any new features (forwards-compatibility issues).

This could be considered desirable behaviour, or it could not. Either way the important thing is consistency and clarity with regards this setting when it is omitted.

There are probably four options:

1. Behave as currently (and update the documentation to explain this more clearly). There may be an argument for this if anyone finds it useful, as there would otherwise be no way to achieve this behaviour (all other options are achievable by explicitly setting an appropriate range).
2. Skip all tests, as per the current documentation (makes some logical sense, but not terribly useful).
3. Run all tests. This means the default is to assume you want your code to run on all PHP versions that we offer tests for. Good for completeness, and a logical default to use, but in practice probably not what many people want (as most developers don't go back as far as 5.0 in their support). That said, it encourages users to think about their support requirements, which is probably a good thing.
4. Assume some other minimum, but no maximum (e.g. we could default to testing compatibility for all PHP >= 5.3, if that were felt to be a sensible minimum). Might be good if there is some consensus about what this should be, but I suspect the obvious choice (peg it to the earliest PHP version that is still officially supported, updating it as this changes) is more restrictive than most users of this tool would like. If you are running the tool you presumably have a higher-than-usual interest in backwards-compatibility and probably need to support older versions than are still officially supported. And any other value is a bit arbitrary (and implies some kind of 'best practice' which I'm not sure it is our place to do).

My recommendation is option 3, as it is simplest to code and simplest to understand. I could also go for option 1 if someone put forward a compelling use-case for it (though arguably this would be better and more clearly handled by an additional 'compatibilityMode' argument with three possible values: 'forward', 'backward' or 'both', which could be used in combination with the testVersion setting).

Anyway, that's all a bit of a brain-dump - sorry for waffling on!

Thoughts?

[jrfnl]

@MarkMaldaba Thanks for you extensive reply. Appreciated.

I'd be all for 3, though I can now better understand the case for 1, which does make some sense too.

Additionally, I would suggest the following - partially based on #197 -:

• Let's add a docblock to each of the functions to clarify their usage.
• Let's have a think about the naming of the functions.

Concerning the function names:

They currently make perfect sense to me, but it took me a while to get them. So for someone who's just starting to contribute to the library or who contributes infrequently, they may not be as intuitive as they may seem for people who use them on a more regular basis. The docblock documentation could help already to make things clearer, but it couldn't hurt to at least have a think about the function names.

About the setting of testVersion:

Another thing I'd like to put out there would be the possibility to just set a minimum testVersion and leave the maximum open to auto-adjust to the available sniffs/PHP version.

A lot of people will use PHPCompatibility as part of their ruleset rather than run it separately from the command line. In that case, they could set the testVersion as part of their ruleset <config name="testVersion" value="5.2-7.0"/>, but they would need to adjust it each time a new PHP version came out of just set it to some silly non-existent value like <config name="testVersion" value="5.2-99.0"/>.
I can imagine we could possibly accept <config name="testVersion" value="5.3-"/> as a way to set the minimum version, but no maximum version and to translate that to array( '5.3', null) for use with the supportsBelow/Above() functions. (and the same visa versa for just setting a max version, no minimum)

[jrfnl]

Side note: I've just been running some tests with setting the test version via the config and it's not actually working at this moment, so the instructions I added to the readme are wrong for that.
I'll be fiddling with this a bit more to see if I can get it working and will send in a PR for adjusting the readme.

---

Update: got it working, I was using the wrong xml key <arg .. should be: <config ...

[MarkMaldaba]

I was thinking about names. How about mustSupportAbove()? Is that any clearer? Or does it need something more drastic?

I'm all for adding the ability for unbounded ranges:

• 5.3 = check it runs on 5.3 only. Equivalent to 5.3-5.3
• 5.3-5.5 = check it runs on all versions from 5.3 to 5.5, i.e. >= 5.3 and < 5.6
• 5.3- = check it runs on all versions >= 5.3
• -5.3 = check it runs on all versions < 5.4
• - = check it runs on all PHP versions. This is a logical deduction from the above, but is a bit of an odd outcome, as I would expect - to mean something like 'skip'!

Given that final point, would it be better to make it explicit. E.g. 5.3-max, min-5.3, min-max, etc.? We could also add a synonym all meaning min-max, for clarity.

[wimg]

This was implemented some time ago. Closing. Feel free to reopen if anthing's still missing

[MarkMaldaba]

This was implemented some time ago. Closing. Feel free to reopen if anthing's still missing

I think the documentation update was implemented, but there was ongoing discussion about whether to (a) rename the functions; or (b) update the logic, to work as per option 3 in my comment, above (#142 (comment)).

I don't know whether that means this ticket should be re-opened, or a new ticket (or tickets) logged - or, actually, that the current situation is fine and no further action is needed. Would be good to have @jrfnl's thoughts on this.

[jrfnl]

While the documentation is better now, the function names are still not as intuitive as could be.
I still notice myself thinking twice every time I need to use one of these functions, so while not the highest priority, I'm reopening it to allow for further ideas and discussion about possible improvements.

[jrfnl]

The function names were changed in PR #1555, so that part can be considered addressed now.