Remove support for PHP 5 in Drupal 8.8

+++ b/core/lib/Drupal/Component/Assertion/Handle.php
@@ -5,37 +5,19 @@
   /**
-   * Registers uniform assertion handling.
+   * Ensures exceptions are thrown when an assertion fails.
    */
   public static function register() {
     // Since we're using exceptions, turn error warnings off.

Can this be deprecated?

Note that the logic browser testing paths for the warning on DRUPAL_MINIMUM_PHP vs. DRUPAL_MINIMUM_INSTALL_PHP with the warning should be left in place; we will need them again for dropping support for other PHP versions.

Re #2

We should open an issue to discover if we still need the PHP workarounds in \Drupal\Component\Utility\Rectangle

That's not a workaround, it's a way to allow determining the size of a picture when rotated in ::transformDimensions based only on the starting width/height i.e. without invoking GD functions.

Actually there would be #2918570: Drupal\KernelTests\Core\Image\ToolkitGdTest fails on PHP 7.1.x-dev and 7.0.x-dev following a PHP bugfix to review and a follow up in #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ .

Investigate if there is an issue about file_progress_implementation() and the 'apc' implementation or create one - patch removes it because it is not possible in PHP7

We have #1561866: Add support for built-in PHP session upload progress for that :)

I think this belongs as a requirement to open Drupal 9.

Wrt #2 and #7, #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ is actually independent from the other issue, so I unpostponed it. It's ready for review.

+++ b/core/lib/Drupal/Component/Annotation/composer.json
@@ -5,7 +5,7 @@
-    "php": ">=5.5.9",
+    "php": ">=7.0.8",

Much like in #3039611: Update core PHP dependencies for 8.8.x... This change is based on what, exactly?

I've been trying to pin down honest constraints for the components in #2876669: Fix dependency version requirement declarations in components for quite a while. A maintainer could have saved me a lot of time and just said, "No, don't bother. We'll just arbitrarily assign them PHP >=7 later."

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
@@ -310,9 +310,9 @@ public function invokeAllDeprecated($description, $hook, array $args = []);
    *
-   * Note that objects are always passed by reference in PHP5. If it is absolutely
-   * required that no implementation alters a passed object in $context, then an
-   * object needs to be cloned:
+   * Note that objects are always passed by reference in PHP. If it is
+   * absolutely required that no implementation alters a passed object in
+   * $context, then an object needs to be cloned:
    * @code
    *   $context = array(

Awww, so cute, a PHP4 => PHP5 hint :)

I think we could just as well remove "in PHP", because it seems pretty obvious that we are talking about that ;) Or even the whole sentence or rewrite it to something like "Objects are always passed by reference, if it is.. ". Actually, that sentence is not correct anyway, an object is not passed by reference, the point is that we are just passing a reference to the actual object: https://www.php.net/manual/en/language.oop5.references.php.

Wondering if we should remove the whole thing. Seems like an extremely rare use case to worry about and by now, most developers should know how objects behave?

> Much like in #3039611: Update core PHP dependencies for 8.8.x... This change is based on what, exactly?

It is based on the existing core/composer.json, we just remove the alternative PHP 5.x constraint there now. We don't have to be "honest" about the version we want to support, that's up to us to define, not what the code *currently* requires. By raising it to PHP 7, we can then *afterwards* starting using PHP7-only syntax in there.

#18: Fine, but I think we should at least reword it then? remove part or the whole first sentence, because as I wrote, "in PHP" seems strange. Obviously we are talking about PHP. That only made sense when it was about PHP5 (and unlike every other PHP 5 comment, it was about how PHP 5 is different to 4, so really PHP5+).

1. +++ b/core/INSTALL.sqlite.txt
   @@ -3,7 +3,7 @@ SQLITE REQUIREMENTS
    
    To use SQLite with your Drupal installation, the following requirements must be
   -met: Server has PHP 5.3.10 or later with PDO, and the PDO SQLite driver must be
   +met: Server has PHP 7.0.8 or later with PDO, and the PDO SQLite driver must be
    enabled.
    
   

   another one that seems a bit unnecessary now, as that is simply the minimum PHP version, we'd have to touch it again if we change it to 7.1 for example. So maybe just say PDO + PDO SQLite driver must be enabled?

