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

Ensure core video block deprecations cannot be overriden #12926

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 2, 2019

Related #12358
Noticed while working on WordPress/gutenberg#16348

Currently the technique used to extend the video block's behaviour overwrites the core video block's deprecated property:

deprecated: [
{
attributes: settings.attributes,
save: settings.save,
isEligible: attrs => ! attrs.guid,
},
],

At the moment this doesn't cause any issues since the core video block doesn't have a deprecated property. If one were to be added, the overwriting that jetpack does would cause invalid blocks.

Changes proposed in this Pull Request:

  • Ensure any deprecations added in the core video block are kept by spreading them into the deprecated array.

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

  • Bug fix

Testing instructions:

Testings is a little tricky, but something like this should enable reproduction of the issue:

  1. Deactivate Jetpack
  2. Add a video block to a new post and save the post
  3. Modify the core video block so that it has a deprecation, changing the save function (e.g it doesn't matter what it's changed to, something like <div>video test</div> would work
  4. Load the post in the editor and ensure there are no validation errors
  5. Activate Jetpack
  6. Load the post in the editor again

Expected (this branch)
No validation errors should be visible.

Actual (current master)
Activating Jetpack causes a validation error.

Proposed changelog entry for your changes:

  • Ensure any deprecations added in the core video block are not overwritten

@talldan talldan added [Type] Bug When a feature is broken and / or not performing as intended [Feature] VideoPress A feature to help you upload and insert videos on your site. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jul 2, 2019
@talldan talldan requested a review from a team July 2, 2019 17:45
@talldan talldan self-assigned this Jul 2, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello talldan! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D30059-code before merging this PR. Thank you!

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 2929703

@jeherve
Copy link
Member

jeherve commented Jul 3, 2019

Is this ready for a review, or still in progress? Do you think you could update the labels accordingly?

Thank you!

@talldan talldan added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jul 3, 2019
@talldan
Copy link
Contributor Author

talldan commented Jul 3, 2019

Hey @jeherve. It should be ready for review, I've added the label. First PR in the jetpack repo, so just learning the ropes.

I'm an a11n, so I can look into D30059-code diff, but probably won't be able to sort it till next week.

Shouldn't hold up a code review though 😄

@jeherve jeherve added this to the 7.6 milestone Jul 3, 2019
@matticbot
Copy link
Contributor

talldan, Your synced wpcom patch D30059-code has been updated.

@jeherve jeherve 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 Jul 19, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve merged commit f03ff74 into master Jul 29, 2019
@jeherve jeherve deleted the fix/video-deprecation branch July 29, 2019 09:02
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants