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

Add support for shortcode embeds that enqueue scripts #9734

Merged
merged 2 commits into from
Sep 18, 2018
Merged

Add support for shortcode embeds that enqueue scripts #9734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7858550
Add support for shortcode embeds that enqueue scripts
notnownikki Sep 10, 2018
aee548d
smol cleanup
notnownikki Sep 10, 2018
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
8 changes: 8 additions & 0 deletions lib/rest-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,17 @@ function gutenberg_filter_oembed_result( $response, $handler, $request ) {
global $wp_embed;
$html = $wp_embed->shortcode( array(), $_GET['url'] );
if ( $html ) {
global $wp_scripts;
Copy link
Member

Choose a reason for hiding this comment

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

Is there an upstream Trac ticket tracking these enhancements to the oEmbed endpoint?

If so, should we add a comment about this addition?
If not, should we create 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.

I'm not sure... @pento @mkaz do you know if there's a ticket/tickets open?

Copy link
Member

Choose a reason for hiding this comment

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

There isn't yet. I was leaving this one until we're looking to merge for 5.0, as gutenberg_filter_oembed_result() is a giant hack that will be have to be mostly rewritten. So far, the things that have been merged to Core have been the ones we could mostly just copy/pasta.

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at this again in relation to the ongoing effort to remove PHP from the Gutenberg plugin. I found these:

But it's not clear from these that the changes that the revisions intended from the pull request were ever included as part of WordPress 5.0.

Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed that the original testing instructions of this pull request fail in a stock WordPress 5.0 install. Seems there's still some remaining work to port. I'll try to put together a combination Gutenberg pull request / Trac ticket.

// Check if any scripts were enqueued by the shortcode, and
// include them in the response.
$enqueued_scripts = array();
foreach ( $wp_scripts->queue as $script ) {
Copy link
Member

Choose a reason for hiding this comment

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

In my local tests, I'm still having problems when I try to embed http://pinterest.co.uk/heald0081/hair-ideas. I checked that $wp_scripts->queue is empty which I assume was unexpected.
I changed Gutenberg in a poopy.life sandbox to contain:

if ( $html ) {
					global $wp_scripts;
					var_dump( $wp_scripts->queue );
					exit;

And I checked $wp_scripts->queue was also empty there. I'm not certain about the reason for this. Maybe I'm doing something wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bet is you tested to make sure it was broken, applied the patch, then tested again? Clear all your embed transients and see if it works :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If not (it hit me a few times!) Can you check that the call to enqueued script happens in the short code?

Copy link
Member

Choose a reason for hiding this comment

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

I'm still having problems with the embed of Pinterest, I used a plugin the clear all the transients and the situation persisted. I checked that Pinterest is not a WordPress supported embed in the classic editor https://codex.wordpress.org/Embeds, but JetPack adds a shortcode for it. Do you have JetPack installed in your testing environment? maybe that explains the reason for the difference in the tests because I'm not using JetPack on the environment I'm testing.

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, this needs Jetpack with the shortcodes module active.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry for the troubles, it works great with JetPAck shortcodes module active.

$enqueued_scripts[] = $wp_scripts->registered[ $script ]->src;
}
return array(
'provider_name' => __( 'Embed Handler', 'gutenberg' ),
'html' => $html,
'scripts' => $enqueued_scripts,
);
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/embed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ export function getEmbedEdit( title, icon ) {
}

const html = 'photo' === type ? this.getPhotoHtml( preview ) : preview.html;
const { scripts } = preview;
const parsedUrl = parse( url );
const cannotPreview = includes( HOSTS_NO_PREVIEWS, parsedUrl.host.replace( /^www\./, '' ) );
// translators: %s: host providing embed content e.g: www.youtube.com
Expand All @@ -227,6 +228,7 @@ export function getEmbedEdit( title, icon ) {
<div className="wp-block-embed__wrapper">
<SandBox
html={ html }
scripts={ scripts }
title={ iframeTitle }
type={ type }
/>
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/sandbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ class Sandbox extends Component {
<head>
<title>{ this.props.title }</title>
<style dangerouslySetInnerHTML={ { __html: style } } />
{ ( this.props.scripts && this.props.scripts.map(
( src ) => <script key={ src } src={ src } />
) ) }
</head>
<body data-resizable-iframe-connected="data-resizable-iframe-connected" className={ this.props.type }>
<div dangerouslySetInnerHTML={ { __html: this.props.html } } />
Expand Down