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.

Sign up for GitHub

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

Jump to bottom

Jetpack_Media: Delete revision history items before updating metadata #14340

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Jetpack_Media: Delete revision history items before updating metadata #14340

merged 1 commit into from
Jan 21, 2020

Conversation

kbrown9
Copy link
Member

@kbrown9 kbrown9 commented Jan 15, 2020 •
edited
Loading

When a media file is remotely edited, the Jetpack_Media::edit_media_file() method is called. Within this method, the wp_generate_attachment_metadata() function is called, which updates the metadata for the updated file. Then Jetpack_Media::limit_revision_history() is called, which determines if the revision history limit has been reached.

If the revision history limit has been reached, the Jetpack_Media::delete_media_history_file()
method is called. This method calls wp_generated_attachment_metadata() on the file that is being deleted, which updates the attachment metadata. However, we already updated the attachment metadata, and this overwrites that updated metadata with the metadata for the file that's being deleted. This causes broken image URLs in the wp-admin media library.

Fixes #14315

Changes proposed in this Pull Request:

  • Call Jetpack_Media::limit_revision_history() before calling wp_generate_attachment_metadata() in the Jetpack_Media::edit_media_file() method. This will prevent the updated attachment metadata from being overwritten.
  • Fix some PHPCS warnings for comments and whitespace.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This fixes an existing part of Jetpack.

Testing instructions

Test site
  • Jetpack must be active and connected.
Test instructions
  1. Add an image to the site's media library.
  2. Navigate to Calypso admin -> Media.
  3. Edit the image.
  4. Navigate to wp-admin -> Media Library. The edited image should display correctly.
  5. Navigate to Calypso admin -> Media.
  6. Edit the image a second time.
  7. Navigate to wp-admin -> Media Library. The edited image should display correctly.

Proposed changelog entry for your changes:

  • Fix a bug that broke image urls if they were remotely updated multiple times.

@kbrown9
Jetpack_Media: Delete revision history items before updating attachme…
51b9f94 
…nt metadata

Fixes Issue # 14315

When a media file is remotely edited, the Jetpack_Media::edit_media_file() method is
called. Within this method, the wp_generate_attachment_metadata() function is called.
Then the Jetpack_Media::limit_revision_history() method is called, which
determines if the revision history limit has been reached.

If the revision history limit has been reached, the Jetpack_Media::delete_media_history_file()
method is called. This method calls wp_generated_attachment_metadata(), which overwrites
the attachment metadata that was just updated with the attachment metadata of the revision that
is about to be deleted. This results in broken image URLs in the wp-admin media library.

To fix this, call Jetpack_Media::limit_revision_history() before calling
wp_generate_attachment_metadata() in the Jetpack_Media::edit_media_file() method. This will
prevent the updated attachment metadata from being overwritten.

Finally, fix some PHPCS warnings for comments and whitespace.
@kbrown9 kbrown9 added [Type] Bug When a feature is broken and / or not performing as intended [Feature] WPCOM API [Status] In Progress [Pri] Normal labels Jan 15, 2020
@kbrown9 kbrown9 added this to the 8.2 milestone Jan 15, 2020
@kbrown9 kbrown9 requested a review from a team January 15, 2020 05:54
@kbrown9 kbrown9 self-assigned this Jan 15, 2020
@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against 51b9f94

@kbrown9 kbrown9 added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 16, 2020
zinigor
zinigor approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Works great, the thumbnail no longer disappears after a couple of edits.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 17, 2020
@jeherve jeherve merged commit 7dc71a5 into master Jan 21, 2020
@jeherve jeherve deleted the fix/delete_rev_history_before_update branch January 21, 2020 10:53
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 21, 2020
jeherve added a commit that referenced this pull request Jan 27, 2020
@jeherve
Changelog: add #14340
0fe5a34
jeherve added a commit that referenced this pull request Jan 28, 2020
@jeherve
Changelog: 8.2 release (#14332)
0589a49 
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

Assignees

@kbrown9 kbrown9

Labels
[Feature] WPCOM API [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.

Media: images do not display in media library after multiple remote edits
5 participants
@kbrown9 @jetpackbot @zinigor @jeherve @matticbot