Make WordPress Core

Opened 9 months ago

Closed 2 weeks ago

#59782 closed defect (bug) (fixed)

PHP deprecation error in WP_Image_Editor_Imagick

Reported by: skithund's profile skithund Owned by: joedolson's profile joedolson
Milestone: 6.6 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: php81 has-patch commit has-unit-tests
Focuses: php-compatibility Cc:

Description

When cropping and saving an image it throws an deprecation error when WP_Image_Editor_Imagick is converting floats to ints.

Implicit conversion from float 1112.7466666666667 to int loses precision

Backtrace originally in JSON format from our central logging, hence funny formatting:

{
  "file": "/srv/www/site/public_html/wp-admin/admin-ajax.php",
  "line": 188,
  "function": "do_action",
  "args": [
    "wp_ajax_image-editor"
  ]
},
{
  "file": "/srv/www/site/public_html/wp-includes/plugin.php",
  "line": 517,
  "function": "do_action",
  "class": "WP_Hook",
  "object": {
    "callbacks": {
      "1": {
        "wp_ajax_image_editor": {
          "function": "wp_ajax_image_editor",
          "accepted_args": 1
        }
      }
    }
  },
  "type": "->",
  "args": [
    [
      ""
    ]
  ]
},
{
  "file": "/srv/www/site/public_html/wp-includes/class-wp-hook.php",
  "line": 334,
  "function": "apply_filters",
  "class": "WP_Hook",
  "object": {
    "callbacks": {
      "1": {
        "wp_ajax_image_editor": {
          "function": "wp_ajax_image_editor",
          "accepted_args": 1
        }
      }
    }
  },
  "type": "->",
  "args": [
    "",
    [
      ""
    ]
  ]
},
{
  "file": "/srv/www/site/public_html/wp-includes/class-wp-hook.php",
  "line": 310,
  "function": "wp_ajax_image_editor",
  "args": [
    ""
  ]
},
{
  "file": "/srv/www/site/public_html/wp-admin/includes/ajax-actions.php",
  "line": 2683,
  "function": "wp_save_image",
  "args": [
    4281
  ]
},
{
  "file": "/srv/www/site/public_html/wp-admin/includes/image-edit.php",
  "line": 930,
  "function": "image_edit_apply_changes",
  "args": [
    {},
    [
      {
        "type": "crop",
        "sel": {
          "x": 0,
          "y": 45,
          "w": 475,
          "h": 326
        }
      }
    ]
  ]
},
{
  "file": "/srv/www/site/public_html/wp-admin/includes/image-edit.php",
  "line": 722,
  "function": "crop",
  "class": "WP_Image_Editor_Imagick",
  "object": {},
  "type": "->",
  "args": [
    0,
    153.60000000000002,
    1621.3333333333335,
    1112.7466666666667
  ]
},
{
  "file": "/srv/www/site/public_html/wp-includes/class-wp-image-editor-imagick.php",
  "line": 606,
  "function": "setImagePage",
  "class": "Imagick",
  "object": {},
  "type": "->",
  "args": [
    1621.3333333333335,
    1112.7466666666667,
    0,
    0
  ]
}

Attachments (1)

59782.2.diff (2.6 KB) - added by joedolson 5 weeks ago.
Refreshed patch

Download all attachments as: .zip

Change History (29)

#1 @skithund
9 months ago

Couldn't add php81 keyword

#2 @SergeyBiryukov
9 months ago

  • Keywords php81 added

This ticket was mentioned in PR #5968 on WordPress/wordpress-develop by nicomollet.


6 months ago
#3

  • Keywords has-patch added

This pull request removes the PHP deprecated errors (like Implicit conversion from float 1112.7466666666667 to int loses precision) in WP_Image_Editor_Imagick/WP_Image_Editor crop method.

While crop expects integers, integers are not always passed to it.
So the goal is to cast the params to cast (int) before passing them to the method.

Tested on:

  • PHP 8.1
  • PHP 8.0

How to reproduce the error:

  1. Go to /wp-admin/media-new.php and upload an image
  2. "Edit" the image
  3. Then do a crop with the mouse and "Apply crop"
  4. See the Implicit conversion from float in the debug log

