Make WordPress Core

Opened 12 years ago

Last modified 4 days ago

#23431 new defect (bug)

[embed] shortcode doesn't work with do_shortcode()

Reported by: jtsternberg's profile jtsternberg Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Embeds Keywords: has-patch has-test-info has-unit-tests dev-feedback
Focuses: Cc:

Description

It would be preferable to use the [embed] shortcode through do_shortcode rather than apply_filters( 'the_content',... in order to avoid sharing plugins, related posts plugins etc appending content to something that could simple be post meta, i.e. looking to convert a youtube url to a video.

Attachments (4)

23431.diff (594 bytes) - added by jtsternberg 12 years ago.
Simple check for the [embed] shortcode that allows filtering via do_shortcode()
23431.2.diff (1.2 KB) - added by kovshenin 11 years ago.
23431.3.diff (1.6 KB) - added by desrosj 3 years ago.
23431.4.diff (1.2 KB) - added by yashjawale 2 weeks ago.
Updated patch with extra CSS removed

Download all attachments as: .zip

Change History (22)

#1 @helen
12 years ago

FWIW, you can do it through WP_Embed::run_shortcode().

#2 @jtsternberg
12 years ago

No, that method doesn't work because it's not a proper static method and uses the $this variable which is unavailable. I CAN do

	$embed = new WP_Embed();
	$video = $embed->run_shortcode( '[embed width="890"]'. esc_url( $video_url ) .'[/embed]' );

But that definitely seems like overkill to me. (Please correct me if I'm wrong)

Last edited 12 years ago by jtsternberg (previous) (diff)

#3 @jtsternberg
12 years ago

Ok, digging a bit further, I realized the WP_Embed class is initialized as a global variable so is accessible to me via

$GLOBALS['wp_embed']->run_shortcode( '[embed width="890"]'. esc_url( $video_url ) .'[/embed]' );

So pardon my interruption here. :)

Curious, should we document that in the codex?

Last edited 12 years ago by jtsternberg (previous) (diff)

#4 @jtsternberg
12 years ago

Ok, another thought regarding the point of this ticket originally: it seems plausible that we could still enable this featured through do_shortcode() by checking for the [embed] shortcode, and then running this method. I'll throw up a patch if I get a chance.

#5 @helen
12 years ago

Well, yes, working code would be global $wp_embed; $wp_embed->run_shortcode( $whatever );. I'd check out the docs for the method - I think you'll find them helpful.

@jtsternberg
12 years ago

Simple check for the [embed] shortcode that allows filtering via do_shortcode()

#6 @SergeyBiryukov
12 years ago

  • Component changed from General to Embeds
  • Version changed from trunk to 3.5

#7 @jtsternberg
12 years ago

  • Keywords has-patch added; needs-patch removed

#8 @wonderboymusic
11 years ago

  • Milestone changed from Awaiting Review to 4.0

@kovshenin
11 years ago

#9 @kovshenin
11 years ago

I think we need to be careful here.

As you probably know, [embed] is a bit special, and it runs earlier than any other shortcode. The embed shortcode can actually return another shortcode as of 3.6, see wp_embed_handler_audio() in media.php which works fine with posts and the_content, but you'd need to run the result twice through do_shortcode() if you're going to do it manually.

There's also a caching mechanism for oEmbed which uses post meta to store the data. It uses the current post global, which means that if you're running the embed shortcode (or the_content filter for that matter) manually without an explicit post context (outside of the loop), you can get pretty weird stuff, like non-relevant oembed cache data stored with the last post of the main query, meaning a "cache flush" every time you publish a new post.

That said using the run_shortcode() method is overkill in this scenario, because you already know the shortcode tag and the URL you'd like to embed, so there's no reason to run the regex in do_shortcode() one more time. You can invoke the shortcode() method directly, which will happily accept shortcode attributes in standard form, and doing that is essentially the same as registering an actual (vs fake) callback for the embed shortcode, see 23431.2.diff.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


11 years ago

#11 @helen
11 years ago

  • Milestone changed from 4.0 to Future Release

#12 @chriscct7
10 years ago

  • Keywords needs-testing needs-unit-tests added

@desrosj
3 years ago

#13 @desrosj
3 years ago

  • Milestone set to Future Release

23431.3.diff is a refresh that applies cleanly.

@swissspidy @azaozz As component maintainers, do you feel this is a change still worth making?

#14 @SirLouen
2 weeks ago

  • Keywords has-test-info has-unit-tests added; needs-testing needs-unit-tests removed
  • Type changed from feature request to defect (bug)

Test Report

Description

✅ This report validates that the indicated patch works as expected.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/23431/23431.3.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-One 2.5
  • MU Plugins: None activated
  • Plugins:
    • Simple Embed Viewer 1.0.0
    • Test Reports 1.2.0

Testing Instructions

  1. Add the code provided in the Artifacts somewhere where it could be executed
  2. Alternatively, you could simply do a do_shortcode with an [embed][/embed] shortcode tags and a youtube URL for example (or any other supported embed URL)
  3. If you are using the code you will find an admin menu item to check for this
  4. 🐞 Embed is not showing.

Expected Results

  • Embed shows with do_shortcode

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • I know that shortcodes are planned to be deprecated at some point for blocks, but still, they are very used and blocks are not the sole standard in this regard and even include a shortcodes block. This patch is supersimple and works fine, so I think it's a good idea to move it forward.
  • Little mistake: patch adds a style into 2019 theme. Don't forget to remove it.

Supplemental Artifacts

Test Code:

add_action( 'admin_menu', 'add_embed_admin_menu' );

function add_embed_admin_menu() {
	add_menu_page(
		'Simple Embed Viewer',
		'Embed Viewer',
		'manage_options',
		'simple-embed-viewer',
		'render_embed_admin_page',
		'dashicons-format-video'
	);
}

function render_embed_admin_page() {
	echo '<h1>Embedded Video</h1>';
	echo do_shortcode( '[embed]https://www.youtube.com/watch?v=adHJw7qDiBQ[/embed]' );
} 
Last edited 2 weeks ago by SirLouen (previous) (diff)

#15 @yashjawale
2 weeks ago

Test Report

Description

This report validates that the indicated patch works as expected.
Testing code used from comment:14

Patch tested: https://core.trac.wordpress.org/attachment/ticket/23431/23431.3.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 138.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Three 1.6
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch, embedding works with shortcodes rendered using do_shortcode.

Additional Notes

  • ⚠️ The patch includes some testing CSS inside twentynineteen/style.css which needs to be removed
  • Attaching the updated patch file with the CSS removed...

Supplemental Artifacts

Screencast: https://files.catbox.moe/jgloxk.mov

@yashjawale
2 weeks ago

Updated patch with extra CSS removed

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


4 days ago

#17 @SirLouen
4 days ago

  • Keywords dev-feedback added

6.9.0 Milestone proposed during today's <bug-scrub>
More information

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


4 days ago

Note: See TracTickets for help on using tickets.