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

Make oEmbed globs configurable and extract more info from the response #10392

Closed
wants to merge 1 commit into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jul 14, 2021

Fixes #9733

Needs moar tests, some refactoring to tidy up the code, to load the providers.json from upstream automagically

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@richvdh
Copy link
Member

richvdh commented Jul 28, 2021

A thought occurs to me (sorry for moving the goalposts): rather than embedding a list of oEmbed provider URLs, could we instead pull the <link rel="alternate" type="application/json+oembed"> value from the response?

Youtube's watch links include content such as the following:

<link rel="alternate" type="application/json+oembed" href="https://www.youtube.com/oembed?format=json&amp;url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3DRzJf02TIqxk" title="PURE RELAXATION - SERVER SOUNDS by Hetzner">

https://oembed.com/#section7 says:

There are currently 290 providers in the registry. Providers and consumers are strongly encouraged to use the discovery mechanism, rather than the registry.

... of course that doesn't work for Twitter, so maybe we need to support a static list as well. Gah.

@ajs124
Copy link

ajs124 commented Jul 28, 2021

Instead of maintaining our own static list, why can't we just pull in the registry from https://oembed.com/providers.json?

@clokep
Copy link
Contributor

clokep commented Jul 28, 2021

Instead of maintaining our own static list, why can't we just pull in the registry from oembed.com/providers.json?

I had recommended this to @t3chguy as a follow-up to keep this PR smaller. It is a bit unclear though if we need to support custom / configurable values. I can come up with use-cases for it, but I'm unsure if they're realistic or not.

@richvdh richvdh linked an issue Aug 6, 2021 that may be closed by this pull request
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I've written some words about how I think we should approach this over at #2752 (comment). TL;DR: let's use a providers.json file rather than config.

Also: please could you separate the bits of this PR that aren't to do with updating the glob list out to a separate PR?

@richvdh richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 6, 2021
@clokep
Copy link
Contributor

clokep commented Aug 31, 2021

Hello! The main gist of this PR was implemented by #10714. I think the "extra more info" bits could be moved to a separate PR, but will likely be difficult to rebase on the changes.

@clokep
Copy link
Contributor

clokep commented Sep 13, 2021

I'm going to close this in-lieu of #10814, which I think makes the same improvements.

@clokep clokep closed this Sep 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Youtube captions (link previews) are useless Use oembed for url previews?
4 participants