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

Adjust priority for template_redirect #893

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

wordpressfan
Copy link
Contributor

Description

Fixes #891

Documentation

User documentation

As mentioned in the issue itself, here we fixed the priority of template_redirect hook's callback to be 10 instead of -1000 to fix the buffer closing conflict with WP Rocket.

Technical documentation

Just changed the priority.

Type of change

Delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).

New dependencies

List any new dependencies that are required for this change.

Risks

As I mentioned here:
#891 (comment)

We added this -1000 priority in the mentioned PR to support rocket CDN so I tested this and it works so we may need to check another CDN plugin to see how it works with this branch.

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Documentation

  • I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
  • The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
  • I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.
  • I listed the introduced external dependencies explicitely on the PR.
  • I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

  • I handled errors when needed.
  •  I wrote user-facing messages that are understandable and provide actionable feedbacks.
  • I prepared ways to observe the implemented system (logs, data, etc.).

Risks

  •  I explicitely mentioned performance risks in the PR.
  • I explicitely mentioned security risks in the PR.

@wordpressfan wordpressfan self-assigned this Aug 13, 2024
@wordpressfan wordpressfan marked this pull request as ready for review August 13, 2024 11:50
@wordpressfan wordpressfan requested a review from a team August 13, 2024 12:31
@Mai-Saad
Copy link

Mai-Saad commented Aug 16, 2024

@wordpressfan thanks for the PR.
Here are the test results using imagify and WPR so far https://docs.google.com/spreadsheets/d/1YpkRpc_PkcszOezpkIooWC6hIWnkIWnXITvoHNjEjIo/edit?gid=0#gid=0
1- Are we going to handle the case in C6 here?
2- Is the o/p on C2 and C7 expected?
3- Do we need other GH about lcp loaded twice in NW and CDN not writing lcp totally? @piotrbak

Note: for row 2 , the behavior is different if we used LL plugin instead of enable the option in WPR , with LL plugin the lcp won't be excluded from LL at all. any idea why LL plugin behaves differently than LL in WPR ? (test page https://new.rocketlabsqa.ovh/gallery/)

@Mai-Saad
Copy link

@wordpressfan As per discussion with @piotrbak we need to handle the C6 related scenario
1- Enable display picture
2- Enable CDN, LL image
3- Visit page have no lcp yet => lcp is added to DB
4- Clear cache and revisit the page => lcp /atf shall be excluded from LL (currently it is not LL)

@remyperona
Copy link
Contributor

Rocket lazyload plugin uses a priority of 2 on template_redirect, same as WP Rocket

@remyperona remyperona added this to the 2.2.3 milestone Sep 27, 2024
@Khadreal Khadreal added this pull request to the merge queue Oct 15, 2024
Merged via the queue into develop with commit 7566f98 Oct 15, 2024
5 checks passed
@Khadreal Khadreal deleted the fix/891-template-redirect-priority branch October 15, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update callback priority on template_redirect for picture display
4 participants