Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#51937 closed defect (bug) (fixed)

Fatal error while cropping

Reported by: boniu91's profile Boniu91 Owned by: iandunn's profile iandunn
Milestone: 5.6 Priority: high
Severity: blocker Version: 5.6
Component: Customize Keywords: php8 https has-patch has-unit-tests commit dev-reviewed
Focuses: javascript, administration Cc:

Description

Environment:
-WordPress 5.6 RC3
-PHP8
-Twenty TwentyOne

Steps to reproduce:
-Go to the Customize > Site Identity
-Click on Select logo
-Upload one (in my case the image is 2599x1550 pixels)
-Click on the Crop Image
-Confirm the 500 error in the logs and in the Network Tab

Error itself:
https://snippi.com/s/zjvyr9t

Screen from the Network Tab:
https://jmp.sh/bqYtoFv

Attachments (1)

image (4).png (50.2 KB) - added by Boniu91 3 years ago.
Screen from the network tab

Download all attachments as: .zip

Change History (22)

@Boniu91
3 years ago

Screen from the network tab

#1 @hellofromTonya
3 years ago

Hello and Welcome to Trac @Boniu91,

Thank you for finding this issue in today's Test Scrub and for reporting it here.

Can you verify if the same error occurs when using PHP 7? Why?

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


3 years ago

#3 @hellofromTonya
3 years ago

  • Keywords php8 added

Tagging with php8 as the fatal error is ValueError.

#4 @iandunn
3 years ago

  • Keywords reporter-feedback added; php8 removed

I haven't been able to reproduce this so far, but haven't tried too hard.

It looks like the problem is that the JS client is sending NaN instead of actual integer values.

@Boniu91:

  • What browser did you use?
  • Were there any errors in the console?

#5 @iandunn
3 years ago

If there are errors in the console, it could help to temporarily disabling all plugins and browser extensions.

At the very least, though, Core should be able to detect when it doesn't have valid values, and handle that gracefully.

#6 @Boniu91
3 years ago

@iandunn @hellofromTonya I can see the problem now. The site was using HTTPS while the settings in the options were pointing to the HTTP. There is a mixed content warning, changing settings to HTTPS solves the problem. 👍

#7 follow-up: @iandunn
3 years ago

  • Focuses javascript added
  • Keywords php8 https needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.6
  • Owner set to iandunn
  • Priority changed from normal to high
  • Severity changed from normal to blocker
  • Status changed from new to accepted

Ah, thanks!

Some rough thoughts:

  1. PHP: Add some code to detect bad inputs and handle it gracefully, similar to r49019 (h/t @SergeyBiryukov). This is maybe a blocker for 5.6? I'll start looking into this.
  1. JS: Detect the bad inputs, disable the crop button, and show a user-friendly error. This would be normal priority / future release.
  1. HTTPS UX: Show an admin notice on Admin > Settings > General when the mixed content is detected, encouraging the admin to upgrade to HTTPS-only. This could also be normal / future. There may already be a ticket for this, as part of the HTTPS project?

Any thoughts on that approach?

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


3 years ago

#9 @dlh
3 years ago

Agreed on normal/future release for the Customizer JS side of things, unless anyone is able to find that the NaNs represent a regression in 5.6.

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


3 years ago

#11 in reply to: ↑ 7 @SergeyBiryukov
3 years ago

Replying to iandunn:

HTTPS UX: Show an admin notice on Admin > Settings > General when the mixed content is detected, encouraging the admin to upgrade to HTTPS-only. This could also be normal / future. There may already be a ticket for this, as part of the HTTPS project?

Just linking to some related tickets here: #10970, #18769, #40353, #47954, #50629.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in PR #791 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#12

  • Keywords has-patch added; needs-patch removed

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

Guards WP_Image_Editor_GD::crop to avoid PHP 8 fatal error by:

  • Checking dimensions are integer
  • And > 0.

#13 in reply to: ↑ description @SergeyBiryukov
3 years ago

Replying to Boniu91:

Error itself:
https://snippi.com/s/zjvyr9t

For reference, just in case that link becomes inaccessible later, the error is:

[04-Dec-2020 14:09:48 UTC] PHP Fatal error:  Uncaught ValueError: imagecreatetruecolor(): Argument #1 ($width) must be greater than 0 in /home/customer/www/domain.dev/public_html/wp-includes/media.php:3511

Stack trace:
#0 /home/customer/www/domain.dev/public_html/wp-includes/media.php(3511): imagecreatetruecolor(0, 0)
#1 /home/customer/www/domain.dev/public_html/wp-includes/class-wp-image-editor-gd.php(326): wp_imagecreatetruecolor(0, 0)
#2 /home/customer/www/domain.dev/public_html/wp-admin/includes/image.php(44): WP_Image_Editor_GD->crop(0, 0, 0, 0, 0, 0, false)
#3 /home/customer/www/domain.dev/public_html/wp-admin/includes/ajax-actions.php(3874): wp_crop_image('/home/customer/...', 0, 0, 0, 0, 0, 0)
#4 /home/customer/www/domain.dev/public_html/wp-includes/class-wp-hook.php(287): wp_ajax_crop_image('')
#5 /home/customer/www/domain.dev/public_html/wp-includes/class-wp-hook.php(311): WP_Hook->apply_filters('', Array)
#6 /home/customer/www/domain.dev/public_html/wp-includes/plugin.php(484): WP_Hook->do_action(Array)
#7 /home/customer/www/domain.dev/public_html/wp-admin/admin-ajax.php(184): do_action('wp_ajax_crop-im...')
#8 {main}
  thrown in /home/customer/www/domain.dev/public_html/wp-includes/media.php on line 3511

#14 @iandunn
3 years ago

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

In 49751:

Media: Return WP_Error when cropping with bad input to avoid fatal.

This avoids an error on PHP 8 caused by calling wp_imagecreatetruecolor() with inputs that aren't numeric, or are less than 0.

Props hellofromtonya, Boniu91, metalandcoffee, SergeyBiryukov.
Fixes #51937.

#15 @iandunn
3 years ago

In 49753:

Media: Return WP_Error when cropping with bad input to avoid fatal.

This avoids an error on PHP 8 caused by calling wp_imagecreatetruecolor() with inputs that aren't numeric, or are less than 0.

Props hellofromtonya, Boniu91, metalandcoffee, SergeyBiryukov.
Reviewed by SergeyBiryukov, iandunn.
Merges [49751] to the 5.6 branch.
Fixes #51937.

#17 follow-up: @hellofromTonya
3 years ago

  • Keywords php8 has-patch removed
  • Milestone changed from 5.6 to 5.7

Closing on 5.6 milestone. PHP 8 is resolved.

Moving the remaining work to 5.7.

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


3 years ago

#19 in reply to: ↑ 17 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.7 to 5.6

Replying to hellofromTonya:

Closing on 5.6 milestone. PHP 8 is resolved.

Moving the remaining work to 5.7.

It seems confusing to keep this as closed on the 5.7 milestone, it should either be reopened for 5.7 or closed on 5.6.

I think the latter makes more sense, as the only actionable item left here is to add some validation for "WordPress Address" and "Site Address" options in General Settings to avoid mixed content issues. That would be something for the tickets linked in comment:11.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#20 @SergeyBiryukov
3 years ago

  • Keywords php8 added

#21 @hellofromTonya
3 years ago

  • Keywords has-patch has-unit-tests commit dev-reviewed added

Thanks Sergey. Lost my mind there...doh.

Adding missing keywords to reflect the current state of this ticket.

Note: See TracTickets for help on using tickets.