Added oEmbed support to the Web plugin to improve title fetching #1613
Conversation
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
progval
reviewed
Feb 3, 2025
Comment on lines
+98
to
+104
| conf.registerGlobalValue(Web, 'useOembedRegistry', | ||
| registry.Boolean(False, _("""Determines whether the bot will use the | ||
| oembed.com providers registry."""))) | ||
|
|
||
| conf.registerGlobalValue(Web, 'useOembedDiscovery', | ||
| registry.Boolean(False, _("""Determines whether the bot will use HTML | ||
| discovery to find oEmbed endpoints."""))) |
There was a problem hiding this comment.
Name the config variables plugins.Web.oembed.registry and plugins.Web.oembed.discovery.
Could you also make them channel-specific, but not op-settable?
| response = utils.web.getUrl(url, timeout=timeout) | ||
| text = response.decode('utf8', errors='replace') | ||
| match = re.search( | ||
| r'<link[^>]+?type="application/json\+oembed"[^>]+?href="([^"]+)"', |
There was a problem hiding this comment.
That's insufficient, it does not cover different attribute orders or single quotes.
| re.IGNORECASE) | ||
| if match: | ||
| endpoint = match.group(1) | ||
| endpoint = endpoint.split('?')[0] |
| oembed_endpoint = self._getOEmbedEndpoint(url) | ||
| if not oembed_endpoint: | ||
| return None | ||
| oembed_url = f"{oembed_endpoint}?format=json&url={url}" |
There was a problem hiding this comment.
don't you need to escape the URL? (use urllib.parse.urlunparse)
Comment on lines
+182
to
+205
| def testtitleOembedRegistry(self): | ||
| try: | ||
| conf.supybot.plugins.Web.useOembedRegistry.setValue(True) | ||
| self.assertResponse( | ||
| 'title https://www.flickr.com/photos/bees/2362225867/', | ||
| 'Bacon Lollys') | ||
| finally: | ||
| conf.supybot.plugins.Web.useOembedRegistry.setValue(False) | ||
|
|
||
| def testtitleOembedDiscovery(self): | ||
| try: | ||
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(True) | ||
| self.assertResponse( | ||
| 'title https://flickr.com/photos/bees/2362225867/', | ||
| 'Bacon Lollys') | ||
| finally: | ||
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(False) | ||
|
|
||
| def testtitleOembedError(self): | ||
| try: | ||
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(True) | ||
| self.assertError('title https://nonexistent.example.com/post/123') | ||
| finally: | ||
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(False) |
There was a problem hiding this comment.
Suggested change
| def testtitleOembedRegistry(self): | |
| try: | |
| conf.supybot.plugins.Web.useOembedRegistry.setValue(True) | |
| self.assertResponse( | |
| 'title https://www.flickr.com/photos/bees/2362225867/', | |
| 'Bacon Lollys') | |
| finally: | |
| conf.supybot.plugins.Web.useOembedRegistry.setValue(False) | |
| def testtitleOembedDiscovery(self): | |
| try: | |
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(True) | |
| self.assertResponse( | |
| 'title https://flickr.com/photos/bees/2362225867/', | |
| 'Bacon Lollys') | |
| finally: | |
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(False) | |
| def testtitleOembedError(self): | |
| try: | |
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(True) | |
| self.assertError('title https://nonexistent.example.com/post/123') | |
| finally: | |
| conf.supybot.plugins.Web.useOembedDiscovery.setValue(False) | |
| def testtitleOembedRegistry(self): | |
| with conf.supybot.plugins.Web.useOembedRegistry.context(True): | |
| self.assertResponse( | |
| 'title https://www.flickr.com/photos/bees/2362225867/', | |
| 'Bacon Lollys') | |
| def testtitleOembedDiscovery(self): | |
| with conf.supybot.plugins.Web.useOembedDiscovery.context(True): | |
| self.assertResponse( | |
| 'title https://flickr.com/photos/bees/2362225867/', | |
| 'Bacon Lollys') | |
| def testtitleOembedError(self): | |
| with conf.supybot.plugins.Web.useOembedDiscovery.context(True): | |
| self.assertError('title https://nonexistent.example.com/post/123') |
use the new style, it's shorter
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.
Two methods are supported and can be configured:
Registry is tried first if enabled, then discovery if enabled,
finally falling back to HTML parsing.
I think it would be nicer to add the calls to
getTitle().I was trying to avoid using
url_workaround(), since it would break support for reddit - for exsample, old.reddit.com isn't in the registry. However, following redirects could actually be beneficial, especially for handling v.reddit.com.For discovery, it makes sense to place this logic close to the title parser, as that would save an extra request. That said, I wanted to open the pull request first to gather some feedback before making further adjustments.