RedirectorPage: hooks or config for redirect (301 vs 302)

[Zauberfisch]

Once in a while I have a client that uses the RedirectorPage to have time based redirects to different locations. An easy example would be a Hotel with separates pages for Summer and Winter. Depending on the time of year, the client would configure the home page to redirect to either of those pages.

I believe we should make this possible either by user configuration directly in the CMS or alternatively by configuration or a hook. So I'd like to propose doing one of the following 3 options:

1. Make a CMS Field for the redirect type (301 or 302)
2. Create a onBeforeRedirect hook in RedirectorPageController that allows overwriting the redirect type
3. Make the Controller check if RedirectType exists as a field on the database and use if if it does. so one could add the DB field via extension. (Though I would prefer one of the other 2 options.)

Once we reach a decision, I'd be happy to implement this. And since it's a non breaking enhancement, I'd probably want to do it for 3.6 and 4.

[chillu]

Make a CMS Field for the redirect type (301 or 302)

I don't think giving CMS authors the choice between a permanent and temporary redirect is a good default - it's too much noise in the base installation for what I'd consider an edge case.

Create a onBeforeRedirect hook in RedirectorPageController that allows overwriting the redirect type

Yep, I'd be happy with that.

[Zauberfisch]

cool, I'll PR against 3.6 with a non breaking change. For 4, will it be merged upwards or should I make a 2nd PR?

[dhensby]

Just to add to what @chillu has said, allowing cms users to choose 301 is a security risk and should not be allowed. If a malicious user we to redirect users offsite a 301 would be very difficult to revert as browsers would cache it.

Also, if your redirect changes depending on the season it should be a 302 as the redirect is not permanent

[Zauberfisch]

you do realize that right now it's always 301?

Yes, I want 302, but right now it's hardcoded 301.

[dhensby]

Gah, what? I was sure it was 302 :/

[Zauberfisch]

nope: https://github.com/silverstripe/silverstripe-cms/blob/6fb1012eb803e75996733e43e650fc5caa4f1aba/code/Model/RedirectorPageController.php#L20

Speaking about it, I wouldn't be opposed to make it a 302. Because I often get the problem that clients change a redirector page and then think the website is broken because it still redirects the old way.

I guess we don't want to change that in 3.6, but perhaps I we could make 302 the default in 4?

[christopherdarling]

I've run into this issue in 3.x too, would it be possible for us to have the 3.x line to have this behind config with 301 as the default for BC?

I think 4 it would be nice to see it defaulting to 302 and have extension hooks as @Zauberfisch suggested in an earlier comment

[dhensby]

I don't see the http header code as an API. Changing the code is not an API breakage.

We have to decide on this, though.

[Zauberfisch]

uh, not considered an API change? lovely.

The more I think about it, there more I feel like it should be 302. From a users perspective it's very counter intuitive that something that can still be changed in the CMS will not end up working for many people as it's cached.

[tractorcow]

I don't see the http header code as an API. Changing the code is not an API breakage.

It might not be a semver violation, but we probably want to reduce undesirable changes creeping in to minor version updates.

I wouldn't mind a dropdown / radio to select redirect type. Make the default for new pages 302, but ensure that upgrades point to 301 unless manually selected. That would ensure a good default going forward without immediately changing existing redirects.

[lozcalver]

I agree with @chillu about not adding this to the CMS - most CMS users don’t know, or care, about the difference between 301 and 302. Even labelling them as “temporary” or “permanent” is going to give them something extra to think about that they don’t want. @Zauberfisch also makes a good point - changing it from 301 to 302 in the CMS might not have any effect until a user’s browser cache expires anyway.

I’d argue that the main benefit of using 301s in this context is SEO, and would point to using the redirectedurls module for that instead. So my vote is: change it to 302, and add the suggested extension hook.

[dhensby]

If CMS users can change the redirect after it's made it's not a 301 (from an SEO point of view). The CMS has a means of 301 redirecting old URLs to new.

