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

Add Libsyn special provider #144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Libsyn special provider #144

wants to merge 1 commit into from

Conversation

rtgoodwin
Copy link

@rtgoodwin rtgoodwin commented Feb 26, 2019

For reasons I can't quite discern, Libsyn' oEmbed URL isn't detected as a regular oEmbed provider. This patch adds Libsyn as a registered special provider.

Example Libsyn oEmbed URL: https://oembed.libsyn.com/embed?item_id=8634992

Per Libsyn support:

The url's absolutely do support oEmbed spec correctly - they wouldn't work with Embed.ly otherwise. You could always try just using the embed code for the player directly in your new site instead of the oEmbed. Most of the 'testers' out there seem to be looking for the metatags for discovery: https://oembed.com/#section4 and not actually parsing the response.

Tested with the oEmbed plugin, which uses Essence to render oEmbed in Craft CMS.

I'd love to understand what is needed to address this as a regular oEmbed provider, but I'm not entirely sure that the discovery method above is what is stopping Essence from parsing.

I did notice that the Libsyn oEmbed link type is text/json+oembed, whereas the spec denotes application/json+oembed. Essence seems to search specifically forjson in the type field though, from what I can see (in Oembed.php line 214's string filter), which seems like it should work?

My PHP is a little rusty, so... :) At any rate, this patch seems to work fine, and happy to help test or report to Libsyn why their file isn't working with the default Essence code.

@felixgirault
Copy link
Member

Hi!

I had a hard time understanding why you did encounter this problem, but I think I got it.
In fact there are two problems :

  • The generic Oembed and OpenGraph providers are not enabled by default (I thought I made it clear in the readme somewhere, but it seems I didn't 😅 )
    They can be enabled when instanciating essence though, using a match-all regex:
new Essence([
	'filters' => [
		'OEmbedProvider' => '//',
		'OpenGraphProvider' => '//'
	]
]);
  • The second problem lies in the URL libsyn provides to gather oEmbed data. In your example it is //oembed.libsyn.com?item_id=8634992, and the fact that there is no protocol confuses cURL (<url> malformed)

I'll try to find some time to make a little fix for this case 🙂

@rtgoodwin
Copy link
Author

@felixgirault ahh, ok!

Well for the first problem I definitely didn't see because I wasn't looking at the plugin code in question when instantiating Essence. :) I can't seem to figure how to tag the author here but will update them. ( https://github.com/wrav/oembed ) For now he has helped by pinning a version of the plugin to the PR here.

On the second problem; let me know. I've reached out to Libsyn and it was "passed on to development" but if there are specifics it would be great to be able to let them know.

Thank you!

@felixgirault
Copy link
Member

felixgirault commented Mar 4, 2019

About the first problem, I don't really know what to do for now. If the plugin allows configuring essence, it could be solved just by adding the code I posted earlier.
But I think the fact that there is no discovery by default in essence is a problem in itself. Maybe it would be a good thing to enable that in the default configuration.
@reganlawton if you want to weigh in on that, you're welcome! (thanks for using essence in your plugin btw 🙂)

@rtgoodwin Thanks for reaching out to Libsyn! The only thing I would advise them would be to put a fully qualified URL in the meta tags (including the protocol), so it could be used without any special treatment.
This is not clearly stated in the spec, but this sentence is close:

The URLs contained within the href attribute should be the full oEmbed endpoint

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

Successfully merging this pull request may close these issues.

None yet

2 participants