diff --git a/core/model/RedirectorPage.php b/core/model/RedirectorPage.php index a5dcab223..980e0146f 100755 --- a/core/model/RedirectorPage.php +++ b/core/model/RedirectorPage.php @@ -33,13 +33,54 @@ class RedirectorPage extends Page { } } + /** + * Return the the link that should be used for this redirector page, in navigation, etc. + * If the redirectorpage has been appropriately configured, then it will return the redirection + * destination, to prevent unnecessary 30x redirections. However, if it's misconfigured, then + * it will return a link to itself, which will then display an error message. + */ function Link() { + if($link = $this->redirectionLink()) return $link; + else return $this->regularLink(); + } + + /** + * Return the normal link directly to this page. Once you visit this link, a 30x redirection + * will take you to your final destination. + */ + function regularLink() { + return parent::Link(); + } + + /** + * Return the link that we should redirect to. + * Only return a value if there is a legal redirection destination. + */ + function redirectionLink() { if($this->RedirectionType == 'External') { - return Convert::raw2att($this->ExternalURL); + if($this->ExternalURL) { + return Convert::raw2att($this->ExternalURL); + } + } else { $linkTo = $this->LinkToID ? DataObject::get_by_id("SiteTree", $this->LinkToID) : null; + if($linkTo) { - return $linkTo->Link(); + // We shouldn't point to ourselves - that would create an infinite loop! Return null since we have a + // bad configuration + if($this->ID == $linkTo->ID) { + return null; + + // If we're linking to another redirectorpage then just return the URLSegment, to prevent a cycle of redirector + // pages from causing an infinite loop. Instead, they will cause a 30x redirection loop in the browser, but + // this can be handled sufficiently gracefully by the browser. + } elseif($linkTo instanceof RedirectorPage) { + return $linkTo->regularLink(); + + // For all other pages, just return the link of the page. + } else { + return $linkTo->Link(); + } } } } @@ -108,17 +149,10 @@ class RedirectorPage extends Page { */ class RedirectorPage_Controller extends Page_Controller { function init() { - if($this->RedirectionType == 'External') { - if($this->ExternalURL) { - Director::redirect($this->ExternalURL); - } - } else { - $linkTo = DataObject::get_by_id("SiteTree", $this->LinkToID); - if($linkTo) { - Director::redirect($linkTo->Link(), 301); - } + if($link = $this->redirectionLink()) { + Director::redirect($link, 301); } - + parent::init(); } @@ -126,9 +160,11 @@ class RedirectorPage_Controller extends Page_Controller { * If we ever get this far, it means that the redirection failed. */ function index() { - return "

" . + return array( + "Content" => "

" . _t('RedirectorPage.HASBEENSETUP', 'A redirector page has been set up without anywhere to redirect to.') . - "

"; + "

" + ); } } ?> \ No newline at end of file diff --git a/tests/RedirectorPageTest.php b/tests/RedirectorPageTest.php new file mode 100644 index 000000000..3a7b5138c --- /dev/null +++ b/tests/RedirectorPageTest.php @@ -0,0 +1,48 @@ +assertEquals("http://www.google.com", $this->objFromFixture('RedirectorPage','goodexternal')->Link()); + $this->assertEquals(Director::baseURL() . "redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->redirectionLink()); + $this->assertEquals(Director::baseURL() . "redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->Link()); + } + + function testEmptyRedirectors() { + /* If a redirector page is misconfigured, then its link method will just return the usual URLSegment-generated value */ + $page1 = $this->objFromFixture('RedirectorPage','badexternal'); + $this->assertEquals(Director::baseURL() . 'bad-external/', $page1->Link()); + + /* An error message will be shown if you visit it */ + $content = $this->get(Director::makeRelative($page1->Link()))->getBody(); + $this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content); + + /* This also applies for internal links */ + $page2 = $this->objFromFixture('RedirectorPage','badinternal'); + $this->assertEquals(Director::baseURL() . 'bad-internal/', $page2->Link()); + $content = $this->get(Director::makeRelative($page2->Link()))->getBody(); + $this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content); + } + + function testReflexiveAndTransitiveInternalRedirectors() { + /* Reflexive redirectors are those that point to themselves. They should behave the same as an empty redirector */ + $page = $this->objFromFixture('RedirectorPage','reflexive'); + $this->assertEquals(Director::baseURL() . 'reflexive/', $page->Link()); + $content = $this->get(Director::makeRelative($page->Link()))->getBody(); + $this->assertContains("A redirector page has been set up without anywhere to redirect to.", $content); + + /* Transitive redirectors are those that point to another redirector page. They should send people to the URLSegment + * of the destination page - the middle-stop, so to speak. That should redirect to the final destination */ + $page = $this->objFromFixture('RedirectorPage','transitive'); + $this->assertEquals(Director::baseURL() . 'good-internal/', $page->Link()); + + $this->autoFollowRedirection = false; + $response = $this->get(Director::makeRelative($page->Link())); + $this->assertEquals(Director::baseURL() . "redirection-dest/", $response->getHeader("Location")); + } +} + +?> \ No newline at end of file diff --git a/tests/RedirectorPageTest.yml b/tests/RedirectorPageTest.yml new file mode 100644 index 000000000..11fae94b6 --- /dev/null +++ b/tests/RedirectorPageTest.yml @@ -0,0 +1,34 @@ +Page: + dest: + Title: Redirection Dest + URLSegment: redirection-dest + +RedirectorPage: + goodexternal: + Title: Good External + URLSegment: good-external + RedirectionType: External + ExternalURL: http://www.google.com + goodinternal: + Title: Good Internal + URLSegment: good-internal + RedirectionType: Internal: + LinkTo: =>Page.dest + badexternal: + Title: Bad External + RedirectionType: External + URLSegment: bad-external + badinternal: + Title: Bad Internal + RedirectionType: Internal + URLSegment: bad-internal + reflexive: + Title: Reflexive + RedirectionType: Internal + LinkTo: =>RedirectorPage.reflexive + URLSegment: reflexive + transitive: + Title: Transitive + RedirectionType: Internal + LinkTo: =>RedirectorPage.goodinternal + URLSegment: transitive