Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

Add parse_oembeds() method to Shortcode class #153

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goldenapples
Copy link
Contributor

Adds a new method to the base class Shortcode::parse_oembeds(), which
behaves basically the same way that parse_iframes() and parse_scripts()
do: it returns an array of all URLs found in the content string, with
the full line matched as the keys, and the matched URL as the values.
Individual shortcode classes can then match the URLs against their own
known patterns, and "reverse" these oembed-style links to post elements.

See #5

Adds a new method to the base class `Shortcode::parse_oembeds()`, which
behaves basically the same way that `parse_iframes()` and `parse_scripts()`
do: it returns an array of all URLs found in the content string, with
the full line matched as the keys, and the matched URL as the values.
Individual shortcode classes can then match the URLs against their own
known patterns, and "reverse" these oembed-style links to post elements/
@goldenapples
Copy link
Contributor Author

@danielbachhuber I'd like to hear your thoughts on this approach for reversing oEmbed-type functionality. It seemed like the simplest way I could think of to "reverse" oEmbeds across the board. Some classes may need to actually get the oEmbed response and use that in their reversal, while others, like Facebook, only need the URL. So I figured having a method available which just ran core's regex for matching oEmbed URLs over the content would be useful.

return false;
}

if ( preg_match_all( '#^(\s*)(https?://[^\s"]+)(\s*)$#im', $content, $matches, PREG_SET_ORDER ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be pretty slow to run so frequently, particularly because it's matched with more regex.

@danielbachhuber
Copy link
Contributor

I'd like to hear your thoughts on this approach for reversing oEmbed-type functionality.

What about, instead of trying to convert oEmbed URLs to shortcodes, we hooked into oEmbed rendering and injected the shortcode when the oEmbed request matched a URL we supported?

@goldenapples
Copy link
Contributor Author

What about, instead of trying to convert oEmbed URLs to shortcodes, we hooked into oEmbed rendering and injected the shortcode when the oEmbed request matched a URL we supported?

Could do that. I was thinking of an approach that would solve for that case as well as a case like #5, though, where we're trying to provide a seamless experience for users who are used to entering URLs for oEmbeds, whether or not a URL actually embeds with oEmbed. Facebook doesn't oembed, and I suspect we can match a lot of other URLs here that don't actually return an oEmbed response.

@danielbachhuber
Copy link
Contributor

I was thinking of an approach that would solve for that case as well as a case like #5,

Yeah, I think the original implementation I suggested will have to much of a performance hit.

where we're trying to provide a seamless experience for users who are used to entering URLs for oEmbeds, whether or not a URL actually embeds with oEmbed.

Right, but couldn't we do that from the other end? When WordPress attempts to reverse a URL, run that request against the URL regex we support with shortcodes?

@goldenapples
Copy link
Contributor Author

Ah, yeah, we could probably do that. On cursory glance it doesn't look like WP_Embed has filters in the right places to do that easily, but with a combination of filters on embed_handler_html, embed_maybe_make_link, and a few other places, we could probably tap into to core's oembed functionality and make it return a shortcode instead.

@davisshaver
Copy link
Member

This would still be a great feature for Shortcake Bakery.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants