Fix WordPress block resolution and embeds as reusable blocks by notnownikki · Pull Request #10035 · WordPress/gutenberg

notnownikki · 2018-09-19T15:28:39Z

Description

There were a couple of problems using Embed blocks as Reusable blocks.

Pasting a WordPress URL created an embed block, but did not resolve the block to a WordPress block. This meant that if you tried to edit a post with the resulting block in it, and converted it to a Reusable block, it would crash. The crash was caused because the new block had the preview data already available, and so the block resolution would kick in and try to replace it with a WordPress block, but this.props.onReplace was not available, so, we got the crash seen in Weird behavior trying to save a WordPress embed as reusable block #9938 . This is fixed in this PR by making sure that the block resolution works and carries across all attributes set on the Embed block when we're embedding WordPress content. It's more complex for WordPress embeds because we need to get back the embed preview to detect it's WordPress and switch to the correct block. We can't go by the URL as we can for the other blocks because there is no regex that can determine a URL is WordPress.
The do_blocks filter ran before do_shortcode but after oembed, so reusable embed blocks did not get their URL processed. This PR changes the priority to fix this.

Fixes: #9938

How has this been tested?

Run through the steps in #9938

The embed block should become reusable, and render correctly on the published post.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

notnownikki · 2018-09-19T16:07:29Z

lib/blocks.php

 	return $rendered_content;
 }
-add_filter( 'the_content', 'do_blocks', 9 ); // BEFORE do_shortcode().
+add_filter( 'the_content', 'do_blocks', 7 ); // BEFORE do_shortcode() and oembed.


Allows embed blocks to be used as reusable blocks

I think this change broke the dynamic blocks in the frontend if you have any meta box in the editor.

@youknowriad this change has even more consequences. For one, it completely invalidates gutenberg_wpautop() who will have has_blocks() check always fail (because do_blocks() already killed markup), causing gutenberg_wpautop() to immediately execute wpautop() on everything. I wouldn't be surprised if that's what's causing #11031 and other new broken autop rendering issues popping up now.

yes, I noticed too. We're working on fixes in #11050

notnownikki · 2018-09-19T16:08:34Z

packages/block-library/src/embed/index.js

 			const switchedURL = this.props.attributes.url !== prevProps.attributes.url;

-			if ( switchedURL && this.maybeSwitchBlock() ) {
+			if ( ( switchedURL || ( hasPreview && ! hadPreview ) ) && this.maybeSwitchBlock() ) {


Try switching the block if the URL changes, or we get a preview. To switch to a WordPress block, we need the preview.

talldan · 2018-09-20T17:28:06Z

packages/block-library/src/embed/index.js


 			if ( ! iframe ) {
-				return;
+				return this.props.attributes.className;


This function could be refactored a bit to avoid so many returns. Could remove this if statement, change the one below to if( iframe && iframe.height && iframe.width ), and then move the remaining return this.props.attributes.className; to the very last line of the function.

Gah yes, I missed a refactor pass!

talldan · 2018-09-20T17:39:01Z

packages/block-library/src/embed/index.js

 					if ( this.props.name !== 'core-embed/wordpress' ) {
-						this.props.onReplace( createBlock( 'core-embed/wordpress', { url } ) );
+						this.props.onReplace(
+							createBlock(


Not really a thing to fix in this PR, but I thought i'd mention that it'd be tidier if maybeSwitchBlock returned the value of createBlock so that it becomes purer and somewhat side-effect free (depending on what createBlock does).

Its caller could call this.props.onReplace with the result (if there is one).

Yes, too many side effects caused this problem. Separating the preview->attribute generation from the code that set the attributes let us fix this, same should be done here. Once this is in I'll do it as a chore PR.

talldan

I'm not all that familiar with some of the code being changed, so I think it'd worth getting someone with more familiarity to review as well.

I tested though and the fix works well. Also seems like it'd be a good fix to get in, as the editor crashes whenever hovering over the reusable block in the block switcher (I imagine that's the preview), so it's quite a nasty bug!

notnownikki · 2018-09-21T08:55:27Z

@talldan thanks :) I've added it to the 4.0 milestone so it should get into the review queue for other peeps. There will be some happy people on my twitter timeline once 4.0 gets out!!

…block-crash

notnownikki · 2018-09-28T09:22:18Z

Merged master to pick up the responsiveness changes there.

gziolo · 2018-10-19T09:10:41Z

@talldan can you do another round of testing and merge it if everything goes well?

talldan

I gave it a good test and no issues, and the explanations make sense.

A few things in the embed block could do with a bit of a tidy up, but that's not really the fault of this PR - so lets get it in!

notnownikki · 2018-10-19T09:51:41Z

@talldan an embed block refactor and tidy and unit test is on my list of stuff to do. It's kinda of grown organically... like a mold.

gziolo · 2018-10-19T09:53:09Z

Kudos @talldan for doing another review so quickly, much appreciated 💯

…ss#10035) * Fix WordPress block resolution and embeds as reusable blocks * Let's not infinitely recurse and crash. * Reduce the amount of return points * Doc update

notnownikki added 2 commits September 19, 2018 16:18

Fix WordPress block resolution and embeds as reusable blocks

31cb652

Let's not infinitely recurse and crash.

958d6dc

notnownikki commented Sep 19, 2018

View reviewed changes

notnownikki requested review from aduth and talldan September 19, 2018 16:08

bph mentioned this pull request Sep 19, 2018

Reusable Block: Embed block only shows URL on frontend #9937

Closed

chrisvanpatten mentioned this pull request Sep 20, 2018

Vimeo Embed doesn't work as reusable block #10072

Closed

talldan reviewed Sep 20, 2018

View reviewed changes

Reduce the amount of return points

a1c299f

notnownikki added [Type] Bug An existing feature does not function as intended [Feature] Blocks Overall functionality of blocks labels Sep 21, 2018

notnownikki added this to the 4.0 milestone Sep 21, 2018

notnownikki mentioned this pull request Sep 28, 2018

Use best fitting embed aspect ratio if exact match doesn't exist #10213

Merged

4 tasks

Merge remote-tracking branch 'origin/master' into fix/reusable-embed-…

fa7bd4c

…block-crash

notnownikki requested a review from a team September 28, 2018 09:22

notnownikki added 2 commits September 28, 2018 10:51

Doc update

953c26b

Merge branch 'master' into fix/reusable-embed-block-crash

187f800

mcsf modified the milestones: 4.0, 4.1 Oct 5, 2018

talldan approved these changes Oct 19, 2018

View reviewed changes

talldan merged commit 3168158 into master Oct 19, 2018

talldan deleted the fix/reusable-embed-block-crash branch October 19, 2018 09:50

mtias added the [Block] Embed Affects the Embed Block label Oct 19, 2018

youknowriad mentioned this pull request Oct 25, 2018

Fix dynamic blocks not rendering in the frontend #11050

Merged

eoinsav mentioned this pull request Oct 25, 2019

Reusable Block: Embed block only shows URL on frontend #18098

Closed

aduth mentioned this pull request Apr 8, 2020

Fix embedded blocks when converted to reusable blocks #21043

Closed

Conversation

notnownikki commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Types of changes

Checklist:

Uh oh!

notnownikki Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

youknowriad Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

lkraav Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

notnownikki Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

talldan Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

notnownikki Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

talldan Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notnownikki Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

talldan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notnownikki commented Sep 21, 2018

Uh oh!

notnownikki commented Sep 28, 2018

Uh oh!

gziolo commented Oct 19, 2018

Uh oh!

talldan left a comment

Choose a reason for hiding this comment

Uh oh!

notnownikki commented Oct 19, 2018

Uh oh!

gziolo commented Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

notnownikki commented Sep 19, 2018 •

edited

Loading

lkraav Oct 25, 2018 •

edited

Loading

talldan Sep 20, 2018 •

edited

Loading

talldan left a comment •

edited

Loading