RFC 93: Redirect improvements

[RealOrangeOne]

Add an RFC (as discussed internally) for adding a few useful features to redirects.

View rendered version

[zerolab]

I think this is well worth flagging. i.e. "There is an existing system path that may stop working. Are you sure?"

[ababic]

Would this still be relevant if redirects continue to only come into play in the event of a 404?

[zerolab]

This is one that doesn't sit well with me at the moment. I can see pitchforks being raised :)

What if we provide a separate, EagerRedirectMiddleware for just this? Plenty of Wagtail sites are perfectly fine with the current setup

[zerolab]

This will need to account for i18n-prefixed URLs

[RealOrangeOne]

Localisation may make this a lot more complex (but is also something we need to account for). For example, if we want to redirect to a page with multiple translations, what URL do we redirect to?

[ababic]

Would this get in the way of people manually setting up redirects in advance of taking certain actions (E.g. Moving a group of pages) when automatic redirect creation is disabled?

[RealOrangeOne]

It would, yes, and so would require automatic redirections to properly kick in. We could make this validation opt-out.

[gasman]

For background, the use-case that originally prompted the desire to validate against circular redirects was a site that added various /foo -> /foo/ redirects (redundantly, since CommonMiddleware already handled this).

With eager redirects out of the picture (no longer required as of Django 4.2 due to django/django@fbac2a4), it seems to me that the usefulness of this becomes more marginal, since this redirect loop would only happen if the destination was a 404 (in which case the redirect would not be doing anything useful even if it were working correctly).

Having said that, there's no real downside to having this validation in place, so happy to go ahead with adding it.

[gasman]

Should we also confirm whether the old_path matches an existing view?

I take it the idea here is to provide some sort of feedback to the user to indicate "this redirect won't trigger because its URL corresponds to an existing page"? I can see the value in that, but I think it's different enough from the focus of this RFC, and hard enough to get right, that we shouldn't try to tackle it here. (Presumably in many / most cases, old_path will indeed pattern-match against at least one view at the urlconf level - namely, wagtail_serve - but we then need to follow Wagtail's routing logic to find out if it's a 404 or not. So either that's a special case for wagtail_serve, or we generalise it to support other views that might return a 404 somewhere in their execution... I think that rabbit-hole could get pretty deep.)

[gasman]

[Converting URL redirects to page redirects]

Should this be automatic, or a warning during creation or a report? Adding links in RichText automatically detects internal links, but prompts the user. This might be more difficult in a conventional creation flow.

My gut feeling is that converting them automatically is possibly more "magic" than we really want - it's overriding what the user specifically asked to do, with what the system thinks they meant to do, and I could imagine situations where it gets it wrong. Something along the lines of:

• A bank wants to relaunch their mortgage calculator app
• Their content editor creates a page at /new-mortgage-calculator/ talking about the new mortgage calculator they'll be launching soon, along with a URL redirect from /mortgage-calculator/ to /new-mortgage-calculator/
• A short while later they deploy the new app at /new-mortgage-calculator/, and move the page to somewhere under /archive/
• The redirect from /mortgage-calculator/ is now unexpectedly pointing to the archived page

[gasman]

Picking this up again for 6.2 planning...

I think we're happy to go ahead with validating against circular redirects, as per #11659.

Re converting URL redirects to page redirects - as per above, I don't think this should happen automatically. A UI for prompting users about this might look something like:

On submitting the "add redirect" form, if the external URL field has been filled in, parse it and run it through Wagtail's routing logic. If this resolves to a page, AND that page's get_url method returns a URL equal to the one entered (this latter check needed to account for things like RoutablePageMixin where converting to a page redirect would lose information), present a confirmation page saying:

The URL {url} corresponds to the page {page title}. Do you wish to create this as a page redirect? This will ensure that the redirect continues to work cleanly if the page is moved or renamed in future.
Yes, change this to a page redirect
No, keep this as a URL redirect

These buttons will be linked to hidden forms; the "yes" button will submit a replica of the original form with the page ID populated and the URL field cleared, while the "no" button will submit a replica of the original form with an additional flag to skip the routing check.

This workflow will only work for adding a single redirect, not bulk uploading - are we happy to treat the latter as out of scope, or do we need to account for that too?

[ababic]

This workflow will only work for adding a single redirect, not bulk uploading - are we happy to treat the latter as out of scope, or do we need to account for that too?

I would say that's a fair compromise. But, I am slightly concerned about giving developers / admins / editors a false sense of security here... It's very easy to assume that because Wagtail treats individual redirects one way, it also does something about batches (whether visible or not), and therefore assume it's something they don't need to consider. Is there anything we could do to alleviate this?