\Drupal\system\Plugin\ImageToolkit\GDToolkit needs to support \GdImage objects for PHP 8 compatibility

What should be done in the __destructor of the toolkit then? Is imagedestroy relevant at all in PHP 8?

Added type-hints

The purpose of testResourceDestruction() is to check that once Drupal's Image objects are losing scope, the memory allocated to the image is cleaned up. See #2531570: Image objects should release memory when losing scope.

The test would still be useful, so I changed it to test memory allocated at various stages.

`is_resource()` can accept any data type, but PHP 7.2 and older versions will fatal error on `instanceOf` checks. So for PHP 7.2 and older version compatibility, we will need a `is_object()` check too: https://php.watch/versions/8.0/gdimage#gdimage-is-resource

Further, `imagedestroy()` function is still available in PHP 8. I think we can simply call `imagedestroy()` without any additional checks because PHP internally no-ops it anyway.

Confirm as no-op https://github.com/php/php-src/blob/master/ext/gd/gd.c#L2040-L2052

Fixed check as suggested #8 if we need to backport it to PHP < 7.3 https://3v4l.org/lJnKN
Also replaced deprecated 'toInt()' usage for Bytes

Now it needs follow-up to finish @todo

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -100,6 +100,8 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
+    // @TODO Remove this method when core start require PHP 8.0 as imagedestroy
+    //   is no longer needed.

this @todo needs to be moved above into the docblock of the method.

+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -427,31 +428,59 @@ public function testManipulations() {
+    $preload_memory_usage = memory_get_usage(TRUE);

it would be nicer to use the service provided by #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service but, alas, that is still NR.

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -124,15 +126,17 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (!(is_object($resource) && $resource instanceof \GdImage)) {
   +      if (!is_resource($resource) || get_resource_type($resource) != 'gd') {
   +        throw new \InvalidArgumentException('Invalid resource argument');
   +      }
   

   could use some comments

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -142,11 +146,11 @@ public function setResource($resource) {
   +    if (!(is_resource($this->resource) || (is_object($this->resource) && $this->resource instanceof \GdImage))) {
   

   same

Filed follow-up #3173031: Clean-up \Drupal\system\Plugin\ImageToolkit\GDToolkit when core require php 8.0

Did not change #3000050 because checkMemoryLimit() is not getting current usage, will dig this issue later

RTBC +1 but I cannot. Thanks @andypost

+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -427,31 +428,59 @@ public function testManipulations() {
+    $preload_memory_usage = memory_get_usage(TRUE);
...
+    $postload_memory_usage = memory_get_usage(TRUE);
...
+    $postdestroy_memory_usage = memory_get_usage(TRUE);
...
+    $postload_memory_usage = memory_get_usage(TRUE);
...
+    $postcreatenew_memory_usage = memory_get_usage(TRUE);
+    $this->assertLessThan($postload_memory_usage - Bytes::toNumber('100 MB'), $postcreatenew_memory_usage);

it could use EnvironmentMemory::getUsed() but not sure it better or more intuitive then one function call? ref #3000050-43: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service
Maybe it more useful to get limits...

Retitling so it does not sound like a feature request :)

This test makes sense only while GD using resources, so I reverted test change to #2 and added another todo

PS patch smaller then interdiff

Made the changes pointed out in #18. Please review.

#14 IMO we should have a followup opened and referred to in the comment. I see the point in #14, but the root cause for entire test to be developed was memory management issues that were addressed in #2531570: Image objects should release memory when losing scope. The memory was filled up by not gc'ed resources, and the solution was to destroy the resources in __destruct. Now we will be losing test coverage of the root cause.

There's test which makes sense only for php7, personally instead of measuring RAM I'd converted test to unit-test to make sure destructor executes resource destroy.
Measuring memory is bad idea because it's PHP GC territory to decide when and how reclaim RAM

There's follow-up already to get rid of method and test, so could be used to improve test

Filed another follow-up #3179058: Remove ToolkitGdTest::testResourceDestruction() when core require PHP 8.0

So clean-up of destructor and fate of the test are separate issues

Changes look OK as per the PHP 8 upgrade notes. I checked for other uses of is_resource() and there are a few GD related ones but on closer inspection they all relate to destroying images after use so is_resource() alone is still OK and these can be removed when we have PHP 8 as a minimum dependency.

This looks great as do the follow-ups.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!