wp-media / imagify-plugin

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

Guard against not string size name #877

Closed wordpressfan closed 7 months ago

wordpressfan commented 7 months ago

Description

Fixes #871

I can't reproduce the issue on my local, I tried to insert new media size with integer name but this didn't throw this error at all, I hope I can get the credentials from the escalated case working to see how this error happens.

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.

Documentation

User documentation

Explain how this code impacts users.

Technical documentation

Explain how this code works. Diagram & drawings are welcomed.

Type of change

Delete options that are not relevant.

New dependencies

List any new dependencies that are required for this change.

Risks

List possible performance & security issues or risks, and explain how they have been mitigated.

Checklists

Feature validation

Documentation

Code style

Observability

Risks

wordpressfan commented 7 months ago

I tried to add unit tests at least for this one but I found that in the WP class uses the filesystem so we need to get the virtual filesystem idea

Here: https://github.com/wp-media/imagify-plugin/blob/eeff063c9192027ea4a21c1c737654f858efd9fc/classes/Optimization/Process/AbstractProcess.php#L137

we need to replace it with

$this->filesystem = imagify_get_filesystem();

and then mock this function inside the virtual filesystem trait as we use in WPR here:

https://github.com/wp-media/wp-rocket/blob/42b2c28d6a72be4991566f153f7268b81afe68d6/tests/VirtualFilesystemTrait.php#L31-L34

The tests are not ready as a framework, so I'll leave it to be handled properly in the other created issue.