OEmbed generates URLs with URL-decoded query string

I found Vimeo's documentation on this, so also adding that to the issue summary

Added oembed.com link

Looks like one additional case was added to the test, so couldn't quite do a clean revert

Seems like a logical fix. And all the documentation in the issue summary makes it easy to understand why this fix should happen.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Do I need to convert this to MR to stop the bot from marking it "needs work"? (if so I can easily do so)

It run only once on issues should be fine like this

ok setting back to RTBC as no code changed, just a mildly passive-aggressive bot

And the patch no longer applies so this can be converted to an MR.

Converted to MR

Restoring status from rebase.

I'm triaging RTBC issues → . I read the IS and the comments. I also skimmed the Vimeo docs and satisfied myself about the encoding. I didn't find any unanswered questions or other work to do.

So this is reverting the change made it 🐛 oembed link does not pass the URL parameter to the provider Fixed . That makes me think we need improved testing or manual testing of this change. @smustgrave, did you test this change, say, using the example parameters from the other issue of ?background=1&mute=0'

Leaving at RTBC.

@quietone If desired, we could test that multiple query parameters can be added via hook_oembed_resource_url_alter(). Currently, only altered=1 is tested, but we could change the test to set background=1 and mute=0 (spoiler it works fine)

The same happens with YouTube video playlists.
This URL gives only the playlist: https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=UfeuR...

"html": "<iframe width=\"200\" height=\"113\" src=\"https://www.youtube.com/embed/UfeuRWJPM4k?feature=oembed\"....

But when is URL encode https://youtube.com/oembed?url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv...

"<iframe width=\"200\" height=\"113\" src=\"https://www.youtube.com/embed/UfeuRWJPM4k?list=PLUQgibAMbpfPFirEnJw7o-bxxQ-oQI1zP\"...

It also respects the video list item.

RTBC, thank you!!

Upload MR patch in file format.

The patch has been tested and works as expected! RTBC

Hiding the patch file as it could confuse reviewers

I think this needs manual testing with Vimeo to ensure it doesn't regress the original issue.

@catch: I have manually tested that background=1&mute=0 can be added to Vimeo URLs via hook_oembed_resource_url_alter() - in fact, I tested this in the test hook, media_test_oembed_oembed_resource_url_alter(), although I didn't (yet) push a commit with that change (see comment #16).

So the premise in the original issue was incorrect. Anyone who wants to add background=1&mute=0 to their Vimeo embeds needs to implement hook_oembed_resource_url_alter(), parse $parsed_url['query']['url'] to get the user-provided query parameters, and if background or mute are present, add them to $parsed_url['query']. If this is an expected use case for Vimeo, then hypothetically this logic could be implemented in core itself, or else in a contrib or custom module, as I mentioned in the issue summary.

Believe #23 answers the concern.

Yeah I think the original bug report was not fantastic and this is definitely how the oembed spec documents that things must be.

Committed and pushed 5776f2879f to 11.x and a13dad16ae to 10.3.x and 383b2a775d to 10.2.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

Manual testing was done in #23