rfcs
rfcs copied to clipboard
wagtail
RFC 93: Redirect improvements
Add an RFC (as discussed internally) for adding a few useful features to redirects.
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 https://github.com/django/django/commit/fbac2a4dd846b52c4f379eacb5bab654fe9540cc), 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.
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.)
[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
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?
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?
