search

iamcal / oembed

The oEmbed Spec
http://oembed.com
MIT License
1.32k stars 651 forks source link

Harmonize the Chainflix domain pattern with others #608

Closed TomiMikola closed 2 years ago

TomiMikola commented 2 years ago

Seems like all the other providers needing to have multiple subdomains (m. and www. on top of the root domain) are using domain.com plus *.domain.com approach. Let's stick to the same pattern with Chainflix too.

This would also fix the Chainflix oEmbed issue appearing in Drupal after #607 was merged.

dpagini commented 2 years ago

This is also affecting my project as of last night. Our Drupal automated tests started failing after, I believe, #607 was merged in.

iamcal commented 2 years ago

merged and site updated, thanks. changes will be reflected when caches expire in ~1hr

raae commented 2 years ago

Is ?* best practice? Not seen that in any other provider.

tosbaha commented 2 years ago

This commit breaks this Remark Plugin. Please revert this commit or fix the regex. As @raae pointed out using *. and ?* causes an issue. AFAIK, this ?* should be just * or maybe *?. Someone who is more experienced with Regex may chime in.

TomiMikola commented 2 years ago

I have no strong opinion in this matter but looking at the other providers the pattern clearly is to use *.domain.com (Just take a look at by running curl -s https://oembed.com/providers.json | grep "\/\*\.")

tosbaha commented 2 years ago

I checked the JSON file and only Chainflix uses ?* The ones that use ? use it as a part of URL or as *?

TomiMikola commented 2 years ago

Ok I see your point. That part wasn't changed in this PR. You should probably create a new PR for it. I was only concerned the issue with the (m|www) in the initial version before this PR.