Skip to content
This repository has been archived by the owner on Mar 16, 2023. It is now read-only.

FIX: Ensure that canonical URLs don’t redirect elsewhere. #152

Merged
merged 2 commits into from
Aug 15, 2019
Merged

FIX: Ensure that canonical URLs don’t redirect elsewhere. #152

merged 2 commits into from
Aug 15, 2019

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented Apr 4, 2019

Redirecting canonical URLs violate HTTP rules and break some search
spiders including Swiftype.

As part of this change, DocumentationPage loses responsibility for
calculating canonical URLs. I’ve opted to leave the methods in there
but they will throw warnings.

Fixes silverstripe/doc.silverstripe.org#181


To do:

  • Add PHPDoc deprecation notice as well as the error

@sminnee
Copy link
Member Author

sminnee commented Apr 4, 2019

FYI this is in support of swiftype search on docs.

Should this be a 2.1 release or a 3.0?

@sminnee sminnee requested a review from robbieaverill April 4, 2019 00:48
@robbieaverill
Copy link
Contributor

We use this module in cwp.govt.nz, which doesn't use SwiftType. I personally think that tailoring this module for a specific third party search provider is a mistake, but if the changes are semantically correct across the board then it's OK I suppose. I'll schedule some time to look at how this will affect the CWP website.

@sminnee
Copy link
Member Author

sminnee commented Apr 4, 2019

It's not tailoring it for a specific third-party search provider — it's fixing things about it that violate HTTP that Google is a little more forgiving of.

@robbieaverill
Copy link
Contributor

Cool, will Google continue to honour it after this change?

@sminnee
Copy link
Member Author

sminnee commented Apr 4, 2019

image

The biggest difference is that the short URLs shown here will likely have a version number put in them, eg
https://docs.silverstripe.org/en/4/developer_guides/templates/syntax/

When a new major version comes out they'll migrate to those via permanent redirectly and newly assigned canonical URLs.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, just a question about the deprecation

@sminnee
Copy link
Member Author

sminnee commented Aug 14, 2019

Thanks – is this ready for merge now from your perspective?

@sminnee
Copy link
Member Author

sminnee commented Aug 14, 2019

I'm happy for you to merge on green, your change looks fine.

@robbieaverill robbieaverill changed the base branch from master to 2 August 14, 2019 23:14
Sam Minnee and others added 2 commits August 15, 2019 11:14
Redirecting canonical URLs violate HTTP rules and break some search 
spiders including Swiftype.

As part of this change, DocumentationPage loses responsibility for
calculating canonical URLs. I’ve opted to leave the methods in there
but they will throw warnings.

Fixes silverstripe/doc.silverstripe.org#181
@robbieaverill
Copy link
Contributor

Switched branch to 2

@robbieaverill
Copy link
Contributor

Yep, go for gold. I've also given @lexakami contributor access to merge this for something she's working on too.

@lexakami lexakami merged commit 6681ced into silverstripe:2 Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants