MINIMUM_SUPPORTED_PHP is less than MINIMUM_PHP

+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -433,6 +433,14 @@ public function testAccessManager() {
+    $this->assertTrue((bool) version_compare(\Drupal::RECOMMENDED_PHP . '.0', \Drupal::MINIMUM_SUPPORTED_PHP, '>='), "\Drupal::RECOMMENDED_PHP should be greater or equal to \Drupal::MINIMUM_SUPPORTED_PHP");

This might not work if e.g. we recommend 8.1, but we need a minimum of 8.1.1 for a PHP bug fix. Still that's a hypothetical for now I guess, we can address that if it happens.

Otherwise +1 to the test for this to avoid getting it wrong again.

+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -433,6 +433,16 @@ public function testAccessManager() {
+    // Recommended is often just MAJOR.MINOR so add a very high patch version in
+    // order to test that it is consistent with \Drupal::MINIMUM_SUPPORTED_PHP.
+    $this->assertTrue((bool) version_compare(\Drupal::RECOMMENDED_PHP . '.99', \Drupal::MINIMUM_SUPPORTED_PHP, '>='), "\Drupal::RECOMMENDED_PHP should be greater or equal to \Drupal::MINIMUM_SUPPORTED_PHP");

Not that I actually expect PHP to ever have a patch version of 99, but isn't this test technically only valid so long as the recommended PHP version is MAJOR.MINOR.98 or less?

Also, what if RECOMMENDED_PHP does contain a patch version? Then this becomes a four-part version like 8.1.0.99. version_compare() doesn't seem to fatal or anything for that, and Composer supports versions like that, but it's certainly not the expected behavior.

If we do recommend a specific patch version is works as expected. I documented the fact I tested that in my previous comment.

Can we put this in the code comments though? (I still don't see where the issue says this. Edit: I'm pretty sure it doesn't.)

Also, I think the test could probably use .9999999999999 or something just to make it far less likely that the test will break? IIRC Composer or someone does similarly already. Still not ideal, but as a reminder Drupal 7 is on 80-something now so 99 is not outside the realm of possibility.

(Also there is an NPM package with a version of 1.0.30001271 in our dependency tree.)

Also it's possible for MINIMUM_SUPPORTED_PHP to only be two parts as well. See #3261447: Add an API for dynamically setting recommended and supported PHP versions based on known and predicted PHP release schedules which explicitly adds and tests that.

Patch looks good.

1. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -433,6 +433,30 @@ public function testAccessManager() {
   +    $this->assertTrue((bool) version_compare($minimum_supported_php, \Drupal::MINIMUM_PHP, '>='), "\Drupal::MINIMUM_SUPPORTED_PHP should be greater or equal to \Drupal::MINIMUM_PHP");
   +    $this->assertTrue((bool) version_compare($recommended_php, \Drupal::MINIMUM_SUPPORTED_PHP, '>='), "\Drupal::RECOMMENDED_PHP should be greater or equal to \Drupal::MINIMUM_SUPPORTED_PHP");
   

   version_compare() return a boolean value when the $operator parameter is used, which we are doing here. Therefor "(bool)" can be removed.

2. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -433,6 +433,30 @@ public function testAccessManager() {
   +    // RECOMMENDED_PHP and MINIMUM_SUPPORTED_PHP can be just MAJOR.MINOR so
   +    // normalize them so that version_compare() can be used.
   

   I cannot find where it is saying that MINIMUM_PHP must be of the type "Major.Minor.Patch". Can we add documentation for it and add a test that it is of the type "Major.Minor.Patch".

My points have been addressed.
For me it is RTBC.

+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -433,6 +433,30 @@ public function testAccessManager() {
+      preg_match('{^(\d{1,5})(\.\d++)?(\.\d++)?$}i', $version, $matches);

Is there any specific reason we're constraining the major version to be between 1 and 5 digits? Because six nines is what Composer uses, or a different reason?

I mean, having PHP version 100,000 is even less likely than having version 8.0.99. :P But as-is the 1-5 digits seems like a magic number. It might be good for the inline comments to explain where the 5-digit limit comes from (and that the normalization pattern with its six nines are from Composer's own internal comparisons).

Outside that, the improved test looks great to me. (The interdiff in #13 is an inter-interdiff but I figured it out after only momentary confusion.) Thanks!

It's just an inline comment. It's free. Documenting hardcoded integers is never a bad thing. Explaining why a test provides the specific coverage it does is never a bad thing. All it has to say is:
// The regex below is borrowed from \Composer\Semver\VersionParser::normalize().

...And we never have to wonder about it again.

(I'd add it myself but I'd prefer to be able to commit the patch.)

I could use a simple task today and saw this. Shortened the suggestion to be under 80 chars.

Random failures. Setting to NR.

The point of the regex string has been addressed.
Back to RTBC.

For me this is very much at the edge of things I would add a test for - we change these constants at most once or twice per year, usually a long time before tagging a release for the branch that changes them, and although we managed to get into a wrong state, it was picked up very quickly and easy to fix.

However, the test is already written and should stop this happening again, so..

Committed c2495d1 and pushed to 10.0.x. Thanks!