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

Create additional MIME types for the full size image. #194

Merged
merged 26 commits into from
Mar 10, 2022

Conversation

mitogh
Copy link
Member

@mitogh mitogh commented Feb 28, 2022

Summary

Fixes #174

Relevant technical choices

The following PR introduces a sources property at the root of the metadata with all the information for every single mime type, in this case file and filesize.

The PR introduces a mechanism to remove the additional files created by the plugin

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

Make sure the variable is closer to the place where is going to be
used.
Add a sources property for the full size image and stored in a
sources property. This property would create an image with the
full size dimensions of the image uploaded into the media library.
Make sure the file follows the `phpcs` rules.
Add the removal of the `sources` property for mimes different
from the original image.

The function would ensure to remove any remaining image is removed.
Add an edge case when the image sizes are not defined
we need to make sure all original images are removed as part of the
files removed instead of returning early when no sizes are present.
@mitogh mitogh added [Type] Feature A new feature within an existing module [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Feb 28, 2022
@mitogh mitogh added this to the 1.0.0-beta.1 milestone Feb 28, 2022
@mitogh mitogh requested a review from felixarntz February 28, 2022 18:47
mitogh and others added 2 commits February 28, 2022 15:14
@mitogh mitogh modified the milestones: 1.0.0-beta.1, 1.0.0-beta.2 Feb 28, 2022
Merge the 2 hooks in charge of creating the full size images with
thes sources property of all the image sizes in a single function.
@adamsilverstein
Copy link
Member

🎉 This worked well in my testing, even with a resource constrained system where multiple retries were required.

Co-authored-by: Adam Silverstein <adamjs@google.com>
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mitogh Left another reply on #194 (comment), I think that point is still valid.

The other changes lgtm, however note that linting is currently failing, so something will need to be fixed there.

mitogh added 3 commits March 8, 2022 15:42
Split and share the logic to generate the `full` size image
with the subsizes for all images.
@mitogh mitogh requested a review from felixarntz March 9, 2022 00:04
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Left a bit more feedback here. Mostly lgtm, however there are a few inconsistencies. Most importantly, we shouldn't get this PR mixed up with what the separate issue #204 is intended for.

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated 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 March 8, 2022 22:53
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Due the original image change is part of a different PR, to keep
each PR separate.
In order to ensure all the required parameters are passed down
when calling this function.
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mitogh A few additional nit-picks, but it's close. My main open question is still https://github.com/WordPress/performance/pull/194/files#r823007963 though, that would be great to clarify.

modules/images/webp-uploads/load.php Outdated 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 added 3 commits March 9, 2022 16:24
The conditional and the usage of a fallback value is not required
An image should contain a positive number to be considered a valid
dimensions.
Add tests to cover additional scenarios handled in the function
`webp_uploads_generate_additional_image_source`
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mitogh Just one more comment based on the newly added code, and I also left a reply to #194 (comment) where I think we still should centralize the validation instead of checking outside of the filter function.

modules/images/webp-uploads/load.php Show resolved Hide resolved
@mitogh mitogh requested a review from felixarntz March 9, 2022 23:31
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Great!

@mitogh mitogh merged commit 6b48480 into trunk Mar 10, 2022
@mitogh mitogh deleted the feature/174-full-image-size branch March 10, 2022 01:18
@felixarntz felixarntz changed the title Create multiple mime types for the full size image. Create additional MIME types for the full size image. Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When generating WebPs, ensure full size is generated(-scaled for large images)
4 participants