2. +++ b/core/lib/Drupal/Component/Assertion/Handle.php
   @@ -5,37 +5,21 @@
   + *
   + * @todo https://www.drupal.org/node/3054072 deprecate this class.
     */
    class Handle {
   

   Didn't find any explicit definition, but the example in https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... has the link at the end after a "." and also starts with an uppercase character. Many of these new todos (which will hopfully only exist for a short time), look a bit strange right now.

3. +++ b/core/modules/field_layout/src/Entity/FieldLayoutEntityFormDisplay.php
   @@ -16,8 +16,9 @@ class FieldLayoutEntityFormDisplay extends EntityFormDisplay implements EntityDi
       */
      public function getDefaultRegion() {
   -    // This cannot be provided by the trait due to
   -    // https://bugs.php.net/bug.php?id=71414 which is fixed in PHP 7.0.6.
   +    // @todo https://www.drupal.org/project/drupal/issues/3054317 Move to trait.
   +    //   This cannot be provided by the trait due to
   +    //   https://bugs.php.net/bug.php?id=71414 which is fixed in PHP 7.0.6.
   

   This is also a bit confusing IMHO. the original comment is now part of the todo, one option might be to add the @todo below the comment, that's what I usually do and also other cases in this patch do, or rewrite the comment so it makes sense as a task that we need to do.

4. +++ b/core/modules/file/tests/src/Functional/DownloadTest.php
   @@ -92,10 +92,6 @@ protected function doPrivateFileTransferTest() {
   -
   -    // Tilde (~) is excluded from this test because it is encoded by
   -    // rawurlencode() in PHP 5.2 but not in PHP 5.3, as per RFC 3986.
   -    // @see http://php.net/manual/function.rawurlencode.php#86506
        // "Special" ASCII characters.
        $basename = " -._!$'\"()*@[]?&+%#,;=:\n\x00" .
          // Characters that look like a percent-escaped string.
   

   Should we then include the ~ again as all supported PHP versions should have a consistent result now (which they did for a long time now..)

5. +++ /dev/null
   @@ -1,33 +0,0 @@
   -/**
   - * @coversDefaultClass \Drupal\Core\TypedData\TypedData
   - *
   - * @group TypedData
   - */
   -class TypedDataTest extends UnitTestCase {
   -
   

   Was wondering why this was removed for a second, but it was testing the custom implemntation of TypedData::__sleep() because we couldn't use the trait there. Now we do and we don't need the unit test anymore. Nice.

Ok, I've went through the patch a few times, and I think as a patch, this is RTBC. This is touching quite a few strange parts, but I think this is doing enough and has lots of well documented follow-ups (nice work!) and corresponding @todo's.

What I don't know is what exactly is still needed to be able to actually commit this:

a) I assume so, but is the decision of a hard break of PHP 5 in 8.8 now final and we can commit this now (I don't see a reason to wait like we did with 8.7)?
b) I assume we somehow need to auto-update all existing test configurations that would use 8.8 + PHP 5.5 (there are quite a few of those in contrib), so we might need some sort of process to update all those to PHP 7(.1?) and disallow that configuration. Or do we say that maintainers are responsible for that themself?
c) Whatever other communication and documentation is necessary, release notes snippet is already there, so that's good.

I'll have a look at b. Theres a lot of other contrib testing setups that are not valid that need examining.

We have a system in place for un-supporting testing configurations. They will show up on their project’s Automated testing tab as “disabled, testing configuration no longer available” and can be edited to something valid. I believe we’ve used this once, and emailed the affected project maintainers to let them know and encourage updating their testing configuration.

Awesome!

I see this got rid of all phpversion() calls. JSON:API used PHP_MAJOR_VERSION instead, but for the same purpose. It's the only occurrence of that in core. I think that should also be removed as part of this patch.

+++ b/core/INSTALL.sqlite.txt
@@ -2,11 +2,8 @@
-If you have not pdo_sqlite available depending on your system there are different ways to install it.
+PHP's PDO SQLite driver must be enabled. If you have not pdo_sqlite available
+depending on your system there are different ways to install it.
 

I know this is already there but we should fix the grammar here.

'If you do not have pdo_sqlite available on your system, there are'. Did this change on commit.

Committed/pushed to 8.8.x, thanks! Nice to be rid of so much of this.

Rest well, PHP5

1. Let's remember to tag things for the release notes.
    
2. The release note here could also use some work; it doesn't make any sense to tell users to use Drupal 8.7.x in the 8.8 release notes? The important information is that PHP 5 support was officially removed in 8.7, but we were nice and still let you update to security releases even if you hadn't updated your server yet. 8.8.0 ends that and your update will fail if your server is not updated to at least PHP 7.0.8. And we should also mention the recommended PHP version, that PHP 7.0 and 7.1 are (at that point) no longer receiving security coverage so you should really update to PHP 7.2 to be safe and avoid risk of insecure/incompatible dependencies.
    
3. Also, this needs a change record. There's a lot of fiddly details in all the support we're dropping here for all the things.
    
4. I also think we need to announce this in more channels than just the release notes and in advance of the 8.8.0 release, because it's the first (and hopefully only) time in the Drupal 8 release cycle that we prevent you from updating your site on purpose. I'm tagging for 8.8.0 highlights as well. Here's the spin: Drupal 8.8 cleanly requires PHP 7 to ensure the security and forward-compatibility of our dependencies, so that security updates will be more predictable and safe until the EOL at the end of 2021. Or something like that. I trust product/the DA to message it effectively. ;)
    
5. The patch mentions PHP 7.2.11. Do we have any compatibility issues with (e.g.) 7.1.0 or 7.2.0 that should be specified? Like is there a minimum version of PHP 7.1 or 7.2 that's needed, and if so should that be hardcoded in the requirements?
    

Added a new followup: #3058116: Remove paragonie/random_compat as a top-level dependency from 8.8.x

Shouldn't this issue be an active meta, to track the followups table?

closed, but related, not sure where else to link it to add visibility.

Polishing the release note a little.