Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 14 months ago

#44632 closed enhancement (fixed)

Properly enqueue wp-embed

Reported by: alpipego's profile alpipego Owned by: audrasjb's profile audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch has-unit-tests dev-reviewed commit
Focuses: javascript, performance Cc:

Description

The wp-embed JS gets enqueued on wp_head instead of wp_enqueue_scripts source:trunk/src/wp-includes/default-filters.php#L532; thus it gets printed in wp_print_footer_scripts although it's not registered that way.

The script should be properly registered with 'in_footer' = true and enqueued on wp_enqueue_scripts.

I'll be happy to provide the patch, if desired.

Attachments (2)

44632.diff (561 bytes) - added by ocean90 3 years ago.
44632.2.diff (1.6 KB) - added by westonruter 3 years ago.

Download all attachments as: .zip

Change History (60)

#1 @swissspidy
6 years ago

I can't remember off the top of my head why we added it this way in the feature plugin...

If we change this behavior we have to make sure it doesn't break anything.

@ocean90
3 years ago

#2 @ocean90
3 years ago

  • Keywords has-patch added

We probably can't change how the script is enqueued since it would break any usage of remove_action( 'wp_head', 'wp_oembed_add_host_js' );. But I don't see any harm in setting the in_footer argument as done in 44632.diff.

Possible workaround:

<?php
/**
 * Improves handling of `wp-embed` script enqueue.
 *
 * By default the script is enqueued on all pages, registered on `wp_head` with
 * priority 10. This prevents third-party plugins to handle this properly which
 * expect all scripts/styles to be registered on `wp_enqueue_scripts`, also hooked
 * into `wp_head` with priority 1.
 *
 * This function unregisters the default action and registers it via `wp_enqueue_scripts`.
 * It also makes sure to enqueue the script in the footer which it isn't by default.
 *
 * @link https://core.trac.wordpress.org/ticket/44632
 */
function trac_44632_wp_embed(): void {
        if ( ! has_action( 'wp_head', 'wp_oembed_add_host_js' ) ) {
                return;
        };

        remove_action( 'wp_head', 'wp_oembed_add_host_js' );

        wp_scripts()->add_data( 'wp-embed', 'group', 1 );
        add_action( 'wp_enqueue_scripts', 'wp_oembed_add_host_js' );
}
add_action( 'wp_head', 'trac_44632_wp_embed', 0 );

@westonruter
3 years ago

#3 follow-up: @westonruter
3 years ago

Related to this is #44306 in which someone reports the messages from the iframes are being sent too early when the script is being printed in the footer. But the footer is where this should be printed regardless (or at least not in the head).

Nevertheless, I also just noticed that the wp-embed script is printed on every page unconditionally. Isn't this incorrect and a negative impact on page performance? Shouldn't it only ever be printed if there is an oEmbed on the page which actually outputs an iframe with the wp-embedded-content class?

Secondly, this script seems it would benefit from the defer attribute being added to it. But the race condition described in #44306 should be figured out. This may entail inlining the script before the first iframe.wp-embedded-content instead. So what about doing just that? What about filtering embed_oembed_html to prepend an inlined wp-embed script before post embeds? See 44632.2.diff.

#4 follow-up: @westonruter
3 years ago

I see there is also already such inlining being done in get_post_embed_html() for internal post embeds. Perhaps that just needs to be done for all post embeds, including external ones.

This ticket was mentioned in PR #1745 on WordPress/wordpress-develop by westonruter.


3 years ago
#5

  • Keywords has-unit-tests added
  • Prevent loading wp-embed script unconditionally on every page (in the footer) in favor of conditionally printing the script inline before post embeds. By printing the script inline before the post embed, the race condition reported in Core-44306 should also be addressed.

Trac ticket: https://core.trac.wordpress.org/ticket/44632

#6 @westonruter
3 years ago

