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 rocket buffer #7011

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Conversation

Khadreal
Copy link
Contributor

@Khadreal Khadreal commented Oct 4, 2024

Description

Fixes wp-media/imagify-plugin#891

Type of change

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

Detailed scenario

Check here for wp-media/imagify-plugin#893 (comment)

Technical description

Changed the priority of rocket buffer callbacks to 2/3 to fix the conflict issue with imagify image rewrite.

Documentation

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.

@Khadreal Khadreal self-assigned this Oct 4, 2024
@Khadreal Khadreal changed the title Update rocket buffer priority for cdn -- wp-media/imagify-plugin#891 Adjust priority for rocket buffer Oct 4, 2024
@Khadreal Khadreal requested a review from a team October 4, 2024 18:11
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for a8997d91 0.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (a8997d9) Report Missing Report Missing Report Missing
Head commit (e85cb61) 37744 16583 43.94%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7011) 2 0 0.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

Copy link
Contributor

@remyperona remyperona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? With the change in Imagify it should not need to be modified on WP Rocket side

@Khadreal
Copy link
Contributor Author

Khadreal commented Oct 8, 2024

Is this change necessary? With the change in Imagify it should not need to be modified on WP Rocket side

Yes, it's necessary.
With the change in imagify, when you use the rocketcdn within wpr the issue still persists, this fix that.

@remyperona
Copy link
Contributor

I'm worried about this change, as it could fix this specific case, but cause issues at the same time. The priority of features on rocket_buffer was specificly set to execute in the right order to avoid conflicts, so if we progress with this change, we will have to test every features and confirm they continue to work as expected.

@Khadreal
Copy link
Contributor Author

Khadreal commented Oct 9, 2024

Should we revert it as it wasn't part of the initial issues but found out it during test.
Also, when you look at the priority features, performance_hints happens before cdn.

@remyperona
Copy link
Contributor

We can keep this open and do QA with the changes in Imagify & RocketCDN. If everything is good, we can close it, if QA report an issue related to it, we will see how to go with this one.

@Khadreal
Copy link
Contributor Author

Khadreal commented Oct 9, 2024

We can keep this open and do QA with the changes in Imagify & RocketCDN. If everything is good, we can close it, if QA report an issue related to it, we will see how to go with this one.

Please could you review it so we can move it to QA

@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Oct 10, 2024

@Khadreal @wp-media/qa-team If we must first do QA on Imagify & RocketCDN PRs, just move this to ready for QA as theyr are reveiewed already and leave a comment explaining this properly in the issue.
You can even add the Imagify and RocketCDN PRs to the board if you want to track them individually.
Don't get blocked because of processes :)

@Khadreal
Copy link
Contributor Author

@wp-media/qa-team please check the comments here

@Khadreal Khadreal added this pull request to the merge queue Oct 15, 2024
Merged via the queue into develop with commit 8b46e6b Oct 15, 2024
12 of 13 checks passed
@Khadreal Khadreal deleted the fix/891-template-redirect-priority branch October 15, 2024 14:07
@piotrbak piotrbak added this to the 3.17.2 milestone Oct 16, 2024
@wordpressfan wordpressfan mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update callback priority on template_redirect for picture display
4 participants