wp-media / imagify-plugin

Speed up your website with lighter images without losing quality.
https://imagify.io
73 stars 27 forks source link

Safe-guard against sizes that are not strings #871

Closed MathieuLamiot closed 7 months ago

MathieuLamiot commented 7 months ago

Context Issue reported on Slack: https://wp-media.slack.com/archives/C056ZJMHG0P/p1712244597119379?thread_ts=1712218266.722999&cid=C056ZJMHG0P

Expected behavior

Acceptance Criteria

Additional information

Maybe this should be checked at the beginning of optimize_sizes?

Here is the stacktrace of the reported issue:

[2024-03-31T18:00:58.812075+00:00] PHP Fatal error: Uncaught TypeError: preg_match() expects parameter 2 to be string, int given in /nas/content/live/user/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php:1554#012Stack trace:#012#0 /nas/content/live/user/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php(1554): preg_match('/^(?<size>.+)@i...', 1920, NULL)#012#1 /nas/content/live/user/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php(500): Imagify\Optimization\Process\AbstractProcess->is_size_next_gen(1920)#012#2 /nas/content/live/user/wp-content/plugins/imagify/classes/Job/MediaOptimization.php(181): Imagify\Optimization\Process\AbstractProcess->optimize_size(1920, 0)#012#3 /nas/content/live/user/wp-content/plugins/imagify/classes/Job/MediaOptimization.php(66): Imagify\Job\MediaOptimization->task_optimize(Array)#012#4 /nas/content/live/user/wp-content/plugins/imagify/inc/classes/Dependencies/deliciousbrains/wp-background-processing/classes/wp-background-process.php(516): Imagify\Job\M in /nas/content/live/user/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php on line 1554

remyperona commented 7 months ago

if we receive a int, we can expect it to be a size value, and convert it to a string to make it work with the preg_match().

It probably worked before because this function was not strict on type, but now it is with more recent versions of PHP.

MathieuLamiot commented 7 months ago

Oh right, good catch! That would be better to try to convert then!

wordpressfan commented 7 months ago

I found the following line of code inside their theme functions.php

add_action( 'after_setup_theme', function() {
    add_image_size( '1920', 1920, 1080 );
} );

If u take a look at the shared error message, you will see this 1920 size coming as integer

PHP Fatal error: Uncaught TypeError: preg_match() expects parameter 2 to be string, int given in /nas/content/live/xx/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php:1554#012Stack trace:#012#0 /nas/content/live/xx/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php(1554): preg_match('/^(?<size>.+)@i...', 1920, NULL)#012#1 /nas/content/live/xx/wp-content/plugins/imagify/classes/Optimization/Process/AbstractProcess.php(500): 

Then I got this snippet in our test site and tried uploading a huge image but I can't see the avif image generated for this size (1920X1080) but again can't see any error in the log.

I'm mentioning the snippet here so QA can add while they validate this case.

MathieuLamiot commented 7 months ago

Awesome! Could it be something we check with an integration test directly in the codebase?