Pull request implements the inlining of wp-embed.js for both oEmbed post embeds and the "HTML Embed" markup provided for users to copy. It also fixes what seems to be bug in the latter, where the markup didn't include the secret on the blockquote and iframe meaning the blockquote would not be hidden.

#7 @westonruter
3 years ago

  • Focuses javascript performance added
  • Milestone changed from Awaiting Review to 5.9

#8 @westonruter
3 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

#9 in reply to: ↑ 3 ; follow-up: @swissspidy
3 years ago

Replying to westonruter:

Nevertheless, I also just noticed that the wp-embed script is printed on every page unconditionally. Isn't this incorrect and a negative impact on page performance? Shouldn't it only ever be printed if there is an oEmbed on the page which actually outputs an iframe with the wp-embedded-content class?

I would have to do some digging to recall why we originally added the script in the header. But because it's printed in the header, it needs to be done unconditionally, as it's too early to know whether it's actually going to be needed or not.

Similar to the wp-emoji JS.

Secondly, this script seems it would benefit from the defer attribute being added to it. But the race condition described in #44306 should be figured out. This may entail inlining the script before the first iframe.wp-embedded-content instead. So what about doing just that? What about filtering embed_oembed_html to prepend an inlined wp-embed script before post embeds? See 44632.2.diff.

AFAIK in such a situation the browser will pause parsing the HTML until the script has been loaded, so that's not ideal.

Just printing in the footer sounds reasonable to me.

To solve the race condition, I think wp-embed-template.js should

  • not send the message on load if the secret is missing
  • send the message as soon as the secret has been added (which means wp-embed.js has been loaded)
Version 0, edited 3 years ago by swissspidy (next)

#10 in reply to: ↑ 4 @swissspidy
3 years ago

Replying to westonruter:

I see there is also already such inlining being done in get_post_embed_html() for internal post embeds. Perhaps that just needs to be done for all post embeds, including external ones.

That's not what the function does. get_post_embed_html() is for providing the embed code via the oEmbed restpoint, in case other platforms want to embed a blog post.

swissspidy commented on PR #1745:


3 years ago
#11

  • Prevent loading wp-embed script unconditionally on every page (in the footer) in favor of conditionally printing the script inline before post embeds. By printing the script inline before the post embed, the race condition reported in Core-44306 should also be addressed.

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

Why can't this be 1 external script in the footer? With wp-embed-template.js being updated to only send the postMessage once it has a secret.

  • Add missing secret to blockquote and iframe in embed code provided for users to copy. This ensures that the inlined wp-embed.js script can hide the blockquote when the iframe loads.

That's a weird one, I thought https://core.trac.wordpress.org/ticket/34451 solved that years ago, but seems like it didn't.

So it sounds like you're testing this on non-WP sites? Otherwise I don't see why this is necessary 🤔

In WP, wp_filter_oembed_result adds these attributes already

westonruter commented on PR #1745:


3 years ago
#12

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

While not ideal, this is standard for embeds. When embedding tweets for example, every single copy will include the same script: <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>. At least here the script is inlined and subsequent copies would short-circuit:

https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/wp/embed.js#L26-L28

Why can't this be 1 external script in the footer? With wp-embed-template.js being updated to only send the postMessage once it has a secret.

I think for the reason outlined in Core-44306. Namely, if there is a post embed and the iframe for that post embed loads before the page finishes loading and before the wp-embed.js script executes, then it could be that the iframe'd post embed window could call postMessage _before_ wp.receiveEmbedMessage() has been added as the message event listener, and then it wouldn't get sized. So the message event should be added before the iframe occurs on the page to be the most robust.

That's a weird one, I thought https://core.trac.wordpress.org/ticket/34451 solved that years ago, but seems like it didn't.

So it sounds like you're testing this on non-WP sites? Otherwise I don't see why this is necessary 🤔

Yes, for example: https://wp-embed-script-loading.glitch.me/#current-html-embed-code

https://i0.wp.com/user-images.githubusercontent.com/134745/136827788-4fe9b032-d6a3-447c-8ff3-777f1f9fbda7.png

