Add an API for dynamically setting recommended and supported PHP versions based on known and predicted PHP release schedules

I like it! Very elegant 😊

#14.1: TIL — pretty much nothing in core does that though… so I am really confused about this statement.

#14.2 +1, that's what I meant, sorry for not making it more clear.

I think this looks good, but I'd like somebody else to confirm RTBC before marking it that :)

Is there any value in mentioning the class name in the __construct description? For example in IDEs that information is already available:

Based on that, I'm wondering if the documentation could be just Constructs a new object.. Regardless of this, we need to also update the class description since it's using the class name without the namespace.

I am a bit out of time. Will review more later.

Resolved few threads of MR.

Whew, reviewing this was quite painful because of so many tiny commits 😅 I like my (public) commit units to be one semantical change, and not literally every step along the way.

What made this worse: GitLab is so painfully slow that I've literally forgotten what I'd previously read by the time the commit loads! I ended up not reading every commit in detail after a while, and instead just making sure it was touching either PhpRequirements or PhpRequirementsTest, so I could just review those two files in detail instead.

Found a number of things in PhpRequirements, marking Needs work for that. It feels very close though!

@Wim Leers, I might be trying to teach a fish how to swim, but a tip to help on your pain would be to navigate by, lets say, 10 by 10 commits and run git diff HEAD~10..HEAD > temp.diff each time, so you don need to check each commit individually.

Yep :) Reviewing locally is the only solution. What I was saying in #29 is that the whole point of merge requests is that it's made easier/faster to do reviews in the browser. But GitLab's poor UX does not actually achieve that.

EDIT: thanks for teaching me that expression btw! 😄

Oh, okay, that makes sense! 😅

Review

I used git diff 973ba2539764ec2036e1c2627efec7e2e2c62f1c HEAD -M5% to see what changed because neither the GitLab UI nor the default git diff behavior are helpful (they show a file being deleted and a new one being created).

Changes Lauri made:

1. \Drupal\Core\PhpRequirements moved to \Drupal\Core\Utility\PhpRequirements — makes sense, given that this class is not going to be used like \Drupal\Core\Link or \Drupal\Core\Url or even \Drupal\Core\DrupalKernel 👍
2. Zero state left in the PhpRequirements class. I could not put my finger on it during my earlier reviews, but now that I see the before vs after, I think this really simplified things! 👍 With no more run-time validation, but instead there is a test for that, which will still prevent invalid values for the hardcoded PHP EOL dates. Things are just as hard-coded as before, but just simpler to interact with and understand.
3. There is now actually more validation: one extra edge case is handled: two PHP versions EOL'ing on the same date requires the older version to be listed before the newer version. 👍
4. No changes to the absolute heart of this change: the getMinimumSupportedPhp() function. @xjm's elegant solution is untouched 😊
5. The diffstat speaks for itself: 7 files changed, 124 insertions(+), 387 deletions(-)

All my concerns on the merge request were related to understandability — those are now all addressed.

Tests: passing even if they seem not to

100% of tests pass … except a few Nightwatch ones, which somehow cause DrupalCI to choke and not even report on test failures 🙈

In other words, #3281863: Nightwatch tests failing >50% of test runs on PHP 7.3 in 9.4.x and 9.5.x, as well as PHP 8.1 on 10.0.x strikes again:

 ✖ Tests/toolbarTest
00:46:02.599  – Change tab (5.719s)
00:46:02.599    Timed out while waiting for element <#toolbar-item-user-tray> to be present for 5000 milliseconds. - expected "found" but got: "not found" (5176ms)
00:46:02.600        at Object.Change tab (/var/www/html/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js:55:13)
00:46:02.600        at processTicksAndRejections (node:internal/process/task_queues:96:5)

I inspected https://dispatcher.drupalci.org/job/drupal_patches/130221/consoleFull and confirmed that 100% of tests are passing except for the random fails that are being looked at in #3281863.

Conclusion

🚀

Adding credit for @alexpott for some pairing/reviewing on this earlier today.

This needs a 10.x MR/patch.

Added a patch for Drupal 10.0.x.

const DRUPAL_MINIMUM_SUPPORTED_PHP doesn't exists in file core/includes/bootstrap.inc, so changes for this file is not valid now.

Here's a Drupal 10 patch that also removes \Drupal::MINIMUM_SUPPORTED_PHP and removes usage from \Drupal\Tests\Core\DrupalTest::testPhpConstants.

Reverted the commit. Also updated the release note 😇

Looks great!

Committed 2448cd3 and pushed to 9.5.x. Thanks!
Committed 11207f9 and pushed to 9.4.x. Thanks!
Committed 3412b7b and pushed to 10.0.x. Thanks!

Backported to 9.4.x because it is helpful to have deprecations in 9.4.x and the rest is new API so not risky,

Seems like fails are caused by private visibility of $drupalMinimumPhp and $phpEolDates properties. ReflectionClass::getStaticPropertyValue() throws exception if a property is not public.

Ah, yes, it's a PHP bug, which seems like was fixed in 2020, but still exists in PHP 7.3.

In the test we can replace

    $reflected->getStaticPropertyValue('phpEolDates')

with

    $reflected->getStaticProperties()['phpEolDates']

and

    $reflected->setStaticPropertyValue('drupalMinimumPhp', $drupal_minimum_php);
    $reflected->setStaticPropertyValue('phpEolDates', $php_eol_dates);

with

    $prop = $reflected->getProperty('drupalMinimumPhp');
    $prop->setAccessible(TRUE);
    $prop->setValue($drupal_minimum_php);
    $prop = $reflected->getProperty('phpEolDates');
    $prop->setAccessible(TRUE);
    $prop->setValue($php_eol_dates);

or something like that.

Here's the patch with my proposed fix for D9.4 and D9.5.

I see that tests for D10 with PHP 8.1 were successful previously, so no need for this workaround.

Committed to 9.5.x, cherry-picked to 9.4.x after confirming it was ok with @xjm given the impending beta release.

Reapplied the original patch to 10.0.x

This change has a consequence discussed here : https://www.drupal.org/project/drupal/issues/3324058
Perhaps an adaptation to be made? Lower the level to just warning and not error?

In any case, I think the opinion of the core commiters is welcome 😉