Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update callback priority on template_redirect for picture display #891

Closed
remyperona opened this issue Aug 7, 2024 · 18 comments · Fixed by #893, wp-media/rocketcdn-plugin#39 or wp-media/wp-rocket#7011
Assignees
Milestone

Comments

@remyperona
Copy link
Contributor

Context
Related to wp-media/wp-rocket#6786

WP Rocket Lazyload exclusion is not working correctly when excluded pattern is part of a picture element created by the picture rewriting feature of Imagify.

Expected beavhior
Lazyload exclusion should work as expected

Additional information
So I believe I found what was happening with the next-gen issue. we have 2 callbacks on template_redirect:

  • Imagify callback on priority -1000
  • WP Rocket callback on priority 2

So we should expect imagify to be finished first, and WP Rocket after. BUT, both callbacks have an ob_start() call, and when multiple are called, they become nested until the buffering stops:

  • Imagify opens buffer first
  • then WP Rocket, nested inside

When output buffering ends (on shutdown action of WP), lower level content is passed to upper, so the order becomes WP Rocket optimizations first, then Imagify second, and HTML is output.
While callback priority is as expected, the output buffering works in the opposite order.

To verify, I tested by changing the priority of Imagify callback to 10, reverting both orders, and it works as expected, the picture element is applied first in the HTML, and then lazyload exclusion works as needed.

@piotrbak piotrbak added this to the 2.2.3 milestone Aug 8, 2024
@wordpressfan
Copy link
Contributor

I can confirm that the provided fix is working from my side, I was trying to find why we used to use this priority -1000 and I found that we did that here:

#383
to fix this issue:
#368

So I believe we need to validate that issue and make sure that CDN is working properly with such change.

@johan-las
Copy link

@wordpressfan
Copy link
Contributor

I tested when using RocketCDN here: https://new.rocketlabsqa.ovh/next-gen-image/
and all good I believe, so we are fine here.

@MathieuLamiot
Copy link
Contributor

Blocked pending product decision on the feedbacks from QA.

@MathieuLamiot
Copy link
Contributor

Feedbacks from QA on the PR need to be investigated then handled. this is not a priority compared to 3.17 os moving this back to Needs Grooming and outside of the sprint.

@alfonso100
Copy link

@camilamadronero
Copy link

@MathieuLamiot
Copy link
Contributor

@Khadreal Do you have an update on this grooming? Thanks!

@Khadreal
Copy link
Contributor

Khadreal commented Oct 1, 2024

Would revert later today, the LL is not excluded if it's picture display and cdn is on.

@Khadreal Do you have an update on this grooming? Thanks!

@Khadreal
Copy link
Contributor

Khadreal commented Oct 1, 2024

After debugging with @wordpressfan, we discovered that the issue is related to the speed between RocketCDN and WPR. We’ll be submitting another PR to WPR to address this.

During lazy loading, the URL in the <source> element of the <picture> tag hasn't been correctly updated. However, the src for images excluded from AboveTheFold optimization has already been rewritten. Because of this, the logic here to check if the image should be excluded will consistently fail.

String: <picture decoding="async" data-id="88132" class="wp-image-88132">
<source type="image/avif" srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1067x800.jpg.avif 1067w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-533x400.jpg.avif 533w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-768x576.jpg.avif 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1536x1152.jpg.avif 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-2048x1536.jpg.avif 2048w" sizes="(max-width: 1067px) 100vw, 1067px"/>
<source type="image/webp" srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1067x800.jpg.webp 1067w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-533x400.jpg.webp 533w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-768x576.jpg.webp 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1536x1152.jpg.webp 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-2048x1536.jpg.webp 2048w" sizes="(max-width: 1067px) 100vw, 1067px"/>
<img decoding="async" width="1067" height="800" data-id="88132" src="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1067x800.jpg" alt="" srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1067x800.jpg 1067w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-533x400.jpg 533w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-768x576.jpg 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-1536x1152.jpg 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/marchlarge1-2048x1536.jpg 2048w" sizes="(max-width: 1067px) 100vw, 1067px"/>
</picture> 
Excluded image : wp-content/uploads/2024/08/7sk3pi.jpg.avif 500w, https://9a2073a2.rocketcdn.me/wp-content/uploads/2024/08/7sk3pi-400x400.jpg.avif 400w, https://9a2073a2.rocketcdn.me/wp-content/uploads/2024/08/7sk3pi-280x280.jpg.avif 280w

@MathieuLamiot
Copy link
Contributor

Thanks @Khadreal. Assigning to you and moving to In Progress then.

@remyperona
Copy link
Contributor Author

is there some way I can help with that?

@Khadreal
Copy link
Contributor

Khadreal commented Oct 4, 2024

I think the challenge here is with the rocketcdn plugin as a standalone, it's using template_redirect which is conflicting with imagify process.
For CDN within wpr, changing the priority for rocket_buffer solves the issue.

I'm thinking of one of the solution
-- If you're using rocketcdn plugin and wpr is installed, we bail out early and use the cdn within the plugin(there might be some drawback here)
-- Consider using rocket_buffer for rocketcdn plugin instead of the template_redirect
Please let me know what you think ?

@remyperona
Copy link
Contributor Author

We can update the priority used in the RocketCDN plugin too, to make sure everything executes in the order we need

@Khadreal
Copy link
Contributor

Khadreal commented Oct 4, 2024

The priority is in the right order though in RocketCDN

@Khadreal
Copy link
Contributor

@wp-media/qa-team to test the PRs for this issue please follow this
RocketCDN + Imagify

WPRocket + Imagify
To test with WPR cdn
Make sure you're on this branch fix/891-template-redirect-priority for imagify and on this branch fix/891-template-redirect-priority for WPR and follow the same step as shared above.

@Mai-Saad
Copy link

2nd round of tests using WPR and imagify in sheet 2 here https://docs.google.com/spreadsheets/d/1YpkRpc_PkcszOezpkIooWC6hIWnkIWnXITvoHNjEjIo/edit?gid=1626911531#gid=1626911531 , waiting PO decision if we need to do further fix here https://wp-media.slack.com/archives/CUKB44GNN/p1728893947213469

@Mai-Saad
Copy link

as per discussion with @piotrbak we are good to proceed with the current state and for C7 , clear priority elements will fix that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment