Support PHPCompatibility testVersion config

[jrchamp]

After reading through the PHPCompatibility documentation, it seems that without setting the testVersion value, we are not getting the errors for backward-incompatible functionality. Specifically, functions, constants, syntaxes and classes that were added in newer versions were not being reported. It was only reporting forward-incompatible changes (functionality that had been removed in newer versions of PHP).

Fixes #205

[codecov-commenter]

Codecov Report

Merging #206 (d4e5e53) into master (f44997a) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #206      +/-   ##
============================================
- Coverage     82.99%   82.96%   -0.03%     
- Complexity      673      675       +2     
============================================
  Files            74       74              
  Lines          2099     2108       +9     
============================================
+ Hits           1742     1749       +7     
- Misses          357      359       +2     

Impacted Files	Coverage Δ
src/Command/CodeCheckerCommand.php	100.00% <100.00%> (ø)
src/Installer/Database/PostgresDatabase.php	73.80% <0.00%> (-3.70%)	⬇️
src/Command/CodeFixerCommand.php	100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jrchamp]

If you are happy with this change, it would be good to update the sample GitHub action file. Here's an example testVersion value for testing from PHP 7.1 through PHP 99.0: 7.1-99.0.

[stronk7]

Hi @jrchamp,

many thanks for the contribution!

Coincidentally... recently I was thinking about which could be the best way to implement the very same in moodle-local_codechecker and I had this annotated here:

1. Allow to pass that information via parameter. Pretty much what your patch here does.
2. If not information is passed:
   2.1. At very least, ensure that the current php version (PHP_MAJOR_VERSION & PHP_MINOR_VERSION ) is applied.
   2.2. Bonus: If environment.xml is available and we know current branch (we do, I think)... look for the PHP requirement and build a range with point 2.1 (when makes sense).

What do you think about that extra point 2 behaviour?

Then, regarding your current patch, that implements point 1 above, few comments...

• I'd suggest to use lower-case option name. Pretty much all the current options are that way, so surely it's good to follow the trend.
• It would be great if some example can be added to tests to verify that it's working as expected (haven't looked how hard that would be).
• Note that, right now, the master branch is the place where the v4.0 development is happening. So we'll have to backport this to the 3.x branch, to get the improvement there. I can do that, no worries.
• To add an example to the GHA/Travis dist files is ok. Just maybe we don't need it if we provide the default behaviour commented in point 2 above.

Let's discuss about this a little bit... ciao :-)

Edited: Grrr... too many wiki/markdown/confluence flavours for my brain. Sorry for the typos.

[stronk7]

Hi again,

just sharing a better and simpler idea that @andrewnicols came with while we were discussing about this.

Can you try if just setting this into the phpcs.xml.dist file of Moodle causes automatically for phpcs (PHPCompatibility) to start checking for those specific branches?

index fdb96780b7b..f58f478c8aa 100644
--- a/phpcs.xml.dist
+++ b/phpcs.xml.dist
@@ -1,4 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <ruleset name="MoodleCore">
+  <config name="testVersion" value="8.0-8.1"/>
   <rule ref="moodle"/>
 </ruleset>

It seems to be working ok here and, depending of the range I use... I get different PHPCompatibility reports.

If it works as expected... it's really the ideal place, because every moodle branch can come with the correct range specified in that existing file (since Moodle 3.11). I think that could replace the point 2 in my proposal above perfectly.

Of course, still, I think that we should create the new option (point 1) like your patch does so, if anybody want to use a custom range for any reason (say old branches, or only want to test against one php version each run, or ...) it can be done.

What do you think?

[andrewnicols]

https://tracker.moodle.org/browse/MDL-77458

Note: These can co-exist. We shoudl do the .phpcs.xml change but being able to override it per run is very useful still.

[stronk7]

It's cool if we get it working by default everywhere by the grace of MDL-77458 , yay!

And yes, still this PR makes sense, for anybody wanting to explicitly check for other ranges different from the "default" ones coming from core. So I just would suggest to consider/amend the four points commented above (maybe we don't need the 4th as far as, by default, it will "just work", @jrchamp and this can land too.

Yay^2 , ciao :-)

[jrchamp]

Okay, I've updated the command option to use the lower-case format. I had added this code to resolve 2.1:

// Default to the current version.
$testVersion = PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION;

I have removed that code, so that it does not overwrite the values that are being added by MDL-77458.

[stronk7]

Thanks @jrchamp , I'm looking to this, specifically to check if can add some unit test to cover it.

Other than that, it looks ok IMO. Will comment soon...

[stronk7]

Hi @jrchamp . I've added a commit to your branch, adding a few test to verify the new test-version option. Once they are passing I will:

• merge this, that goes to master (for incoming v4).
• backport to v3, so we can make a new release including this.

Ciao :-)

[stronk7]

Ok,

the "codecov-project" job above is failing, but it's because of some recent changes to PostgreSQL connection, required by Travis. So we can safely ignore it.

The important point is that the "codecov-patch" job is telling us that we have covered 100% of the code modified by the PR.

So, so far, all good, coverage-wise.

[stronk7]

For the records, this has been backported to the 3.x branch... will make a release soon... ciao :-)