Trac ticket:
https://core.trac.wordpress.org/ticket/59782

#4 @mai21
6 months ago

  • Tested the patch on WP 6.4.3 and 6.5-alpha-57529, with PHP 8.3.1, and can confirm that it's fixed.

Steps
1- Go to /wp-admin/media-new.php and upload an image
2- "Edit" the image
3- Then do a crop with the mouse and "Apply crop"
4- See the Implicit conversion from float in the debug log

With Patch
No 'PHP Deprecated: Implicit conversion from float xxx.xx' in debug.log

General note
After cropping the image, the size displayed on the image edit page is still that of the original image, not the newly generated image. However, if the image was used on a page, the new size will be used. => Not related to the patch, not sure if we have a ticket about this.

General note => Steps:
1- Upload an image
2- Open media library -> edit image
3- Notice the file size displayed on image data at the most right
4- Crop the image and click apply crop then save edits
5- Click the update button
6- Notice the file size => didn't change while the dimensions changed. (if we downloaded the new file, the size is different than that displayed on the edit image page)

Expected:
File size is updated as same as dimensions since the size is already changed

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


3 months ago

#6 @antpb
3 months ago

  • Milestone changed from Awaiting Review to 6.6

Moving this into 6.6 to get attention on this for the next release. patch and testing are looking good!

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


7 weeks ago

#8 @joedolson
7 weeks ago

#61375 was marked as a duplicate.

#9 @joedolson
7 weeks ago

#61375 was marked as a duplicate.

#10 @joedolson
7 weeks ago

  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


5 weeks ago

@joedolson
5 weeks ago

Refreshed patch

#12 @joedolson
5 weeks ago

  • Keywords commit added

Patch is straightforward, has unit tests, and has been tested on PHP8, 8.1, and 8.3.

I tested on PHP 8.2, and confirmed that as well; just to be thorough.

Marking for commit.

This ticket was mentioned in PR #6876 on WordPress/wordpress-develop by @joedolson.


5 weeks ago
#13

  • Keywords has-unit-tests added

#14 @joedolson
5 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 58457:

Media: Fix implicit conversion from float to int in image cropping.

Cast crop values to integers to prevent PHP error caused by implicit conversion from float to int values when cropping images using ImageMagick.

Props skithund, mai21, nicomollet, amanias1977, joedolson.
Fixes #59782.

@joedolson commented on PR #5968:


5 weeks ago
#15

In r58457

@joedolson commented on PR #6876:


5 weeks ago
#16

In r58457.

#17 follow-up: @andrewserong
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi all, I'm just reopening this ticket as the fix appears to have caused a regression in WP 6.6 RC 1 where the image cropping tools in the block editor are no longer working. I've shared reproduction steps over in https://github.com/WordPress/gutenberg/issues/62855

While testing locally, reverting the change in https://github.com/WordPress/wordpress-develop/pull/6876 appears to get the cropping tools working again. Is it possible to either revert or update this fix for 6.6 to fix the block editor cropping tools?

This ticket was mentioned in Slack in #core-editor by andrewserong. View the logs.


4 weeks ago

This ticket was mentioned in PR #6909 on WordPress/wordpress-develop by @andrewserong.


4 weeks ago
#19

## What

Fixes cropping tools in the block editor. Related Gutenberg issue: https://github.com/WordPress/gutenberg/issues/62855

## Why

As of https://github.com/WordPress/wordpress-develop/commit/4f175e167ebd33d68ebe347fcc376f1342d9e873 / https://github.com/WordPress/wordpress-develop/pull/6876, the width and height values are rounded and cast to an int before the comparison to see if the target width and height differ from the original width and height.

Since they are now ints, it exposes a bug where the && of the if conditional meant that if you were only cropping in one dimension, the check wouldn't pass, and cropping would not occur.

In the block editor, the cropping tools are aspect ratio based, so one of the dimensions will always match that of the source image. Therefore, now that the values are cast as ints, we need to update the condition that allows a cropping to occur. In this case, I believe it should be || instead of &&. If either width or height is different from the source image, then we should allow a crop.

## How to test

  1. Add an image block to a post in the block editor
  2. Click on the Crop tool in the block toolbar
  3. Select a Crop from the aspect ratio dropdown
  4. Click Apply
  5. The aspect ratio should be applied in the final image
  6. Save and load the post on the site frontend and double check that the crop is correct

