Disabling url.py module also disables @url decorator

[dgw]

The point of adding @module.url() to the module API was to simplify the song-and-dance of adding a module's URL patterns to the global list of "callbacks" so that url.py wouldn't process links already handled by a more specific plugin.

However, if url.py is not loaded, any callable using @url will simply never run, because these functions are dispatched in url.py instead of by Sopel's core:

sopel/sopel/modules/url.py

Lines 239 to 245 in 0543f2f

	# Then, check if there's anything in the callback list
	for regex, function in tools.iteritems(bot.memory['url_callbacks']):
	match = regex.search(url)
	if match:
	# Always run ones from @url; they don't run on their own.
	if run or hasattr(function, 'url_regex'):
	function(bot, trigger, match)

I consider this to be a pretty major bug in the API. The point of having url_callbacks is for url.py to avoid double-processing links that another module already handles. Module API functionality must not depend on loading any module besides coretasks (which cannot be disabled, for obvious reasons).

(On a personal note, this also explains why I couldn't get @url-decorated callables to trigger on my own Sopel instance when I tried a few years ago. I've had url.py disabled approximately forever.)

---

I think the solution here is to move url.py's URL-detection and callback-dispatch logic into coretasks, and the exclude & exclusion_char settings from the [url] config section into [core]. This @rule:

sopel/sopel/modules/url.py

Lines 157 to 158 in 0543f2f

	@rule(r'(?u).*(https?://\S+).*')
	def title_auto(bot, trigger):

would become just another @url decorator.

That said, making all modules equal by moving dispatch of @url into coretasks presents a dilemma: How to determine which callback/pattern is "most appropriate", and avoid having the same URL processed by both url.py and (an)other module(s)? I'm almost positive that this conundrum is the reason why the current implementation was implemented as-is, but I'm also sure it's a solvable problem.

Given the lack of immediate ideas (at least on my end), and the fact that this is a fairly major overhaul, I'm assigning this issue to Sopel 7. That's the ideal, but if implementing a solution requires an API change, this will be pushed to Sopel 8 so as to provide advance warning via release notes. (It will also, of course, be pushed off if it simply isn't ready in a reasonable amount of time.)

[Exirel]

Thanks for the thorough description of the issue! I saw your message on my PR, and I'm thinking about it since then. I'll see what I can come up with, and I'm sure we'll find a solution.

[Exirel]

While we are at it, the url plugin has some weird behavior:

10:43:35 <@Exirel> .title https://www.instagram.com/p/BqJ2jHXBB3q/
10:43:35 <Icicle> Exirel: Sorry, fetching that title failed. Make sure the site is working.
10:43:37 <Icicle> [insta] Photo by Marc Dubuisson (@unpied) | "SpaceZ" | 640x640 | Likes: 1,074 | Comments: 8 | Uploaded: 2018-11-14 08:24:37

Here is what's going on:

1. the .title command is executed by the url plugin,
2. the .title command see that there is an URL callback for that URL, so it found no result
3. the .title command does not run the callback, because it does not have a url_regex attribute - because instagram's callback uses rule and not url,
4. the .title command think there is no result, and say "sorry"
5. the instagram's rule catches the line and is triggered

It's a bit tricky, but it means that URL callback must not be called unless they have a url_regex attribute, but they must still prevent the "default URL callback" to be triggered if they match.

That's some undocumented stuff!

[Exirel]

I think the solution here is to move url.py's URL-detection and callback-dispatch logic into coretasks, and the exclude & exclusion_char settings from the [url] config section into [core].

It looks like it is more complicated than that. The exclude parameter is ignored by url plugin when it comes to trigger URL callbacks (see the check_callbacks function).

However, the exclusion_char is used everywhere: in the process_url function and in the setup hook function.

That being said, I would keep these parameters in the url section, for the url plugin, and ignore them for other URL callbacks - plugins can do whatever they want. Otherwise, plugins will register callback to prevent the default behavior, but they won't use the url decorator, because they won't have a way to manage their URLs themselves.

See the instagram module: it does its own logic. It registers a rule callable, and not a url one, so the exclusion_char is ignored.

At the end of the day, the exclusion_char is more like a disadvantage for external plugins than a helper.

[dgw]

See the instagram module: it does its own logic. It registers a rule callable, and not a url one, so the exclusion_char is ignored.

To be fair, instagram probably only uses rule because @kwaaak doesn't trust the url decorator, or something. 😆 It should use url (as you originally changed it to do in #1483, before I found this bug) so that multiple Instagram links in a line will work properly. I suspect any module that uses rule to detect URLs has a similar issue to sopel-irc/sopel-github#13 hiding in it, honestly.

[Exirel]

I did a bit of work in #1508 just to start somewhere. Now I just need to know if we should merge as-is, and work on a new PR for the next step, as #1508 is already an improvement and can be documented as-is for plugin maintainers; or if I should continue in this PR in order to fix this issue in one go.

[dgw]

I just said in #1508, but I'll repeat here: Let's make the new interface one step, and actually try to replace the url module separately. That way, bikeshedding the replacement won't hold up the straightforward addition of those convenience methods.

[dgw]

Should be fixed by #1510, so removing "Patches Welcome" label.