Skip to content

Fix fatal error in REST API response when an image has no attachment metadata. #568

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

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

mehulkaklotar
Copy link
Member

@mehulkaklotar mehulkaklotar commented Oct 28, 2022

Summary

This bug is very unlikely to happen as it depends on attachment metadata. It is assumed that metadata should always be present for the attachment but when metadata is deleted from DB, we should ensure proper error handling by checking the data from REST.

Fixes #567

Relevant technical choices

Added more checks for media_details in response of REST API for attachments

Checklist

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

@mehulkaklotar mehulkaklotar added [Type] Bug An existing feature is broken [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Oct 28, 2022
@mehulkaklotar mehulkaklotar added this to the 1.7.0 milestone Oct 28, 2022
@mehulkaklotar mehulkaklotar self-assigned this Oct 28, 2022
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.

@mehulkaklotar Fix generally looks reasonable, but please add a unit test for the bug, so that we can verify it is fixed.

In the test, you can use the rest_prepare_attachment to e.g. make $data['media_details'] an object. With that test, we should then verify whether the old code would fail vs the new code would pass.

@mehulkaklotar
Copy link
Member Author

@felixarntz I have added the new unit test for checking media_details in the attachment response data in the latest commit. Please review it once.

I have used the REST controller's get_item and deleted the attachment metadata before to get the media_details as an object because rest_prepare_attachment doesn't have control over changing the data in the response object in the test.

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.

@mehulkaklotar Thank you, the test looks excellent!

@felixarntz felixarntz requested a review from jjgrainger October 31, 2022 19:44
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Great!

@felixarntz felixarntz changed the title Fix fatal error in rest api response for sizes in media details Fix fatal error in REST API response when an image has no attachment metadata. Oct 31, 2022
@felixarntz felixarntz merged commit 56bf94f into trunk Oct 31, 2022
@felixarntz felixarntz deleted the fix/567-fatal-error-handling branch October 31, 2022 23:48
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] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error handling: Cannot use object of type stdClass as array
3 participants