Skip to content

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

Merged
felixarntz merged 2 commits intotrunkfrom
fix/567-fatal-error-handling
Oct 31, 2022
Merged

Fix fatal error in REST API response when an image has no attachment metadata.#568
felixarntz merged 2 commits intotrunkfrom
fix/567-fatal-error-handling

Conversation

@mehulkaklotar
Copy link
Member

@mehulkaklotar mehulkaklotar commented Oct 28, 2022 •
edited
Loading

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
felixarntz
felixarntz reviewed Oct 28, 2022
View reviewed changes
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

mehulkaklotar commented Oct 31, 2022

@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.

felixarntz
felixarntz approved these changes Oct 31, 2022
View reviewed changes
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
adamsilverstein
adamsilverstein approved these changes Oct 31, 2022
View reviewed changes
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

Reviewers

@adamsilverstein adamsilverstein adamsilverstein approved these changes

@felixarntz felixarntz felixarntz approved these changes

@mitogh mitogh Awaiting requested review from mitogh

@jjgrainger jjgrainger Awaiting requested review from jjgrainger

Assignees

@mehulkaklotar mehulkaklotar

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

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

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

3 participants

@mehulkaklotar @adamsilverstein @felixarntz