Skip to content

Make parameters to Gutenberg filter callbacks more robust #5866

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
Merged
merged 3 commits into from
Feb 11, 2021
Merged

Make parameters to Gutenberg filter callbacks more robust #5866

merged 3 commits into from
Feb 11, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Feb 10, 2021 •
edited
Loading

Summary

Fixes a reported issue on the support forums, where depending on the Gutenberg version editing a post could produce an error like:

TypeError: Cannot read property 'type' of null
    at v (https://www.ryansmithphotography.com/wp-content/plugins/amp/assets/js/amp-block-editor.js?ver=401ab3f2d960ecfe6233b84afa6650f8:13:5950)
    at https://www.ryansmithphotography.com/wp-includes/js/dist/hooks.min.js?ver=84b89ab09cbfb4469f02183611cc0939:2:5031
    at Qt (https://www.ryansmithphotography.com/wp-includes/js/dist/blocks.min.js?ver=f4636ab86bbcd1d9adb613a053f522e7:3:114677)
    at https://www.ryansmithphotography.com/wp-includes/js/dist/block-editor.min.js?ver=ee2642fa39827fa8f0de00446089caf1:12:277174
    at we (https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:84:293)
    at zj (https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:226:496)
    at Th (https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:152:223)
    at tj (https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:152:152)
    at Te (https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:146:151)
    at https://www.ryansmithphotography.com/wp-includes/js/dist/vendor/react-dom.min.js?ver=16.13.1:61:68

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@codecov
Copy link

codecov bot commented Feb 10, 2021 •
edited
Loading

Codecov Report

Merging #5866 (b057b5f) into develop (3f6b836) will increase coverage by 0.00%.
The diff coverage is 78.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #5866   +/-   ##
==========================================
  Coverage      75.15%   75.15%           
- Complexity      5657     5663    +6     
==========================================
  Files            210      210           
  Lines          16987    17015   +28     
==========================================
+ Hits           12766    12788   +22     
- Misses          4221     4227    +6
Flag Coverage Δ Complexity Δ
javascript 75.13% <0.00%> (-0.81%) 0.00 <0.00> (ø)
php 75.15% <100.00%> (+0.03%) 0.00 <5.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
assets/src/block-editor/helpers/index.js 27.85% <0.00%> (-1.25%) 0.00 <0.00> (ø)
src/PairedRouting.php 96.30% <100.00%> (+0.29%) 117.00 <5.00> (+6.00)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2021 •
edited
Loading

Plugin builds for 0a58352 are ready 🛎️!

@pierlon pierlon requested a review from westonruter February 10, 2021 02:33
@pierlon pierlon self-assigned this Feb 10, 2021
@pierlon pierlon added Editor WS:Core Work stream for Plugin core labels Feb 10, 2021
@pierlon pierlon added this to the v2.0.11 milestone Feb 10, 2021
@westonruter westonruter requested a review from delawski February 10, 2021 02:54
westonruter
westonruter approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM, but I defer to @delawski for final review.

delawski
delawski reviewed Feb 11, 2021
View reviewed changes
Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@pierlon This looks good to me. I left a couple of very minor and optional comments.

@pierlon pierlon requested a review from delawski February 11, 2021 17:16
delawski
delawski reviewed Feb 11, 2021
View reviewed changes
assets/src/block-editor/helpers/index.js Outdated
@@ -266,6 +275,10 @@ export const getLayoutOptions = ( block ) => {
* @return {Function} Edit function.
*/
export const filterBlocksEdit = ( BlockEdit ) => {
if ( ! isObject( BlockEdit ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In light of the latest updates, should this also be changed to isFunction()? It looks that the BlockEdit is always a 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.

Yes good catch. Done in 0a58352.

@pierlon pierlon requested a review from delawski February 11, 2021 19:38
delawski
delawski approved these changes Feb 11, 2021
View reviewed changes
Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@westonruter westonruter merged commit accbaf7 into develop Feb 11, 2021
@westonruter westonruter deleted the harden/gutenberg-filters branch February 11, 2021 21:14
@westonruter westonruter modified the milestones: v2.0.11, v2.1 Feb 11, 2021
@westonruter
Copy link
Member

@pierlon This doesn't cherry-pick cleanly into 2.0 so I've re-milestoned for v2.1. The issue was reported by someone testing with 2.1-alpha anyway.

@pierlon
Copy link
Contributor Author

pierlon commented Feb 11, 2021 •
edited
Loading

@westonruter Should I open a PR specifically for 2.0 or that won't be needed?

@westonruter
Copy link
Member

I don't think anyone is reporting this as a problem in 2.0.x so it may not be needed. @jamesozzie or @milindmore22 may be able to indicate otherwise.

@schlessera schlessera self-assigned this Apr 27, 2021
@schlessera
Copy link
Collaborator

I have trouble figuring out how to specifically trigger this issue for QA testing, as I don't know what combination of Gutenberg and AMP Plugin versions were initially conflicting. @pierlon Do you remember more details?

@pierlon
Copy link
Contributor Author

pierlon commented Apr 28, 2021

@schlessera I wasn't able to determine what Gutenberg/AMP plugin combo caused the bug, unfortunately. The quickest way I can think of to replicate the error could be something like:

let _element;

addFilter( 'blocks.getSaveElement', 'hijack/invalid', ( element ) => {
	_element = element;
	return null;
}, 9 );

addFilter( 'blocks.getSaveElement', 'hijack/valid', () => _element, 11 );

@schlessera
Copy link
Collaborator

QA passed

AMP plugin at the commit immediately before this change:

Edit Post ‹ Theme Unit Test — WordPress - Google Chrome 2021-04-28 at 6 22 01 AM

After update to the latest AMP plugin and a rebuild/refresh:
Edit Post ‹ Theme Unit Test — WordPress - Google Chrome 2021-04-28 at 8 12 43 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@delawski delawski delawski approved these changes

Assignees

@schlessera schlessera

Labels
Editor WS:Core Work stream for Plugin core
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
4 participants
@pierlon @westonruter @schlessera @delawski