mudrd8mz / moodle-tool_pluginskel

Generator of Moodle plugins skeletons
https://moodle.org/plugins/tool_pluginskel
Other
51 stars 46 forks source link

Add php 8.0 support #109

Open scara opened 3 years ago

scara commented 3 years ago

Based on https://github.com/moodlehq/moodle-plugin-ci/issues/94 it's time to address it into the skeleton too 😉.

Moodle 3.11 and above will support PHP 8.0 according to MDL-70745. Besides, a new PHP setting requirement has been added too: max_input_vars = 5000 (MDL-71390).

Different paths on how to address the php.ini requirement can be adopted: a. add that setting on any PHP version of the matrix: https://github.com/scara/moodle-local_twittercard/commit/9b8e324e60333591904bedd876ca75ec016c6e65 b. add that setting only on those PHP versions requiring it: https://github.com/scara/moodle-local_twittercard/commit/0a3bb44cb5a080515943fd8e9c2a6c47a581eb1f

/cc @stronk7

HTH, Matteo

stronk7 commented 3 years ago

In our docker php images we have added it for 7.3, 7.4 and 8.0 images. Why those 3? Because the are the ones supported by Moodle 3.11 and up.

Also note that for sure all 311 and master executions need it, at very least to get complete phpunit runs working. There are some environment tests that will fail (no matter it's a recommendation for 7.3 and 7.4 and a requirement only for 8.0). All them will fail that test. This is a 7.3 run against 311 missing the variable:

There was 1 failure:

1) core_environment_testcase::test_environment with data set #30 (environment_results Object (...))
Problem detected in environment (custom_check:max_input_vars), fix all warnings and errors!
Failed asserting that false is true.

/var/www/html/lib/tests/environment_test.php:93
/var/www/html/lib/phpunit/classes/advanced_testcase.php:80

By comparison, in core's Travis (under peer-review now, see MDL-70900) and GHA (landed last week, see MDL-71390) we are setting it always (for 311 and master branches).

So, surely, the easiest us to apply for it globally and done, only putting things in individual jobs where there is a difference (keeping them simpler).

That's my feeling, at very least.

BTW, also, be noted that for 8.0... surely you'll need also to add the xmlrpc-beta extension (if you want to get complete phpunit runs passing). Again, take a look to MDL-70900, because it's being added there for core.

Ciao :-)

scara commented 3 years ago

TNX @stronk7!

Also note that for sure all 311 and master executions need it, at very least to get complete phpunit runs working.

Do you mean even for any plug-in code? It looks like not required but for core code and for those plug-ins using big forms - which could be here "any" since it should be a generic working skeleton.

HTH, Matteo

stronk7 commented 3 years ago

Yeah, yeah. Normally we don't execute complete runs and, in any case, they aren't controlled by the plugin own CI stuff. I ended mixing stuff because I've spent the last days fighting with php80 elsewhere.

Note I use to run here my plugins as part of complete runs too (to verify that privacy and other aspects are ok), but yes, those aren't under plugins CI umbrella but core's one.

Anyway, all we need to ensure is that the site is installable by CI and done, yep. And only requirement, so far, is the max_input_vars one, the php-xmlrpc is optional. I still think that globally setting it is better, after all, future php versions will need it, yes or yes and it doesn't harm to have it on older ones.

Ciao :-)