Fixes Container, Event Manager, CLI & Rewrite rules for AVIF

Tabrisrp commented 4 months ago

Description

In this PR:

Start usage of the container & event manager
Fix issue with the CLI command for next gen images generation
Fix issue with rewrite rules for WebP & AVIF
Fix the picture display and change it to a subscriber
Fix some function names

Type of change

Bug fix (non-breaking change which fixes an issue).
Checklists

Generic development checklist

[x] My code follows the style guidelines of this project, with adapted comments and without new warnings.
[ ] I have added unit and integration tests that prove my fix is effective or that my feature works.
[ ] The CI passes locally with my changes (including unit tests, integration tests, linter).

Test summary

[x] I triggered all changed lines of code at least once without new errors/warnings/notices.

MathieuLamiot commented 4 months ago

@Miraeld or @Khadreal to check this one to unlock the tests

Mai-Saad commented 4 months ago

@Tabrisrp Thanks for the PR. Please find exploratory notes below: (tested on nginx so far) 1- After a fresh install, if we enable Display Next-Gen -> rewrite rules, we will have this warning in debug.log and admin will have this (note: rewrite rules added in conf) [09-Feb-2024 05:35:49 UTC] PHP Warning: Undefined variable $result in /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Avif/RewriteRules/Display.php on line 79 Screenshot from 2024-02-09 07-37-09 2- Whenever we change anything in settings and save, we will see the bulk optimization notice. Is this expected? /cc @piotrbak (this is not on trunk) 3- Rewrite rules added to conf is as below, shouldn't we replace webp by next-gen in the 1st and last line? Shouldn't the location line have webp along with avif? Not sure about the lines in between, @piotrbak what do you think? Is there a sample of expected rules?

# BEGIN Imagify: rewrite rules for webp
location ~* ^(/.+)\.(jpg|jpeg|jpe|png|gif|avif)$ {
    add_header Vary Accept;

    if ($http_accept ~* "webp"){
        set $imwebp A;
    }
    if (-f $request_filename.webp) {
        set $imwebp  "${imwebp}B";
    }
    if ($imwebp = AB) {
        rewrite ^(.*) $1.webp;
    }
}
# END Imagify: rewrite rules for webp

4- If we access this page https://imagify.rocketlabsqa.ovh/picture-elements/ while display image -> picture element is enabled, we will have fatal error

[09-Feb-2024 06:03:54 UTC] PHP Fatal error:  Uncaught Error: Call to undefined method Imagify\Picture\Display::get_cdn_source() in /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php:725
Stack trace:
#0 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php(668): Imagify\Picture\Display->url_to_path()
#1 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php(582): Imagify\Picture\Display->get_nextgen_image_data_set()
#2 [internal function]: Imagify\Picture\Display->process_image()
#3 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php(431): array_map()
#4 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php(130): Imagify\Picture\Display->get_images()
#5 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php(105): Imagify\Picture\Display->process_content()
#6 [internal function]: Imagify\Picture\Display->maybe_process_buffer()
#7 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/functions.php(5373): ob_end_flush()
#8 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(324): wp_ob_end_flush_all()
#9 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#10 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action()
#11 /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/load.php(1260): do_action()
#12 [internal function]: shutdown_action_hook()
#13 {main}
  thrown in /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/plugins/imagify-plugin/classes/Picture/Display.php on line 725

5- Using image having Webp or AVIF, then enable display -> picture element. the webp or avif won't be served at FE https://imagify.rocketlabsqa.ovh/image-have-webp/ (this is working on trunk) https://imagify.rocketlabsqa.ovh/image-have-avif/

6- In edit image page if we click generate next-Gen image, we will be redirected to imagify settings page (this isnot happening on trunk)

Tabrisrp commented 4 months ago

Point 1 and 4 should be fixed. For point 3, the rules are as expected, There will be a block for webp and a block for avif depending on what is enabled.

I can't reproduce point 2.

For point 5 and 6, could we create separate issues for them, as they are not related to the fixes done on this PR.

piotrbak commented 4 months ago

@Tabrisrp @Mai-Saad For the 3rd point, the expectation here was that we'll be displaying AVIF > WEBP > Optimized Image no matter what's enabled. The same should apply for the Picture method.

