The Wayback Machine - https://web.archive.org/web/20220720233235/https://github.com/WordPress/performance/pull/195
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

Replace the full size image in the_content with additional MIME type if available #195

Merged
merged 10 commits into from Mar 10, 2022

Conversation

mitogh
Copy link
Member

@mitogh mitogh commented Feb 28, 2022

Summary

Fixes #174

This PR requires:

Relevant technical choices

This PR updates the changes introduced by #152 and #194 in order to replace the full size image if present in the new format, (in this case WebP) making sure all images served to the client are in the same format when a new image is uploaded into WordPress.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.
mitogh added 2 commits Feb 28, 2022
If the full size image is present as part of the metadata
information, make sure the image is replaced in the content
with the new reference to the expected image.

This fixes #174
@mitogh mitogh requested a review from felixarntz Feb 28, 2022
@mitogh mitogh self-assigned this Feb 28, 2022
@mitogh mitogh added [Focus] Images [Module] WebP Uploads labels Feb 28, 2022
@mitogh mitogh added this to the 1.0.0-beta.1 milestone Feb 28, 2022
@mitogh mitogh added the [Type] Enhancement label Feb 28, 2022
@mitogh mitogh changed the title Feature/174 full image size in content Feb 28, 2022
@mitogh mitogh removed this from the 1.0.0-beta.1 milestone Feb 28, 2022
@mitogh mitogh added this to the 1.0.0-beta.2 milestone Feb 28, 2022
@mitogh mitogh changed the base branch from trunk to feature/174-full-image-size Feb 28, 2022
@mitogh mitogh added the Needs Review label Feb 28, 2022
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Excellent!

Copy link
Member

@felixarntz felixarntz left a comment

@mitogh The new code overall looks solid, I found a few small issues that mostly relate to the already existing code, but we had missed them before.

modules/images/webp-uploads/load.php Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Show resolved Hide resolved
mitogh and others added 4 commits Mar 9, 2022
Prevent from running a loop when is not required
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@mitogh mitogh requested a review from felixarntz Mar 9, 2022
Copy link
Member

@felixarntz felixarntz left a comment

LGTM!

Let's wait with merging this though until #194 is ready, since technically the implementation here depends on how we implement #194.

@felixarntz
Copy link
Member

@felixarntz felixarntz commented Mar 9, 2022

Ah nevermind the above, I see you've created this against your branch for #194. I think for the future it would be better to create both PRs based on trunk, since if we merge this now, it would be harder to review #194 as it would include the changes from here which were already approved.

So I'd still say it's better to wait until #194 has been approved before we merge this.

@mitogh
Copy link
Member Author

@mitogh mitogh commented Mar 9, 2022

Thanks, mostly I've created this based on #194 just to have smaller diffs I agree this should is currently being blocked by #194 but having everything based on trunk will create merge conflicts after approval which usually triggers another review, so when having this approved and PR against #194 allows for simpler flow, however, I do see the problem specifically around dependencies between each PR.

Base automatically changed from feature/174-full-image-size to trunk Mar 10, 2022
@mitogh mitogh merged commit d70c915 into trunk Mar 10, 2022
3 checks passed
@mitogh mitogh deleted the feature/174-full-image-size-in-content branch Mar 10, 2022
@felixarntz felixarntz changed the title Replace the_content with the full size image in new mime types if available Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Images [Module] WebP Uploads Needs Review [Type] Enhancement
3 participants