Deprecate resource mentions in GDToolkit method names

Rerolled patch in #4, thanks!

Just updating the patch and this should fix the failing tests. Although, I am a bit tired ....

Can we change all the deprecations to 10.1.x we're targeting 10.1 for stuff like this and not 10.0

Thanks!

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -31,9 +31,21 @@ class GDToolkit extends ImageToolkitBase {
   +  /**
   +   * A GD image.
   +   *
   +   * @var \GdImage|null
   +   */
   +  protected $image = NULL;
   +
   

   We can typehint the property as ?\GdImage and remove from the docblock

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -135,12 +151,43 @@ public function setResource($resource) {
   +  public function setGdImage(\GdImage $image): self {
   

   self or static? static would allow extending the class IIRC

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -135,12 +151,43 @@ public function setResource($resource) {
   +  public function getGdImage(): \GdImage|NULL {
   

   typehint should be ?\GdImage

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -182,7 +229,7 @@ protected function load() {
   +      $this->setGdImage($resource);
   

   IMHO we should also rename the local $resource variable

5. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
   @@ -85,15 +85,15 @@ protected function execute(array $arguments) {
   +        imagedestroy($this->getToolkit()->getGdImage());
   

   imagedestroy is a noop since PHP 8.0, we can remove it and adjust the inline comment. Also rename $original_resource

6. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php
   @@ -55,7 +55,7 @@ protected function execute(array $arguments = []) {
   +    $original_resource = $this->getToolkit()->getGdImage();
   

   Rename $original_resource

7. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php
   @@ -64,15 +64,15 @@ protected function execute(array $arguments = []) {
   +        imagedestroy($this->getToolkit()->getGdImage());
   

   imagedestroy is noop

8. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -94,18 +94,18 @@ protected function execute(array $arguments) {
   +    $original_res = $this->getToolkit()->getGdImage();
   ...
   +    if ($new_res = imagerotate($this->getToolkit()->getGdImage(), 360 - $arguments['degrees'], $arguments['background_idx'])) {
   +      $this->getToolkit()->setGdImage($new_res);
   

   Rename $original_res and $new_res

9. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
   @@ -460,7 +460,7 @@ public function testGifTransparentImages(): void {
   +    $resource = $image->getToolkit()->getGdImage();
   

   Rename $resource

10. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
    @@ -473,7 +473,7 @@ public function testGifTransparentImages(): void {
    +    $resource = $image->getToolkit()->getGdImage();
    

    Rename $resource

11. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
    @@ -481,7 +481,7 @@ public function testGifTransparentImages(): void {
    +    $resource = $image_reloaded->getToolkit()->getGdImage();
    

    Rename $resource

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -135,12 +149,43 @@ public function setResource($resource) {
   +  public function setGdImage(\GdImage $image): static {
   

   this signature will potentially cause BC break if NULL is passed to legacy setResource() or to the method directly. We cannot rule that out from contrib, setting the resource to null can be a way to invalidate the image if for any reason a toolkit operation fails.

   So either we allow null like
   public function setGdImage(?\GdImage $image): static
   or we introduce a separate method to null the property.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
   @@ -93,19 +93,15 @@ protected function execute(array $arguments) {
   -    // Stores the original GD resource.
   -    $original_res = $this->getToolkit()->getResource();
   -
   

   seems like this is removed with no replacement?

#20.2 oh yeah I missed that!

Thanks looks all good to me now.

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -31,9 +31,19 @@ class GDToolkit extends ImageToolkitBase {
    * @var \GdImage|null
+   *
+   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use
+   *   \Drupal\system\Plugin\ImageToolkit\GDToolkit::image instead.
+   *
+   * @see https://www.drupal.org/node/3265963
    */
   protected $resource = NULL;

This change results in this property no longer being set. So keeping it around but unused feels odd. If we feel we need to maintain the protected property that we should maintain the value.

I think we should do something similar to the \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait and leverage the __get() function to trigger a deprecation if someone tries to use $this->resource and then we should give them $this->image.

Well, that would be only relevant for classes that extend the GDToolkit class itself, which I am not sure there are any... the toolkit operations would not access it directly anyway.

However, this is already agreed, so fine, but we also need a __set() implementation to fully cover BC.

I suppose so, if we need to be fully BC... you're right.

Thanks. Cannot see anything else.

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -28,11 +28,9 @@
   -  protected $resource = NULL;
   +  protected ?\GdImage $image = NULL;
   

   I wonder if this is really necessary, is it worth doing the deprecation dance for an internal property? It can just live as $resource for historical reasons - we only really need to update the public API?

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -135,12 +178,43 @@ public function setResource($resource) {
   +  public function setGdImage(?\GdImage $image): static {
   ...
   +  public function getGdImage(): ?\GdImage {
   

   Maybe bikeshedding here but do we need Gd in the method name, given the class is already the GDToolkit? Is setImage/getImage better?

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php
   @@ -39,7 +39,7 @@ protected function arguments() {
   -        'description' => 'If TRUE, this operation is being used to create a temporary image by another GD operation. After performing its function, the caller is responsible for destroying the original GD resource.',
   +        'description' => 'If TRUE, this operation is being used to create a temporary image by another GD operation. After performing its function, the caller is responsible for destroying the original GD object.',
   

   Does the statement about destroying still hold true now we are no longer using resources?

4. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
   @@ -460,9 +460,9 @@ public function testGifTransparentImages(): void {
   +    $gdImage = $image->getToolkit()->getGdImage();
   

   Variables should be snake_case when snake case is used elsewhere in the same file.

Looks good to me, #31.2-4 addressed. #34 would be an interesting follow-up I guess, even if at first sight I'd daresay we cannot exclude that a class extending this would not use it - so, is it or isn't it API? Just checked and in the ImageMagick module that argument is not implemented.

Flaky test failure?

Quite sure this needs a reroll after commit of #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors

Thanks for working on this! I guess the patch is the latest/canonical version here and not the merge request, so I am going to close the MR for clarity.

I made some small improvements to the change record.

1. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -28,11 +28,9 @@
   -  protected $resource = NULL;
   +  protected ?\GdImage $image = NULL;
   

   This is, technically, an internal BC break to make the typehint stricter. I think it is probably low-risk, but should be restricted to a minor only and maybe be mentioned in the CR.

2. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -112,6 +110,47 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if ($name === 'resource') {
   ...
   +    if ($name == 'resource') {
   ...
   +    if ($name == 'resource') {
   ...
   +    if ($name == 'resource') {
   

   Why are some of these == but the others ===?

3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -112,6 +110,47 @@ public static function create(ContainerInterface $container, array $configuratio
   +      @trigger_error('Accessing the \Drupal\system\Plugin\ImageToolkit\GDToolkit::resource property is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use \Drupal\system\Plugin\ImageToolkit\GDToolkit::image instead.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Setting the \Drupal\system\Plugin\ImageToolkit\GDToolkit::resource property is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use \Drupal\system\Plugin\ImageToolkit\GDToolkit::image instead.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Checking the \Drupal\system\Plugin\ImageToolkit\GDToolkit::resource property is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use \Drupal\system\Plugin\ImageToolkit\GDToolkit::image instead.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Unsetting the \Drupal\system\Plugin\ImageToolkit\GDToolkit::resource property is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use \Drupal\system\Plugin\ImageToolkit\GDToolkit::image instead.', E_USER_DEPRECATED);
   

   Method names in all the deprecation messages should have parens. OTOH the correct way to document PHP member properties is \Drupal\whatever\Class::$property. Both ::resource and ::image need to be fixed one way or the other.

   Also, these seem to refer to an image method or property that does not exist?

4. +++ b/core/tests/Drupal/Tests/Component/Utility/RectangleTest.php
   @@ -77,13 +77,10 @@ public function testRotateDimensions($width, $height, $angle, $exp_width, $exp_h
   -   *     if (is_resource($old_res)) {
   -   *       imagedestroy($old_res);
   -   *     }
   

   I wondered why this is being removed from the example code. Answer above is:

   imagedestroy is a noop since PHP 8.0, we can remove it and adjust the inline comment.

   So that's fine.

NW mainly for #2, although the CR could use some expansion about deprecated properties/stricter property types and etc. as well. Thanks!

Saving credits for reviewers.

+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -460,9 +460,9 @@ public function testGifTransparentImages(): void {
+    $gd_mage = $image->getToolkit()->getImage();
+    $color_index = imagecolorat($gd_mage, $image->getWidth() - 1, 0);
+    $color = array_values(imagecolorsforindex($gd_mage, $color_index));

@@ -473,17 +473,17 @@ public function testGifTransparentImages(): void {
+    $gd_mage = $image->getToolkit()->getImage();
+    $color_index = imagecolorat($gd_mage, $image->getWidth() - 1, 0);
+    $color = array_values(imagecolorsforindex($gd_mage, $color_index));

One more thing -- the local variable here is mis-named. While it is kind of magic, I think this should be "image" and not "mage". 🧙‍♂️

Addressed 43.2 and 45.
Please review.

The changes in patch #48 look good.

Wonder if we can update the IS and CR with the additional things being deprecated outside the 2 mentioned.

the name of the methods on the CR is outdated

Seems random failure.

I have updated the CR for readability and converted to a table.

Change looks good. Hopefully can get in early for 10.2

Committed and pushed 3b44eb23976 to 11.x. Thanks!

Published the change record.

Updated the CR to say 10.2 since it doesn't (appear) to have landed in 10.1.