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

Replace oEmbed implementation with 3rd party library #5487

Closed
sminnee opened this issue May 9, 2016 · 14 comments
Closed

Replace oEmbed implementation with 3rd party library #5487

sminnee opened this issue May 9, 2016 · 14 comments

Comments

@sminnee
Copy link
Member

sminnee commented May 9, 2016

Follow-on from #5461.

There's no need for us to maintain our own oEmbed implementation. Let's move to a 3rd party package for this. https://github.com/oscarotero/Embed looks like a decent implementation.

https://github.com/essence/essence is another reasonably popular one, although it has more external dependencies, isn't commit to as frequently, and isn't as popular on packagist, so the oscarotero one gets my vote.

@Firesphere
Copy link
Contributor

Firesphere commented May 10, 2016

Hmmm, to bad I can not assign myself to this. I've started stripping out oEmbed in favor of oscarotero now, will possibly create a separate branch for essence if wished.

p.s.
I vote yes. From the looks of it, Oembed is... ehm... ehm... I will not continue putting down thoughts here, for the sake of humanity, okay? :P

@Firesphere
Copy link
Contributor

oscarotero embed requires Guzzle 5.
From the looks of it, essence is slightly less complex. Any preference?
@silverstripe/core-team pong

@kinglozzer
Copy link
Member

oscarotero embed requires Guzzle 5

It’s optional - you can configure how the requests are resolved (it’s only in require-dev in composer.json, not require).

+1 for https://github.com/oscarotero/Embed

@kinglozzer
Copy link
Member

Also, allow me to ping @silverstripe/core-team for you @Firesphere :D

@Firesphere
Copy link
Contributor

Pong. Thanks @kinglozzer :D

@dhensby
Copy link
Contributor

dhensby commented May 10, 2016

@kinglozzer is correct, Guzzle is just suggested, not required.

+1 for https://github.com/oscarotero/Embed

@sminnee
Copy link
Member Author

sminnee commented May 10, 2016

+1 for oscarotero/Embed without Guzzle. essence only looks less complicated because it's spread across 3 packages, I expect ;-)

@Firesphere
Copy link
Contributor

I admit I only had a quick look at it. Will go with oscarotero then. On functionality, I doubt it'll be very much different.

@Firesphere
Copy link
Contributor

Firesphere commented May 12, 2016

While we're at it...
What about the shortcode parser? I'd think using https://github.com/thunderer/Shortcode would be a nice way to get those kind of dragging-on things out of the way
edit
The package after the edit that is, not the docroot of a website.

@dhensby
Copy link
Contributor

dhensby commented May 12, 2016

Yes please, lets pull all this stuff out :)

Lots of our implementations of these features are quite MVP and have bugs that just never get round to being fixed.

@sminnee
Copy link
Member Author

sminnee commented May 12, 2016

I think that solving the shortcode parser as part of this PR isn't the best idea. Get oEmbed extracted, the PR raised (and merged), and then move on to shortcodes.

@dhensby
Copy link
Contributor

dhensby commented May 12, 2016

Yes, different pr

@Firesphere
Copy link
Contributor

Oh obviously different PR. Just added it here as the thought came up. Will open a new item for it when I get to it (or someone else does)

Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 15, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
… as Embed/Embed does sadly _not_ have a test-interface.

Moved the shortcodes to statics, it makes more sense to me to have it declared static.
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
… as Embed/Embed does sadly _not_ have a test-interface.

Moved the shortcodes to statics, it makes more sense to me to have it declared static.
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
… as Embed/Embed does sadly _not_ have a test-interface.

Moved the shortcodes to statics, it makes more sense to me to have it declared static.
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
… as Embed/Embed does sadly _not_ have a test-interface.

Moved the shortcodes to statics, it makes more sense to me to have it declared static.
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
Firesphere added a commit to Firesphere/silverstripe-framework that referenced this issue May 21, 2016
clarkepaul pushed a commit to open-sausages/silverstripe-framework that referenced this issue May 31, 2016
@dhensby
Copy link
Contributor

dhensby commented Jul 11, 2016

This is done

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

No branches or pull requests

4 participants