I'm not in favour of an option in the CMS. I'm in favour of changing redirects to 302 and making this non-configurable.

[Zauberfisch]

so, sounds like we are agreeing on switching to 302 and adding a onBeforeRedirectHook so developers can change behavior if needed.

But, while changing to 302 is not really breaking any functionality, it does change the behavior of the website slightly. Therefore I would suggest adding the hook in both, 3.6 and 4, but the switch to 302 only in 4 and keeping 3.6 on 301.

[dhensby]

Therefore I would suggest adding the hook in both, 3.6 and 4, but the switch to 302 only in 4 and keeping 3.6 on 301.

We'll figure out the nuances of this separately. Do you want to write the PR for 3?

[Zauberfisch]

I'd be happy to do PRs for 3 and 4. But I'd like to have the nuances figured out first though

[dhensby]

I'm saying let's add the hook to 3 - we'll figure out the return code separately.

[chillu]

301 Permanent redirects are exactly what RedirectorPages are: A permanent redirect from one page to another page, or another URL. Arguing that they're temporary redirects because pages could be theoretically removed isn't reflecting the reality on how they're used. Zauberfisch has a specialised case on this ticket, which could be solved in a specialised module. Permanent redirects are recommended for SEO: https://moz.com/learn/seo/redirection I think in most cases, 301s reflect the authors intention: "Replace" one page with another in terms of search results and SEO.

Other options for redirection, like 302s and meta refreshes, are poor substitutes, as they generally will not pass the rankings and search engine value like a 301 redirect will. The only time these redirects are good alternatives is if a webmaster purposefully doesn't want to pass link juice from the old page to the new.

The caching of permanent redirects is desired. We could add a warning when changing the target of a RedirectorPage, or when trying to remove a RedirectorPage?

[Zauberfisch]

As long as it's extendable, I'm fine either way. But if we make 301 the default, I would also strongly tend towards adding a warning.

But I feel more and more in favour of actually making it the users choice. And if we do add a small warning, I think this could be acceptable. I'm thinking adding a checkbox "[x] Make this redirect permanent?" and then additionally display a info "Permanent Redirects are recommended for SEO, but will be cached ..."

[dhensby]

OK, let's not complicate the core too much, if you want to add that extension to your site, you can.

So decision is:

1. Add a hook to allow modification of response
2. Change default response to be no-cache so the 301 is not cached.

[chillu]

Change default response to be no-cache so the 301 is not cached.

I think not caching makes these pages more computationally expensive than they need to be. Keep in mind that it's quite common to use RedirectorPage exactly for high load situations like a short alias for a widely publicised campaign (e.g. in a TV ad). I'd suggest to limit caching to an hour by default. Configurable, so you can reduce that if you're really paranoid about your ability to change redirects on short notice.

[Zauberfisch]

ok, that sounds like a reasonable solution. I'll be happy to implement this. What branch will we merge this into?

[Zauberfisch]

Additionally, I'd like to raise the following question:

should Controller->redirect('/foo', 301) also have a Cache-Control header with an expiry date?

[chillu]

ok, that sounds like a reasonable solution. I'll be happy to implement this. What branch will we merge this into?

I'd be fine with 3.x for this - it's not an API change.

[dhensby]

should Controller->redirect('/foo', 301) also have a Cache-Control header with an expiry date?

I guess it makes sense to have a global config, yes.

Keep in mind that it's quite common to use RedirectorPage exactly for high load situations like a short alias for a widely publicised campaign

In that scenario you'd expect most visits to be unique anyway and this still leads to CMS users seeing 301s to the wrong place after editing a page URL...

[chillu]

In that scenario you'd expect most visits to be unique anyway and this still leads to CMS users seeing 301s to the wrong place after editing a page URL...

Any site expeciting higher loads will have a reverse proxy (Varnish etc), which seem to get cached by default (Acquia treats their Drupal hosting the same: https://docs.acquia.com/article/caching-redirects-varnish)