I copied the HTML Embed code to embed on another site, and you can see that the blockquote doesn't get hidden as it does when it is embedded in WordPress:

https://i0.wp.com/user-images.githubusercontent.com/134745/136827848-d24a92b8-138e-4947-ab3a-50297bc6bb88.png

In WP, wp_filter_oembed_result adds these attributes already

Humm. But wp_filter_oembed_result isn't running on the embed_html filter. So yeah, it seems the non-WP embedding is the problem I was trying to fix.

#13 in reply to: ↑ 9 @westonruter
3 years ago

Replying to swissspidy:

I would have to do some digging to recall why we originally added the script in the header. But because it's printed in the header, it needs to be done unconditionally, as it's too early to know whether it's actually going to be needed or not.

I think the reason may be related to #44306, where the message event should be added before iframes load.

To get around this, the script could be printed right before the post embeds appear on the page. You're right that this is not ideal since it does block rendering, but better multiple blocking inline scripts than a one external one?

To solve the race condition, I think wp-embed-template.js should

  • not send the message on load if the secret is missing
  • send the message as soon as the secret has been added (which means wp-embed.js has been loaded)

But wp_filter_oembed_result is adding the secret to the initial HTML, so it doesn't seem that watching for the secret location hash will work since it would always be present as I see?

Just printing in the footer sounds reasonable to me.

If the race condition message passing situation can be figured out, then great. But I think this should also conditionally print the script in the footer if there was actually any instance of a post embed printed on the page.

#14 follow-up: @swissspidy
3 years ago

But wp_filter_oembed_result is adding the secret to the initial HTML, so it doesn't seem that watching for the secret location hash will work since it would always be present as I see?

Ah, right.

Before I typed that, I actually wanted to propose another idea:

The host script (wp-embed.js) could send a read postMessage to all the embeds on the page once it's loaded. So the individual embeds could respond to that by sending the height message in reply.

So the flow would be:

  1. Host site renders all embeds
  2. All iframes send their height message on load, just like today
  3. wp-embed.js finishes loading, sends ready message to all iframes
  4. All iframes send their height message in response to the ready message

Of course, if the script somehow would be loaded already earlier, the flow would be simpler:

  1. Host site renders all embeds
  2. wp-embed.js finishes loading, sends ready message to all iframes
  3. If an iframe receives the ready message but isn't loaded yet, it doesn't do anything
  4. All iframes send their height message on load, ignoring the ready which was received

But I think this should also conditionally print the script in the footer if there was actually any instance of a post embed printed on the page.

Definitely.

swissspidy commented on PR #1745:


3 years ago
#15

This causes lots of duplicated inline scripts on the page, doesn't it? That doesn't sound ideal.

While not ideal, this is standard for embeds.

Right. For non-WP it makes sense like this, but we should keep avoiding this in WP context like we already do today.

I think for the reason outlined in Core-44306. [...]

Let's try to nail the race condition fix before attempting to improve loading order. A good opportunity to hit two birds with one stone.

#16 in reply to: ↑ 14 @westonruter
3 years ago

Replying to swissspidy:

The host script (wp-embed.js) could send a read postMessage to all the embeds on the page once it's loaded. So the individual embeds could respond to that by sending the height message in reply.

So it sounds like (1) iframed windows should send the height as soon as they load, (2) they should also send the height when the parent page requests it (in addition to when the page is resized):

  • src/js/_enqueues/wp/embed.js

    a b  
    104104                for ( i = 0; i < iframes.length; i++ ) {
    105105                        source = iframes[ i ];
    106106
    107                         if ( ! source.getAttribute( 'data-secret' ) ) {
     107                        secret = source.getAttribute( 'data-secret' );
     108                        if ( ! secret ) {
    108109                                /* Add secret to iframe */
    109110                                secret = Math.random().toString( 36 ).substr( 2, 10 );
    110111                                source.src += '#?secret=' + secret;
     
    117118                                iframeClone.removeAttribute( 'security' );
    118119                                source.parentNode.replaceChild( iframeClone, source );
    119120                        }
     121
     122                        source.contentWindow.postMessage( {
     123                                message: 'ready',
     124                                secret: secret
     125                        } );
    120126                }
    121127        }

