Merge pull request #152 from sminnee/fix-canonical-url

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

code/controllers/DocumentationViewer.php

@@ -588,10 +588,26 @@ class DocumentationViewer extends Controller implements PermissionProvider
      */
     public function getCanonicalUrl()
     {
-        if (!$this->getPage()) {
+        $page = $this->getPage();
+
+        if (!$page) {
             return '';
         }
-        return $this->getPage()->getCanonicalUrl();
+
+        // Build the basic canoncial URL, relative to link_base as is mapped in the manifest
+        $url = Controller::join_links(
+            $page->getEntity()->getLanguage(),
+            $page->getRelativeLink()
+        );
+
+        // If the canoncial URL is, in fact, a redirect, post that directly - redirecting canoncials are a bad thing.
+        // See https://github.com/silverstripe/doc.silverstripe.org/issues/181 for discussion
+        if ($potentialRedirect = $this->manifest->getRedirect($url)) {
+            $url = $potentialRedirect;
+        }
+
+        // Turn into an absolute URL
+        return Director::absoluteUrl(Controller::join_links(self::getDocumentationBaseHref(), $url));
     }
 
     /**

code/models/DocumentationPage.php

@@ -270,6 +270,8 @@ class DocumentationPage extends ViewableData
 
     /**
      * Determine and set the canonical URL for the given record, for example: dev/docs/en/Path/To/Document
+     *
+     * @deprecated 2.1.0 This method is no longer used.
      */
     public function populateCanonicalUrl()
     {
@@ -371,11 +373,14 @@ class DocumentationPage extends ViewableData
     /**
      * Set the canonical URL to use for this page
      *
+     * @deprecated 2.1.0 This method is no longer used.
      * @param string $canonicalUrl
      * @return $this
      */
     public function setCanonicalUrl($canonicalUrl)
     {
+        user_error("DocumentationPage::setCanonicalUrl() is deprecated; it won't affect DocumentationViewer", E_USER_WARNING);
+
         $this->canonicalUrl = $canonicalUrl;
         return $this;
     }
@@ -384,10 +389,16 @@ class DocumentationPage extends ViewableData
      * Get the canonical URL to use for this page. Will trigger discovery
      * via {@link DocumentationPage::populateCanonicalUrl()} if none is already set.
      *
+     * @deprecated 2.1.0 This method is no longer used.
      * @return string
      */
     public function getCanonicalUrl()
     {
+        user_error(
+            "DocumentationPage::getCanonicalUrl() is deprecated; use DocumentationViewer::getCanonicalUrl() instead.",
+            E_USER_WARNING
+        );
+
         if (!$this->canonicalUrl) {
             $this->populateCanonicalUrl();
         }

tests/DocumentationPageTest.php

@@ -90,22 +90,4 @@ class DocumentationPageTest extends SapphireTest
 
         $this->assertEquals('Sort - Doctest', $page->getBreadcrumbTitle());
     }
-
-    public function testGetCanonicalUrl()
-    {
-        $page = new DocumentationPage(
-            $this->entity,
-            'file.md',
-            DOCSVIEWER_PATH . '/tests/docs/en/test/file.md'
-        );
-
-        $this->assertContains(
-            'dev/docs/en/test/file/',
-            $page->getCanonicalUrl(),
-            'Canonical URL is determined, set and returned'
-        );
-
-        $page->setCanonicalUrl('some-other-url');
-        $this->assertSame('some-other-url', $page->getCanonicalUrl(), 'Canonical URL can be adjusted via public API');
-    }
 }