Fatal error while cropping

[Boniu91]

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

[Boniu91]

Screen from the network tab

[hellofromTonya]

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?

[hellofromTonya]

Tagging with php8 as the fatal error is ValueError.

[iandunn]

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?

[iandunn]

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.

[Boniu91]

@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. 👍

[iandunn]

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?

[dlh]

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.

[SergeyBiryukov]

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.

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.

[SergeyBiryukov]

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

[iandunn]

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.

[iandunn]

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.

Merged via changeset https://core.trac.wordpress.org/changeset/49751

[hellofromTonya]

Closing on 5.6 milestone. PHP 8 is resolved.

Moving the remaining work to 5.7.

[SergeyBiryukov]

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.

[hellofromTonya]

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

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