wp-media / imagify-plugin

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

htaccess is rewritten whenever we enable/disable avif or change settings #837

Closed Mai-Saad closed 2 months ago

Mai-Saad commented 3 months ago

Before submitting an issue please check that you’ve completed the following steps:

Describe the bug When we enable/disable avif or unselect thumbnail then save, htaccess is rewritten

To Reproduce Steps to reproduce the behavior:

  1. imagify installed and activated on apache server
  2. uncheck some thumbnails
  3. check htaccess

Expected behavior htaccess isnot rewritten and it's timestamp isnot updated

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

Acceptance Criteria (for WP Media team use only) Clear instructions for developers, to be added before the grooming

Miraeld commented 3 months ago

I don't get what's wrong here,

The expected behavior:

htaccess is rewritten and it's timestamp is updated

Right now when we save the settings it gets rewritten and the file timestamp is updated. Isn't it what we want ?

Tabrisrp commented 3 months ago

The htaccess should only be updated if the display next-gen and the method value change

Miraeld commented 3 months ago

Reproduce the problem

To reproduce the issue, follow these steps:

  1. Install and enable Imagify
  2. Go to the setting page of Imagify
  3. Save the settings without changing anything
  4. Check the modified datetime of the .htaccess file => It should be updated

Identify the root cause

There are two root causes, First one is located here: https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Webp/Display.php#L63

Second one: https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Avif/Display.php#L57

The issue is the same for both, we are only checking if this is enabled or not, and the remove() cause the modification of the htaccess even tho we did not change anything to the settings.

Scope a solution To solve the issue, we must check if it was enabled before or not. To do that, we can add

        $was_enabled = (bool) get_imagify_option( 'display_nextgen' );

here: https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Avif/Display.php#L49 and https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Webp/Display.php#L55

and change the condition to

        if ( $enabled && ! $was_enabled ) {
            // Add the WebP file type.
            $result = $this->get_server_conf()->add();
        } elseif ( ! $enabled && $was_enabled ) {
            // Remove the WebP file type.
            $result = $this->get_server_conf()->remove();
        }

Development steps:

Effort estimation: XS Is a refactor needed in that part of the codebase? No

Khadreal commented 3 months ago

Shouldn't we add the same condition to imagify-plugin/classes/Avif/Display.php ?

Tabrisrp commented 3 months ago

LGTM