Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the X (formerly Twitter) oEmbed provider to use the new x.com domain instead of the deprecated twitter.com domain, addressing intermittent 520 Cloudflare errors. The changes include updating the API endpoint, adding comprehensive integration tests, and improving documentation for the oEmbed service.
Key changes:
- Updated X oEmbed provider API endpoint from
publish.twitter.comtopublish.x.com - Added integration tests for the X oEmbed provider with multiple test cases
- Enhanced documentation for
IOEmbedServiceandOEmbedServicewith XML comments - Fixed spelling and formatting in error logging
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Umbraco.Core/Media/EmbedProviders/X.cs | Updated API endpoint from twitter.com to x.com domain |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/OEmbedServiceTests.cs | Added new integration test file with 4 test cases for X provider, marked as [Explicit] to avoid CI failures from external dependencies |
| src/Umbraco.Core/Services/OEmbedService.cs | Added XML documentation for class and methods, fixed "oembed" → "oEmbed" spelling, improved code formatting |
| src/Umbraco.Core/Services/IOEmbedService.cs | Added comprehensive XML documentation for interface and GetMarkupAsync method |
AndyButland
changed the title
This was referenced Jan 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prerequisites
Addresses: #21052
Description
To investigate the linked issue I created an integration test and found it consistently failing with a 520 error from Cloudflare. I noticed we were still using a URL of
http://publish.twitter.com/oembedto get the HTML to embed, and changing it tohttp://publish.x.com/oembedresolved the problem.Not sure if this is the cause of the reported intermittent error but in any case, it seems likely the old domain could go away at some point, or at least be less well monitored.
I've updated the URL in this PR and added some integration tests. They pass, and previously failed, but have been marked as
Explicitjust to avoid a random, transient failure in an external dependency causing an issue when we are looking to build in our pipeline.Testing
Review integration test and manually try embedding a tweet from X in an RTE.
Release
As well as merging for 17.1 this should be cherry-picked back to
v16/devfor 16.5.