Oembed config - oembed_endpoints (#2752)

[srdjan-catalyst]

Moved twitter patterns from preview_url_resource to oembed_endpoints

Signed-off-by: Srdjan [email protected]

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)

[clokep]

See also #10392.

[srdjan-catalyst]

See also #10392.

Yes, it's a bit unfortunate. I made the change some time ago and did not have time to do the PR. Pick one that you like better I guess.

[richvdh]

I think this is meant to fix #2752.

[richvdh]

Thanks for the contribution!

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: note that CI is failing; we'll need to fix the failures before we can merge anything.

[richvdh]

@srdjan-catalyst: it doesn't matter too much in this case, since I didn't give it a thorough review, but please don't force-push changes in future, because it makes it hard to see what has changed since an earlier review.

[srdjan-catalyst]

@richvdh I'm about to commit again. Would you prefer a separate commit, or squashed force push this time please?

[richvdh]

a new commit please. there is never any need to squash.

[richvdh]

Thanks very much for your work on this! It's definitely progressing in the right direction, but also I have a few comments below.

[richvdh]

I think mocking out the HTTP client is fine, but out of interest, was there a reason you chose to replace this logic with mocks?

[srdjan-catalyst]

I could not make FakeTransport work with SSL at all with tests. I spent a day trying to make it work with no success. Other tests are passing because they are checking that certain requesta are attempted, and the fact that they are breaking at the SSL level does not matter for the outcome. In case of oembed we have two consecutive calls, and the second call never happens because the first one breaks.
Urls that come from oembed.com are (almost) all https, so I wanted to keep that.

[clokep]

Hello! I implemented something similar in #10714, I think that meets all the requirements that you were going for, but please shout if not. The changes from that should be in Synapse 1.43.0 (which won't be released for several weeks). Thanks for your contribution!