This is to handle the scenario when the AVIF is enabled + the browser is comaptible with WEBP but not with AVIF. Also when AVIF is enabled, but the AVIF version is not yet generated while we have WEBP present.

Is this possible?

Tabrisrp commented 4 months ago

I updated the code so that the rules are added to the htaccess for both WebP and AVIF, as long as the rewrite option is enabled.

The order in the htaccess is cascading, so the WebP rules are first before the AVIF.

piotrbak commented 4 months ago

@Tabrisrp @Mai-Saad We should always try to display AVIF as the first one, is this how it'd work with the current order?

Mai-Saad commented 4 months ago

@Tabrisrp @piotrbak Currently, if we have image with only AVIF version existing on server, using rewrite rules, won't be applied and the AVIF image won't be served on FE. While if we have image with only WebP, the Webp will be served on FE using rewrite rules. While if image have both webP and AVIF, WebP will be served even with picture option.

Note:

on the current branch (not on feature/avif), we have duplicate in the rewrite rules text. can you please check this :pray:
For point 5, it was working on feature/avif branch but we can add new GH
For 6 added new GH as it's same on main feature branch https://github.com/wp-media/imagify-plugin/issues/803

Tabrisrp commented 4 months ago

New update:

Rewrites rules order updated to correctly prioritize avif, then WebP
Updated picture display to correctly prioritize avif, then WebP
Remove double display of conf file notice on NGINX
Fixed Bulk optimization notice, should only display when enabling convert_to_avif

Mai-Saad commented 4 months ago

@Tabrisrp Thanks for the update.

rewrite rules working fine on apache and picture. however, on nginx, if the image has only webP it won't be served (tested on local app using new our generated rewrite rules). Can you please check this and let me know if we need another GH :pray:
regarding bulk notice, it is still displayed on the live site whenever we change settings. Shall we create separate GH for this?
on nginx, using rewrite rules (on local app) , AVIF type appeared as octet-stream, is this expected? (note:rewrite rules on apache will show type as avif)

Tabrisrp commented 4 months ago

however, on nginx, if the image has only webP it won't be served (tested on local app using new our generated rewrite rules). Can you please check this and let me know if we need another GH

I don't know why it would not work, it's the same rules as before. I have no knowledge on NGINX configuration, I'm not sure how to help on that.

regarding bulk notice, it is still displayed on the live site whenever we change settings. Shall we create separate GH for this?

I can't reproduce it

on nginx, using rewrite rules (on local app) , AVIF type appeared as octet-stream, is this expected? (note:rewrite rules on apache will show type as avif)

I found the following in ShortPixel documentation:

You basically need to look for the file named mime.types, which is usually located in the root of the NGINX installation (/etc/nginx/), and add the two MIME types: image/avif and image/avif-sequence

piotrbak commented 4 months ago

however, on nginx, if the image has only webP it won't be served (tested on local app using new our generated rewrite rules). Can you please check this and let me know if we need another GH

@Mai-Saad Does it work on the trunk?

regarding bulk notice, it is still displayed on the live site whenever we change settings. Shall we create separate GH for this?

@Mai-Saad Did you try clearing all the transients? Does it happen on the Rocketlabs or local environment (or both)?

Mai-Saad commented 4 months ago

https://github.com/wp-media/wp-rocket/issues/6312

working fine on trunk using local env

@Mai-Saad Did you try clearing all the transients? Does it happen on the Rocketlabs or local environment (or both)?

It is happening on https://imagify.rocketlabsqa.ovh/ but not local (same after delete transients)

piotrbak commented 4 months ago

@Tabrisrp Could you take a look at this environment?

@Mai-Saad @Tabrisrp We'll tacke the NGINX rewrite rules here.

Mai-Saad commented 3 months ago

You basically need to look for the file named mime.types, which is usually located in the root of the NGINX installation (/etc/nginx/), and add the two MIME types: image/avif and image/avif-sequence

@Tabrisrp adding image/avif avif; in conf/nginx/mime-types.conf.hbs works while using picture and rewrite on local env :tada: :pray:

wp-media / imagify-plugin