WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 6 weeks ago

Last modified 6 weeks ago

#51937 closed defect (bug) (fixed)

Fatal error while cropping

Reported by: Boniu91 Owned by: 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 6 weeks ago.
Screen from the network tab

Download all attachments as: .zip

Change History (22)

@Boniu91
6 weeks ago

Screen from the network tab

#1 @hellofromTonya
6 weeks 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.


6 weeks ago

#3 @hellofromTonya
6 weeks ago

  • Keywords php8 added

Tagging with php8 as the fatal error is ValueError.

#4 @iandunn
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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.


6 weeks ago

#9 @dlh
6 weeks 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.


6 weeks ago

#11 in reply to: ↑ 7 @SergeyBiryukov
6 weeks 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 6 weeks ago by SergeyBiryukov (previous) (diff)

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


6 weeks ago

  • 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
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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.


6 weeks ago

#19 in reply to: ↑ 17 @SergeyBiryukov
6 weeks 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 6 weeks ago by SergeyBiryukov (previous) (diff)

#20 @SergeyBiryukov
6 weeks ago

  • Keywords php8 added

#21 @hellofromTonya
6 weeks 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.