Trac ticket: https://core.trac.wordpress.org/ticket/59782

#20 in reply to: ↑ 17 @jrf
4 weeks ago

Replying to andrewserong:

Hi all, I'm just reopening this ticket as the fix appears to have caused a regression in WP 6.6 RC 1 where the image cropping tools in the block editor are no longer working. I've shared reproduction steps over in https://github.com/WordPress/gutenberg/issues/62855

While testing locally, reverting the change in https://github.com/WordPress/wordpress-develop/pull/6876 appears to get the cropping tools working again. Is it possible to either revert or update this fix for 6.6 to fix the block editor cropping tools?

Just looked over the tickets. The fix here is correct and should stay in. The fix exposed an unrelated, pre-existing logic bug in the REST API, which affects GB, which highlights a need for better automated tests for that area of the REST API as that bug should have been caught long ago.

I suggest closing this ticket again as fixed and treating the bug in the REST API as a stand-alone bug as it could have been reproduced prior to this fix going in if passed the right test data.

Last edited 4 weeks ago by jrf (previous) (diff)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 weeks ago

#22 @andrewserong
4 weeks ago

Thanks for the feedback @jrf, I have opened a separate ticket in #61514

@andrewserong commented on PR #6909:


4 weeks ago
#23

Thanks for the feedback @jrfnl! I've added a unit test, copying the existing one for image cropping, but updating it to include cropping on only one axis.

@andrewserong commented on PR #6909:


3 weeks ago
#24

@jrfnl or @joedolson this PR is ready for another review and commit if it's all looking okay. It'd be great to get this fix in for the next 6.6 RC if we can.

#25 @SergeyBiryukov
3 weeks ago

In 58612:

REST API: Correct image cropping tools in the block editor.

As of [58457], the width and height cropping values are cast to an integer before the comparison to see if the target width and height differ from the original width and height.

Since they are now integers, it exposes a bug where the && of the if conditional meant that if you were only cropping in one dimension, the check wouldn't pass, and cropping would not occur.

In the block editor, the cropping tools are aspect ratio based, so one of the dimensions will always match that of the source image. Therefore, now that the values are cast as integers, the condition that allows a cropping to occur needs to be updated. If either width or height is different from the source image, then a crop should be allowed.

Follow-up to [50124], [58457].

Props andrewserong, jrf, kevin940726.
Fixes #61514. See #59782.

@SergeyBiryukov commented on PR #6909:


3 weeks ago
#26

Thanks for the PR! Merged in r58612.

#27 @davidbaumwald
2 weeks ago

In 58692:

REST API: Correct image cropping tools in the block editor.

As of [58457], the width and height cropping values are cast to an integer before the comparison to see if the target width and height differ from the original width and height.
Since they are now integers, it exposes a bug where the && of the if conditional meant that if you were only cropping in one dimension, the check wouldn't pass, and cropping would not occur.
In the block editor, the cropping tools are aspect ratio based, so one of the dimensions will always match that of the source image. Therefore, now that the values are cast as integers, the condition that allows a cropping to occur needs to be updated. If either width or height is different from the source image, then a crop should be allowed.

Follow-up to [50124], [58457].

Reviewed by davidbaumwald.
Merges [58612] to the 6.6 branch.

Props andrewserong, jrf, kevin940726.
Fixes #61514. See #59782.

#28 @hellofromTonya
2 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Just looked over the tickets. The fix here is correct and should stay in. The fix exposed an unrelated, pre-existing logic bug in the REST API, which affects GB, which highlights a need for better automated tests for that area of the REST API as that bug should have been caught long ago.

I suggest closing this ticket again as fixed and treating the bug in the REST API as a stand-alone bug as it could have been reproduced prior to this fix going in if passed the right test data.

This ticket was reopened due to a regression. But as @jrf noted, the fix in this ticket did not cause the regression. The regression itself is now fixed in #61514 via changesets [58612] (on trunk) and [ ] (on the 6.6 branch).

Thus, I'm reclosing this ticket as fixed via [58457].

Note: See TracTickets for help on using tickets.