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

Adds sources information to the attachment media details of the REST response #224

Merged
merged 9 commits into from Mar 16, 2022

Conversation

eugene-manuilov
Copy link
Contributor

Summary

Fixes #167

Relevant technical choices

Checklist

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

@eugene-manuilov eugene-manuilov added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images Issues related to the Images focus area labels Mar 9, 2022
@eugene-manuilov eugene-manuilov added this to the 1.0.0-beta.2 milestone Mar 9, 2022
@mitogh mitogh added the [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) label Mar 9, 2022
Copy link
Member

@mitogh mitogh left a comment

Choose a reason for hiding this comment

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

Great work on this one, few minor comments/questions.

tests/modules/images/webp-uploads/webp-uploads-test.php Outdated Show resolved Hide resolved
Comment on lines 491 to 493
if ( empty( $metadata['file'] ) ) {
return $response;
}
Copy link
Member

Choose a reason for hiding this comment

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

Curios about the need for this conditional if the file key is not accessed in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a concise way to ensure that the metadata array contains data that belongs to an image.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it follow that part, I think this sectionof code is not required due you already have this section in place few lines below:

if ( ! isset( $data['media_details']['sizes'] ) || ! is_array( $data['media_details']['sizes'] ) ) {

Which already covers that scenario.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that check here looks unnecessary. A more appropriate check if we want to avoid running the following logic unnecessarily may be empty( $metadata['sizes'] ) for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mitogh @felixarntz ok, this check is removed. 👍

modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
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.

@eugene-manuilov Nothing to add from my end other than what @mitogh has already commented on. Other than that, this looks solid 👍

eugene-manuilov and others added 2 commits March 10, 2022 11:10
Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>
Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.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.

One minor comment, but overall lgtm!

Comment on lines 491 to 493
if ( empty( $metadata['file'] ) ) {
return $response;
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree that check here looks unnecessary. A more appropriate check if we want to avoid running the following logic unnecessarily may be empty( $metadata['sizes'] ) for example.

Copy link
Member

@mitogh mitogh left a comment

Choose a reason for hiding this comment

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

👍

@felixarntz felixarntz merged commit f546557 into trunk Mar 16, 2022
@felixarntz felixarntz deleted the feature/167-source-url branch March 16, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Images Issues related to the Images focus area [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose source_url for all additional mime types via REST API
3 participants