NEW Return 404 when redirector page wants to link to missing page

[ssmarco]

Issue #2665

I recently got a ticket where content authors have quite a number of RedirectorPages
and that they find it hard to track which ones to update whenever they unpublish the original page.
It makes more sense to them that whenever they unpublish a page, the RedirectorPage should display 404 as well.
This is quite opposite with the existing behaviour where the RedirectorPage maintains a published state.
Adding an extension point in this case provides additional flexibility.

• This extension is only applied whenever the link is null or not found
• One possible scenario is an internal link has been unpublished
• If lots of RedirectorPage then its hard to track and update
• An example use case is to change the content and status code to 404
• Another way is to redirect users to existing 404 page

[emteknetnz]

Another option for this is that we have a configurable private static along the lines of '404 on fail' that is false by default to retain the current behaviour. They would make it easier / more obvious for other developers to change to this behaviour since it's just a config flag rather than having to write an new extension class

I think this may be a better option because I cannot think of any other useful behaviour that would be implemented in the extension hook.

[emteknetnz]

I'm honestly finding the approach in this PR a bit weird

The existing logic on this function is:

1. attempt to redirect with a 301 status code
2. if that doesn't work because the target page has been archived, treat this like a normal page. Since page->Content is assumed to be blank, use the getContent() function with a 200 status code.

The default text for getContent() is A redirector page has been set up without anywhere to redirect to.

The reason for PR being raised as I understand it was because a cms author had changed a previous regular page into a redirector page, so it retained the old $page->Content (which they're no longer able to edit because that FormField is not shown in the CMS)

Repurposing an old page seems like a fairly reasonable thing for a CMS author to do with old content. It became a problem when the page being redirected to was archived and the link no longer worked

I really don't think that adding an extension hook and attempting to fix this with custom code on a case by case basis is pretty sub-optimal

A 404 is also potentially wrong here since that should be returned for the URL of the archived target page, though the redirector page should still return a 301/302. Unfortunately we no longer know what the target URL should be as the target page and corresponding URLSegment has been archived.

A redirect to the home page is probably not a bad default for Live pages where redirection is not working, though Draft previews should show the contents of getContent() so that CMS authors get feedback that they need to do something

@clarkepaul do you have any views on this?

[michalkleiner]

I really don't think that adding an extension hook and attempting to fix this with custom code on a case by case basis is pretty sub-optimal

do or don't think it's sub-optimal?

From my perspective, a redirector page shouldn't really hold any content, so if the destination page is not found, return standard 404 using the error page if present. If the destination does exist, redirect with 301/302. Nothing more, nothing less.

[clarkepaul]

I think a 404 page is better than sending them to the home page.

I don't think the redirector page should be triggered to be unpublished unless the CMS user has opted into it.

I think we need to empower so "CMS authors get feedback that they need to do something", for instance when unpublishing they get a warning that other pages are linked to it prior to unpublish/archiving. This is probably a separate but related issue, reports page related, or reason to look into CMS notifications.

[ssmarco]

I like your quote @clarkepaul about empowering CMS authors and thus makes sense to empower Devs as well if CMS authors needs help in this case as well as for backwards compatibility. This is the mean reason I think of extension hook which gives Devs flexibility whether the old behaviour is correct or wrong.

[ssmarco]

@emteknetnz @michalkleiner @clarkepaul Thanks for the feedback. Have updated my pull request taking points from all.

[dhensby]

I agree with the overall approach that if a redirector page points to a missing page, then it should return 404.

Also we should provide a clear/easy way for these dependent pages to be found and/or automatically unpublished

[dhensby]

My suggested approach to this would be:

1. Get a fix in now that leads RedirectorPages that don't point anywhere to 404.
2. Create a task / issue for getting a better CMS user experience for automatic unpublishing or flagging of dependant pages when a page is unpublished

[GuySartorelli]

I've retargetted this to 5 and rebased, updated to current best practices and made the requested changes.

One test is still failing but I ran out of time to look into it - I'll look at it in the new year but @ssmarco this is still your PR, so if you want to keep working on it and fix that failing test before I get to it that'd be amazing.

[emteknetnz]

Could you also add a quick unit test for the scenario for redirecting to a file

[GuySartorelli]

Added test scenario for files to the existing test via the dataprovider.

[emteknetnz]

Tested locally, works good