Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add php 8.0 support #109

Open
scara opened this issue May 9, 2021 · 3 comments
Open

Add php 8.0 support #109

scara opened this issue May 9, 2021 · 3 comments

Comments

@scara
Copy link

scara commented May 9, 2021

Based on moodlehq/moodle-plugin-ci#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: scara/moodle-local_twittercard@9b8e324
b. add that setting only on those PHP versions requiring it: scara/moodle-local_twittercard@0a3bb44

/cc @stronk7

HTH,
Matteo

@stronk7
Copy link

stronk7 commented May 9, 2021

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
Copy link
Author

scara commented May 10, 2021

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
Copy link

stronk7 commented May 10, 2021

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants