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

Fix WordPress block resolution and embeds as reusable blocks #10035

Merged
merged 6 commits into from
Oct 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ function do_blocks( $content ) {

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.
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows embed blocks to be used as reusable blocks

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

@lkraav lkraav Oct 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
* Remove all dynamic blocks from the given content.
Expand All @@ -275,7 +275,7 @@ function strip_dynamic_blocks( $content ) {
* @return string
*/
function strip_dynamic_blocks_add_filter( $text ) {
add_filter( 'the_content', 'strip_dynamic_blocks', 8 ); // Before do_blocks().
add_filter( 'the_content', 'strip_dynamic_blocks', 6 ); // Before do_blocks().

return $text;
}
Expand Down
55 changes: 46 additions & 9 deletions packages/block-library/src/embed/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ export function getEmbedEdit( title, icon ) {
return class extends Component {
constructor() {
super( ...arguments );

this.switchBackToURLInput = this.switchBackToURLInput.bind( this );
this.setUrl = this.setUrl.bind( this );
this.maybeSwitchBlock = this.maybeSwitchBlock.bind( this );
this.getAttributesFromPreview = this.getAttributesFromPreview.bind( this );
this.setAttributesFromPreview = this.setAttributesFromPreview.bind( this );
this.setAspectRatioClassNames = this.setAspectRatioClassNames.bind( this );
this.getResponsiveHelp = this.getResponsiveHelp.bind( this );
Expand Down Expand Up @@ -91,7 +91,7 @@ export function getEmbedEdit( title, icon ) {
const switchedPreview = this.props.preview && this.props.attributes.url !== prevProps.attributes.url;
const switchedURL = this.props.attributes.url !== prevProps.attributes.url;

if ( switchedURL && this.maybeSwitchBlock() ) {
if ( ( switchedURL || ( hasPreview && ! hadPreview ) ) && this.maybeSwitchBlock() ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return;
}

Expand Down Expand Up @@ -154,7 +154,24 @@ export function getEmbedEdit( title, icon ) {
if ( includes( html, 'class="wp-embedded-content" data-secret' ) ) {
// If this is not the WordPress embed block, transform it into one.
if ( this.props.name !== 'core-embed/wordpress' ) {
this.props.onReplace( createBlock( 'core-embed/wordpress', { url } ) );
this.props.onReplace(
createBlock(
Copy link
Contributor

@talldan talldan Sep 20, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

'core-embed/wordpress',
{
url,
// By now we have the preview, but when the new block first renders, it
// won't have had all the attributes set, and so won't get the correct
// type and it won't render correctly. So, we work out the attributes
// here so that the initial render works when we switch to the WordPress
// block. This only affects the WordPress block because it can't be
// rendered in the usual Sandbox (it has a sandbox of its own) and it
// relies on the preview to set the correct render type.
...this.getAttributesFromPreview(
this.props.preview, this.props.attributes.allowResponsive
),
}
)
);
return true;
}
}
Expand Down Expand Up @@ -189,6 +206,8 @@ export function getEmbedEdit( title, icon ) {
}
}
}

return this.props.attributes.className;
}

/**
Expand All @@ -210,11 +229,14 @@ export function getEmbedEdit( title, icon ) {
}

/***
* Sets block attributes based on the preview data.
* Gets block attributes based on the preview and responsive state.
*
* @param {string} preview The preview data.
* @param {boolean} allowResponsive Apply responsive classes to fixed size content.
* @return {Object} Attributes and values.
*/
setAttributesFromPreview() {
const { setAttributes, preview } = this.props;

getAttributesFromPreview( preview, allowResponsive = true ) {
const attributes = {};
// Some plugins only return HTML with no type info, so default this to 'rich'.
let { type = 'rich' } = preview;
// If we got a provider name from the API, use it for the slug, otherwise we use the title,
Expand All @@ -227,10 +249,25 @@ export function getEmbedEdit( title, icon ) {
}

if ( html || 'photo' === type ) {
setAttributes( { type, providerNameSlug } );
attributes.type = type;
attributes.providerNameSlug = providerNameSlug;
}

this.setAspectRatioClassNames( html );
attributes.className = classnames(
this.props.attributes.className,
this.getAspectRatioClassNames( html, allowResponsive )
);

return attributes;
}

/***
* Sets block attributes based on the preview data.
*/
setAttributesFromPreview() {
const { setAttributes, preview } = this.props;
const { allowResponsive } = this.props.attributes;
setAttributes( this.getAttributesFromPreview( preview, allowResponsive ) );
}

switchBackToURLInput() {
Expand Down