And then wp-embed-template.js would to add a message event listener to listen for ready.

If the height didn't change, then the message should no-op, doing something like this:

  • src/js/_enqueues/wp/embed.js

    a b  
    5757                                continue;
    5858                        }
    5959
    60                         source.removeAttribute( 'style' );
    61 
    6260                        /* Resize the iframe on request. */
    63                         if ( 'height' === data.message ) {
     61                        if ( 'height' === data.message && data.value !== source.height ) {
     62                                source.removeAttribute( 'style' );
     63
    6464                                height = parseInt( data.value, 10 );
    6565                                if ( height > 1000 ) {
    6666                                        height = 1000;

The new ready message being sent from the host to the post embed would depend, naturally, on both WordPress sites being updated to the latest version of WordPress.

Is this what you have in mind?

#17 @swissspidy
3 years ago

Yup, precisely.

Both sites needing to be updated is not ideal short term, but this new ready message makes this more stable long term.

felixarntz commented on PR #1745:


3 years ago
#18

@westonruter @swissspidy I wonder whether we could go an alternate direction where we tweak a bit of the JS instead of focusing on only PHP-side changes. I think with a few JS changes we could get to a place where the loading order of the JS file doesn't matter as much, so we could conditionally enqueue it in the footer - I believe that's our preferred goal anyway, as it ensures the script is only loaded once and only if needed.

Potentially we could do something like the following:

  • Introduce a new very simple inline script that is printed on wp_head, which does the following:
    • Listen to the message event.
    • In the callback, add the event.data to an array of messages, e.g. a wp.messageQueue.
  • Update wp-embed.js to check if there is a non-empty wp.messageQueue and if so, iterate through it run the callback for each, or potentially even manually retrigger a message event on window.

Having a queue that listens for messages right away would allow us to load the actual wp-embed script at any point later. Of course the approach here still leaves us with one unconditionally loaded script, but that script would be inlined and probably be around 5 lines of code unminified. Last but not least, having this message queue could also help with any other scripts relying on message events, so it would facilitate e.g. plugin developers being able to load their message-dependent scripts in the footer as well, since they could just look at wp.messageQueue on load.

Thoughts?

westonruter commented on PR #1745:


3 years ago
#19

Discussing over chat now…

swissspidy commented on PR #1745:


3 years ago
#20

Just so we all have the same context, have you also seen the idea discussed at https://core.trac.wordpress.org/ticket/44632#comment:16?

Loading wp-embed in the footer with the idea discussed above solves the reported performance issue _and_ the race condition. What would be the advantage of a message queue over that?

How would that inline script stop listening to messages once wp-embed is loaded / how is the queue cleared? You'd want to avoid reading the same message twice.
Is there even a use case for a general purpose postMessage queue for for use by plugin developers?

Right now the two-way communication approach outlined in https://core.trac.wordpress.org/ticket/44632#comment:16 (and wp-embed loaded in the footer) seems more preferable IMO.
It would also solve the race condition issues when embedding on non-WordPress sites. Something which the postMessage queue wouldn't cover.

felixarntz commented on PR #1745:


3 years ago
#21

@swissspidy

Just so we all have the same context, have you also seen the idea discussed at https://core.trac.wordpress.org/ticket/44632#comment:16?

I hadn't, until @westonruter pinged it to me :)

What would be the advantage of a message queue over that?

My initial concern was that this would cause problems between WP versions with the enhancement and without the enhancement since they would never send/receive the ready event. But @westonruter clarified we'd send the message events twice, once on load and once on ready, so in that case my concern is alleviated.

Is there even a use case for a general purpose postMessage queue for for use by plugin developers?

There always is - but honestly that would have just been a nice side-effect, not a primary "selling point" for this approach.

It would also solve the race condition issues when embedding on non-WordPress sites. Something which the postMessage queue wouldn't cover.

Can you elaborate on that? I'm not sure I understand these race conditions with non-WP, I thought we're only speaking about the issues with wp-embed.js.

Anyway, after a clarifying chat with @westonruter, I'm onboard with the latest approach suggested in the ticket. Let's preferably keep the technical discussion in the PR so that it's available in both places :)

#22 @hellofromTonya
3 years ago

@westonruter is this on your radar to finish before 5.9 feature freeze (end of day Nov 9th)?

#23 @westonruter
3 years ago

Yes, I'm working on it tomorrow.

#24 @westonruter
3 years ago

Nevertheless, this should really be rather considered a bug than an enhancement.

#25 @hellofromTonya
3 years ago

Thanks, Weston for the update as well as more context about it's current enhancement type. If it goes past feature freeze, let's change it to defect.

westonruter commented on PR #1745:


3 years ago
#26

OK, this is now conditionally enqueueing wp-embed.js only when there is a post embed on the page. What's left is to address the race condition from Core-44306, which involves the ready message being sent from the host page to the post embed windows.

westonruter commented on PR #1745:


3 years ago
#27

OK, this is now conditionally enqueueing wp-embed.js only when there is a post embed on the page. What's left is to address the race condition from Core-44306, which involves the ready message being sent from the host page to the post embed windows.

#28 @westonruter
3 years ago

  • Version set to 4.4

Ready for review: https://github.com/WordPress/wordpress-develop/pull/1745

See PR description for proposed changes:

  • Prevent loading wp-embed script unconditionally on every page in favor of conditionally enqueueing when a post oEmbed is detected. The wp-embed script is also explicitly marked as being in the footer group. Sites which currently disable post embed scripts from being added via remove_action( 'wp_head', 'wp_oembed_add_host_js' ); will continue to do so.
  • Send a ready message from the host page to each post embed window in case the iframe loads before the wp-embed.js script. When the ready message is received by the post embed window, it sends a the same height message as it sends when it loads. This addresses #44306. To reproduce this issue, see example plugin.
  • Eliminate use of grunt-include to inject emoji script and the post embed script. Instead obtain the script contents via file_get_contents() (as is done elsewhere in core) and utilize wp_print_inline_script_tag()/wp_get_inline_script_tag() to write out the script. This simplifies the logic and allows the running of src without SCRIPT_DEBUG enabled.
  • For the embed code that users are provided to copy (for embedding outside of WP), add the missing inlined wp-embed.js script and the secret on the blockquote/iframe. This ensures the blockquote will be hidden when the iframe loads. The embed code in question is accessed here (and via get_post_embed_html()

...

#29 @westonruter
3 years ago

With the introduction of the ready message, we'll be able to safely add async to the wp-embed script as well, eventually. I'd also love to do so for comment-reply.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

westonruter commented on PR #1745:


3 years ago
#31

@swissspidy Do you want to test more before I commit?

swissspidy commented on PR #1745:


3 years ago
#32

@westonruter Ship it!

westonruter commented on PR #1745:


3 years ago
#33

Rebasing onto latest trunk prior to commit…

#34 @westonruter
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 52132:

Embeds: Conditionally enqueue wp-embed only if needed and send ready message in case script loads after post embed windows.

  • Prevent loading wp-embed script unconditionally on every page in favor of conditionally enqueueing when a post embed is detected. The wp-embed script is also explicitly marked as being in the footer group. Sites which currently disable post embed scripts from being enqueued via remove_action( 'wp_head', 'wp_oembed_add_host_js' ) will continue to do so.
  • Send a ready message from the host page to each post embed window in case the iframe loads before the wp-embed script does. When the ready message is received by the post embed window, it sends the same height message as it sends when it loads.
  • Eliminate use of grunt-include to inject emoji script and the post embed script. Instead obtain the script contents via file_get_contents() (as is done elsewhere in core) and utilize wp_print_inline_script_tag()/wp_get_inline_script_tag() to construct out the script. This simplifies the logic and allows the running of src without SCRIPT_DEBUG enabled.
  • For the embed code that users are provided to copy for embedding outside of WP, add the secret on the blockquote and iframe. This ensures the blockquote will be hidden when the iframe loads. The embed code in question is accessed here via get_post_embed_html().

Props westonruter, swissspidy, pento, flixos90, ocean90.
Fixes #44632, #44306.

westonruter commented on PR #1745:


3 years ago
#35

Committed to trunk in r52132.

This ticket was mentioned in PR #1878 on WordPress/wordpress-develop by westonruter.


3 years ago
#36

This fixes something I neglected in https://github.com/WordPress/wordpress-develop/pull/1745 when I removed the use of grunt-include to do the script injection. I failed to see that it was also being used in print_embed_styles() and print_embed_scripts() to include wp-embed-template.min.css and wp-embed-template.min.js respectively when SCRIPT_DEBUG is off. The result was the embed template erroneously serving:

{{{html
<style>
include "css/wp-embed-template.min.css"
</style>
...
<script>
include "js/wp-embed-template.min.js"
</script>
}}}

This naturally caused a JS syntax error, as well as broken styling.

With the changes in this PR, when SCRIPT_DEBUG is enabled or disabled, the result is:

Before | After


https://i0.wp.com/user-images.githubusercontent.com/134745/141595658-6b34eb9d-909e-4848-a52d-a60a62ad07ac.png | https://i0.wp.com/user-images.githubusercontent.com/134745/141595663-bd861607-4a8a-4717-b993-935740fbd4d9.png

Trac ticket: https://core.trac.wordpress.org/ticket/44632

#37 @westonruter
3 years ago

In 52151:

Embeds: Fix inclusion of wp-embed-template script and style when SCRIPT_DEBUG is disabled.

Amends [52132].
See #44632.

westonruter commented on PR #1878:


3 years ago
#38

Committed to trunk in r52151.

#39 @westonruter
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I found an issue issue with [52132] where prepending the wp-embed script to the get_post_embed_html() response is resulting in a regex parsing issue in wp_filter_oembed_result(). It is failing to extract the blockquote from the string.

This ticket was mentioned in PR #1879 on WordPress/wordpress-develop by westonruter.


3 years ago
#40

In testing post embeds from a site running trunk, I was surprised to find that wp_filter_oembed_result() was no longer able to parse out the <blockquote> here:

https://github.com/WordPress/wordpress-develop/blob/1e6bede076285c9fbafad7ce5a3cb4b1d153e56b/src/wp-includes/embed.php#L919-L924

As of #1745, get_post_embed_html() prepends the wp-embed script before the <blockquote> the resulting markup looks like this:

{{{html
<script>...</script>
<blockquote><a href="https://make.wordpress.org/core/2021/10/12/proposal-for-a-performance-team/">Proposal for a Performance team</a></blockquote><iframe title="&#8220;Proposal for a Performance team&#8221; &#8212; Make WordPress Core" src="https://make.wordpress.org/core/2021/10/12/proposal-for-a-performance-team/embed/#?secret=U4GcbJWnud" width="500" height="282" frameborder="0" marginwidth="0" marginheight="0" scrolling="no"></iframe>
}}}

After wp_kses() strips out the <script> and </script>, we're left with the raw script text contents preceding the initial <blockquote> (here ...).

Given the regex |(<blockquote>.*?</blockquote>)?.*(<iframe.*?></iframe>)|ms, the initial group is marked as optional so if it is not at the very beginning of the string (as it isn't here), then any initial text followed by anything including a <blockquote> will be matched by .* which is not a group, until ultimately only the <iframe> gets matched:

[[Image(https://i0.wp.com/user-images.githubusercontent.com/134745/141609075-8911cc9a-c33e-4372-b455-94507e32e0a9.png)]]

The effect then that the <blockquote> is not included in the response as a fallback, and instead the rendered HTML is just:

{{{html
<p><iframe class="wp-embedded-content" sandbox="allow-scripts" security="restricted" title="&#8220;Embeds Changes in WordPress 4.5&#8221; &#8212; Make WordPress Core" src="https://make.wordpress.org/core/2016/03/11/embeds-changes-in-wordpress-4-5/embed/#?secret=erWLJU2ZRi#?secret=EI7GbHNMoS" data-secret="EI7GbHNMoS" width="600" height="338" frameborder="0" marginwidth="0" marginheight="0" scrolling="no"></iframe></p>
}}}

When it should rather be:

{{{html
<blockquote class="wp-embedded-content" data-secret="P6BHfgvygr"><p><a href="https://make.wordpress.org/core/2016/03/11/embeds-changes-in-wordpress-4-5/">Embeds Changes in WordPress 4.5</a></p></blockquote>
<p><iframe class="wp-embedded-content" sandbox="allow-scripts" security="restricted" style="position: absolute; clip: rect(1px, 1px, 1px, 1px);" title="&#8220;Embeds Changes in WordPress 4.5&#8221; &#8212; Make WordPress Core" src="https://make.wordpress.org/core/2016/03/11/embeds-changes-in-wordpress-4-5/embed/#?secret=f1CCnegK47#?secret=P6BHfgvygr" data-secret="P6BHfgvygr" width="600" height="338" frameborder="0" marginwidth="0" marginheight="0" scrolling="no"></iframe>
}}}

Simply appending wp-embed fixes the regex parsing problem.

Trac ticket: https://core.trac.wordpress.org/ticket/44632

#41 @westonruter
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52153:

Embeds: Fix parsing of post embeds in wp_filter_oembed_result() by appending wp-embed script instead of prepending it in get_post_embed_html().

Due to the way that the blockquote and iframe are being parsed with a regular expression in wp_filter_oembed_result(), if there is any content at all before the blockquote start tag then it will fail to be included in the first matching group. By appending the wp-embed script instead of prepending it in get_post_embed_html(), then the parsing issue is avoided.

Also use non-greedy match wp_maybe_enqueue_oembed_host_js().

Amends [52132].
Fixes #44632.

westonruter commented on PR #1879:


3 years ago
#42

Committed to trunk in r52153.

This ticket was mentioned in Slack in #meta by otto42. View the logs.


3 years ago

#44 @SergeyBiryukov
3 years ago

In 52325:

Build/Test Tools: Remove the replace:emoji-banner-text Grunt task.

The task was previously used to ensure that /*! This file is auto-generated */ comment is not included on front end as part of the inline emoji detection script.

As the wp-emoji-loader.js script is now included via file_get_contents() and wp_print_inline_script_tag() instead of grunt-include to simplify the logic, the task does not find anything to replace and is no longer necessary.

Additionally, include a line break before the wp-emoji-loader.js script content for better line wrapping.

Follow-up to [48096], [50651], [52132].

See #44632, #44306, #53363.

kafleg commented on PR #1745:


3 years ago
#45

Embed preview is still not working.

  • WP 5.9 Beta 3
  • Twenty Twenty-Two theme

In the block editor, WordPress Embed is used and link of the theme page is inserted. The preview on the block editor is perfectly fine but doesn't work in the front end. Only the title is displayed.

#47 follow-up: @westonruter
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It turns out that the approach taken here doesn't work with block-based themes like TT2, as discovered by @kafleg. Given this old action code:

<?php
add_action( 'wp_head', 'wp_oembed_add_host_js' );

The changes introduced in this ticket involve making wp_oembed_add_host_js() do simply:

<?php
add_filter( 'embed_oembed_html', 'wp_maybe_enqueue_oembed_host_js' );

The problem is that with block-based themes, do_blocks() runs before wp_head runs when get_the_block_template_html() is called. This means that the embed_oembed_html filter would have already applied run before wp_head is done, as seen in template-canvas.php:

<?php
/*
 * Get the template HTML.
 * This needs to run before <head> so that blocks can add scripts and styles in wp_head().
 */
$template_html = get_the_block_template_html();
?><!DOCTYPE html>
<html <?php language_attributes(); ?>>
<head>
        <meta charset="<?php bloginfo( 'charset' ); ?>" />
        <?php wp_head(); ?>
</head>

<body <?php body_class(); ?>>
<?php wp_body_open(); ?>

<?php echo $template_html; // phpcs:ignore WordPress.Security.EscapeOutput ?>

<?php wp_footer(); ?>
</body>
</html>

If we want to maintain back-compat for allowing authors to disable the oEmbed host JS via:

<?php
remove_action( 'wp_head', 'wp_oembed_add_host_js' )

Then I think what we need to do is something like this: https://github.com/WordPress/wordpress-develop/pull/2059

westonruter commented on PR #1745:


3 years ago
#48

@kafleg Good catch. I've re-opened the ticket and I've drafted a fix: https://github.com/WordPress/wordpress-develop/pull/2059

#49 in reply to: ↑ 47 @westonruter
3 years ago

Replying to westonruter:

If we want to maintain back-compat for allowing authors to disable the oEmbed host JS via:

<?php
remove_action( 'wp_head', 'wp_oembed_add_host_js' )

Then I think what we need to do is something like this: https://github.com/WordPress/wordpress-develop/pull/2059

There are 75 plugins doing so: https://wpdirectory.net/search/01FQ2SK19WA6MBS3JWZ7B59EBM

kafleg commented on PR #2059:


3 years ago
#50

Thank you for this PR @westonruter

Tested and worked perfectly.

  • WordPress - WordPress Develop from Git
  • Theme: TT2

#51 @westonruter
3 years ago

  • Keywords dev-feedback added

PR is ready for review.

swissspidy commented on PR #2059:


3 years ago
#52

Could the wp-embed JS just be a frontend script of the WP embed block instead?

westonruter commented on PR #2059:


3 years ago
#53

Could the wp-embed JS just be a frontend script of the WP embed block instead?

That could be, but shouldn't remove_action( 'wp_head', 'wp_oembed_add_host_js' ) still result in the script being removed as has been the case for a long time?

Also, making it a dependency of the WP embed block would not catch the case where post embeds appear in Classic block or the embed shortcode.

#54 @hellofromTonya
3 years ago

  • Keywords dev-reviewed commit added; dev-feedback removed

PR 2059 resolves the reported issue as confirmed by @kafleg. Marking for commit.

#55 @audrasjb
3 years ago

  • Owner changed from westonruter to audrasjb
  • Status changed from reopened to assigned

Self-assigning for commit.

#56 @audrasjb
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 52437:

Embeds: Fix oEmbed host script enqueueing on block-based themes.

This change fixes oEmbed host script enqueueing on front-end when using block themes.

It deprecates wp_oembed_add_host_js in favor of wp_maybe_enqueue_oembed_host_js. The action is still triggered in default-filters.php to ensure backward compatibility for websites that are removing the action. There is now a has_action() check in wp_maybe_enqueue_oembed_host_js() to see if wp_oembed_add_host_js() has not been unhooked from running at the wp_head action.

Follow-up to [52132], [52151], [52153], [52325].

Props swissspidy, westonruter, flixos90, kafleg.
Fixes #44632.

#58 @westonruter
14 months ago

In 55875:

Emoji: Remove extraneous sprintf() from _print_emoji_detection_script().

The sprintf() is not being passed any values, and there is no placeholder to replace.
This originated in r52132 in which I erroneously included this sprintf() in
two places. I fixed one of the cases in r52151 but I missed the other.

Follow-up to [52132], [52151].

Fixes #58436.
See #44632.

Note: See TracTickets for help on using tickets.