Page MenuHomePhabricator

Add support to UrlShortener to support ShortUrl redirects
Open, MediumPublic

Description

When deploying UrlShortener, we will undeploy ShortUrl, but we still need those urls to work. So add support to UrlShortener to read from the shorturl database and handle those.


NOTE: This is a proposed outcome for T187045: Code Stewardship Review: ShortUrl Extension.

Event Timeline

Legoktm raised the priority of this task from to Needs Triage.
Legoktm updated the task description. (Show Details)
Legoktm added a subscriber: Legoktm.

Probably. I guess we could leave ShortUrl in place but that's cruft...

I guess we could redirect the s/<something> urls to a w.wiki url and then redirect them again? :P or should we just merge in the code? what happens to the shorturll db?

I was thinking of just copying code from ShortUrl. The main thing is that we no longer expose (old) ShortUrls anywhere, and we don't create any *new* ones. The db is split per-wiki, so it has to stay where it is.

s/* --> Special:LegacyShortUrls --> destination.

Or we could keep ShortUrl as is, put it in a readonly mode.

Is there a plan for this? We now have UrlShorterner enabled everywhere but no plan for getting rid of ShortUrl.

Krinkle changed the task status from Open to Stalled.Jul 2 2020, 4:19 PM
Krinkle triaged this task as Medium priority.

The main thing is that we no longer expose (old) ShortUrls anywhere, and we don't create any *new* ones.

we could keep ShortUrl as is, put it in a readonly mode.

So it looks like the outcome willl be that the current UI deployed on some wikis where short URLs are eagerly generated on page view (violates DBPerf, T122708) and shows them atop the page for sharing, is something that will no longer be supported.

Instead, people would need to use https://w.wiki, possibly via a gadget that produces a link like https://w.wiki/?url=URL so that users can still get there easily and then only have to submit the form to find or make the short url accordingly.

Perhaps we can already remove the current UI and eager generation today, ahead of T187045 and this task (T107188). If not, what is blocking that?

This comment was removed by Krinkle.
Ammarpad changed the task status from Stalled to Open.EditedJul 12 2020, 1:57 PM
Ammarpad added a subscriber: Ammarpad.

I think keeping the extension in 'readonly' mode, while easier, it has cost and we will be back here to undeploy again. (cf. CodeReview T116948).

I will be happy to move this forward to make UrlShortener understands these legacy urls but need some clarification. Since ShortUrl has database table per each wiki, so where will they eventually live so as to undeploy the extension? Once UrlShortener can have access to the static table it will be able to read and process them.

The plan would be to add a configuration flag that will only be enabled on these projects where the ShortUrls is currently deployed.

Instead, people would need to use https://w.wiki, possibly via a gadget that produces a link like https://w.wiki/?url=URL so that users can still get there easily and then only have to submit the form to find or make the short url accordingly.

UrlShortener already provides a sidebar link that does the same thing. On click, it sends api POST request to create the link and display it. It is disabled on production

I have been thinking about this. I think merging two extension is not a good idea, it makes the code less coherent and turn it into some sort of spagethhi . My counter proposal is:

  • Introduce a readonly mode for ShortUrl extension
  • Enable it on production at the same time of enabling sidebar link (with UrlShortner) on those wikis. i.e. Turn ShortUrl readonly, Turn off read only mode of UrlShortener in those wikis (the wikis should be small, so not much work)
  • Remove the write parts of ShortUrl extension
  • Keep and maintain the shell left of ShortUrl in production

What do you think?

I am fine with that. My comment was largely based on what the task proposed: Add support to UrlShortener to support ShortUrl redirects, may be we should rename it now.

Indeed making UrlShortener to support ShortUrl codes would make the code some kind of garbled, because you have got to explain that this extension is supporting another different routine of abandoned extension. But maintaining ShortUrl extension, even skinned 'shell' of it is not free either and, as I said we may probably be back here talking of 'undeploy' again when it starts causing more harm than good, due to deprecations and other inevitable refactoring of the parts it depend on.

So that makes it worthwhile to consider both options and their short term and long-term repercussions. In the short term, making ShortUrl read-only and leaving UrlShortener out of this is both easy and arguably clean solution. but I am not sure if that's also the case from the long-term perspective.

But do we care about that? I am not sure who will finally decide here (to merge, go for readonly, do something else, etc), but if I may give my opinion, both options are OK. Although I will favor dropping ShortUrl completely and redirect the special page: T240188 (But this may not necessarily means merging it with UrlShortener, there might be other options out there).

Turn off read only mode of UrlShortener in those wikis (the wikis should be small, so not much work)
[Enable] sidebar link (with UrlShortner) on those wikis.

So can you help me understand why this was disabled for other wikis? For instance on English Wiki a user will see this. Is it just we want force them to create it on meta-wiki? I think this is relevant now we want turn it on on subset of projects and what if another project request it to be turned on for them too? possibly after if they see it enabled on project near them. Or the sidebar link that will become visibily present on subset of the projects and not the others.

I am fine with that. My comment was largely based on what the task proposed: Add support to UrlShortener to support ShortUrl redirects, may be we should rename it now.

Indeed making UrlShortener to support ShortUrl codes would make the code some kind of garbled, because you have got to explain that this extension is supporting another different routine of abandoned extension. But maintaining ShortUrl extension, even skinned 'shell' of it is not free either and, as I said we may probably be back here talking of 'undeploy' again when it starts causing more harm than good, due to deprecations and other inevitable refactoring of the parts it depend on.

True but if we remove most of the code and leave only the shell of it, I assume these kind of issues stop and at least reduced to minimum.

But do we care about that? I am not sure who will finally decide here (to merge, go for readonly, do something else, etc), but if I may give my opinion, both options are OK. Although I will favor dropping ShortUrl completely and redirect the special page: T240188 (But this may not necessarily means merging it with UrlShortener, there might be other options out there).

Turn off read only mode of UrlShortener in those wikis (the wikis should be small, so not much work)
[Enable] sidebar link (with UrlShortner) on those wikis.

So can you help me understand why this was disabled for other wikis? For instance on English Wiki a user will see this. Is it just we want force them to create it on meta-wiki? I think this is relevant now we want turn it on on subset of projects and what if another project request it to be turned on for them too? possibly after if they see it enabled on project near them. Or the sidebar link that will become visibily present on subset of the projects and not the others.

There is two parts: Making it non-readonly and enabling the link on sidebar.

  • Making it non-readonly is mostly blocked on checking if the ratelimit works (it might be you can bypass the ratelimit by requesting it in another wiki, we want to make sure it doesn't happen). Once we are sure the ratelimit works, it can go live everywhere.
  • Enabling the link on the sidebar on the other hand is a complicated mess, it needs a UX review first (probably a rewrite) then a performance review, then slowly rolling it out (to make sure pressure on the database stays within healthy boundaries) but OTOH, this can go live on the wikis that have shorturl enabled (as an alternative to shorturl) because a) they are pretty small and b) this thing at any shape is better than shorturl and c) if the ratelimit doesn't work, the number of wikis we roll this out to is still small.

@Ammarpad mostly summed up my points about maintenance overhead with having a separate extension. I think subsuming ShortUrl into UrlShortener makes sense long term as it:

  • removes the overhead for maintaining a separate git repo think of all the CI/npm/composer/phan/PHPCS/etc. config
  • clearly indicates that UrlShortener is the future of ShortUrl
  • unifies all short URL logic, making it straightforward to look in just one place when you're trying to find or debug something (there's already enough confusion just based on the names alone)

I don't think it'll be as messy as feared, the ShortUrl stuff will be relatively self-contained and marked as legacy, guarded by feature flags.

We could do it in stages too, first turn ShortUrl into the shell, deploy all that, and then merge it into UrlShortener as cleanup.

Sure, if that's preferred for you too. I'm fine as well, let's first make the shell of it to make the merge easier. Does that sound good?