Skip to content

Commit

Permalink
ENHANCEMENT Backport versioned querystring fix (#2153)
Browse files Browse the repository at this point in the history 
* Backport versioned querystring fix

* Fix versioned state reset

* Fix up tests

* Fix permissions test

* Linting
  • Loading branch information
Damian Mooyman authored and Aaron Carlino committed May 7, 2018
1 parent 7cc4ce7 commit dfdaac4
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 103 deletions.
2 changes: 1 addition & 1 deletion code/controllers/CMSMain.php
View file
Expand Up @@ -188,7 +188,7 @@ public function Link($action = null) {
'/', // trailing slash needed if $action is null!
"$action"
);
$this->extend('updateLink', $link);
$this->extend('updateLink', $link, $action);
return $link;
}

Expand Down
17 changes: 10 additions & 7 deletions code/model/SiteTree.php
View file
Expand Up @@ -465,7 +465,10 @@ static public function link_shortcode_handler($arguments, $content = null, $pars
* @return string
*/
public function Link($action = null) {
return Controller::join_links(Director::baseURL(), $this->RelativeLink($action));
$relativeLink = $this->RelativeLink($action);
$link = Controller::join_links(Director::baseURL(), $relativeLink);
$this->extend('updateLink', $link, $action, $relativeLink);
return $link;
}

/**
Expand Down Expand Up @@ -542,7 +545,7 @@ public function RelativeLink($action = null) {
* @return string
*/
public function getAbsoluteLiveLink($includeStageEqualsLive = true) {
$oldStage = Versioned::current_stage();
$oldMode = Versioned::get_reading_mode();
Versioned::reading_stage('Live');
$live = Versioned::get_one_by_stage('SiteTree', 'Live', array(
'"SiteTree"."ID"' => $this->ID
Expand All @@ -556,7 +559,7 @@ public function getAbsoluteLiveLink($includeStageEqualsLive = true) {
$link = null;
}

Versioned::reading_stage($oldStage);
Versioned::set_reading_mode($oldMode);
return $link;
}

Expand Down Expand Up @@ -2429,7 +2432,7 @@ public function doUnpublish() {

$this->invokeWithExtensions('onBeforeUnpublish', $this);

$origStage = Versioned::current_stage();
$origMode = Versioned::get_reading_mode();
Versioned::reading_stage('Live');

// We should only unpublish virtualpages that exist on live
Expand All @@ -2445,7 +2448,7 @@ public function doUnpublish() {
// $page->write() calls syncLinkTracking, which does all the hard work for us.
$page->write();
}
Versioned::reading_stage($origStage);
Versioned::set_reading_mode($origMode);

// Unpublish any published virtual pages
if ($virtualPages) foreach($virtualPages as $vp) $vp->doUnpublish();
Expand Down Expand Up @@ -2521,7 +2524,7 @@ public function doRestoreToStage() {
if(method_exists($conn, 'allowPrimaryKeyEditing')) $conn->allowPrimaryKeyEditing('SiteTree', false);
}

$oldStage = Versioned::current_stage();
$oldMode = Versioned::get_reading_mode();
Versioned::reading_stage('Stage');
$this->forceChange();
$this->write();
Expand All @@ -2534,7 +2537,7 @@ public function doRestoreToStage() {
$page->write();
}

Versioned::reading_stage($oldStage);
Versioned::set_reading_mode($oldMode);

$this->invokeWithExtensions('onAfterRestoreToStage', $this);

Expand Down
24 changes: 12 additions & 12 deletions code/model/SiteTreeFileExtension.php
View file
Expand Up @@ -60,7 +60,7 @@ public function BackLinkTracking($filter = null, $sort = null, $join = null, $li
SiteTreeFileExtension::BackLinkTracking() have been deprecated.
Please manipluate the returned list directly.', Deprecation::SCOPE_GLOBAL);
}

if(class_exists("Subsite")){
$rememberSubsiteFilter = Subsite::$disable_subsite_filter;
Subsite::disable_subsite_filter(true);
Expand All @@ -71,7 +71,7 @@ public function BackLinkTracking($filter = null, $sort = null, $join = null, $li
SiteTreeFileExtension::BackLinkTracking() have been deprecated.
Please manipluate the returned list directly.', Deprecation::SCOPE_GLOBAL);
}

$links = $this->owner->getManyManyComponents('BackLinkTracking');
if($this->owner->ID) {
$links = $links
Expand All @@ -80,14 +80,14 @@ public function BackLinkTracking($filter = null, $sort = null, $join = null, $li
->limit($limit);
}
$this->owner->extend('updateBackLinkTracking', $links);

if(class_exists("Subsite")){
Subsite::disable_subsite_filter($rememberSubsiteFilter);
}

return $links;
}

/**
* @todo Unnecessary shortcut for AssetTableField, coupled with cms module.
*
Expand All @@ -101,7 +101,7 @@ public function BackLinkTrackingCount() {
return 0;
}
}

/**
* Updates link tracking.
*/
Expand All @@ -110,7 +110,7 @@ public function onAfterDelete() {
// site does its thing
$brokenPageIDs = $this->owner->BackLinkTracking()->column("ID");
if($brokenPageIDs) {
$origStage = Versioned::current_stage();
$origMode = Versioned::get_reading_mode();

// This will syncLinkTracking on draft
Versioned::reading_stage('Stage');
Expand All @@ -124,10 +124,10 @@ public function onAfterDelete() {
$brokenPage->write();
}

Versioned::reading_stage($origStage);
Versioned::set_reading_mode($origMode);
}
}

/**
* Rewrite links to the $old file to now point to the $new file.
*
Expand All @@ -138,15 +138,15 @@ public function onAfterDelete() {
*/
public function updateLinks($old, $new) {
if(class_exists('Subsite')) Subsite::disable_subsite_filter(true);

$pages = $this->owner->BackLinkTracking();

$summary = "";
if($pages) {
foreach($pages as $page) $page->rewriteFileURL($old,$new);
}

if(class_exists('Subsite')) Subsite::disable_subsite_filter(false);
}

}
14 changes: 10 additions & 4 deletions tests/controller/ContentControllerTest.php
View file
Expand Up @@ -7,16 +7,21 @@ class ContentControllerTest extends FunctionalTest {

protected static $fixture_file = 'ContentControllerTest.yml';

protected static $use_draft_site = true;

protected static $disable_themes = true;

public function setUp() {
parent::setUp();
Config::inst()->update('Director', 'alternate_base_url', '/');
$this->useDraftSite(false);
}

/**
* Test that nested pages, basic actions, and nested/non-nested URL switching works properly
*/

public function testNestedPages() {
RootURLController::reset();
$this->useDraftSite(true);
Config::inst()->update('SiteTree', 'nested_urls', true);

$this->assertEquals('Home Page', $this->get('/')->getBody());
Expand Down Expand Up @@ -52,7 +57,7 @@ public function testNestedPages() {
*/
public function testChildrenOf() {
$controller = new ContentController();

$this->useDraftSite(true);
Config::inst()->update('SiteTree', 'nested_urls', true);

$this->assertEquals(1, $controller->ChildrenOf('/')->Count());
Expand All @@ -70,6 +75,7 @@ public function testChildrenOf() {

public function testDeepNestedURLs() {
Config::inst()->update('SiteTree', 'nested_urls', true);
$this->useDraftSite(true);

$page = new Page();
$page->URLSegment = 'base-page';
Expand Down Expand Up @@ -126,7 +132,7 @@ public function testLinkShortcodes() {

$this->assertContains(
sprintf('<a href="%s">Testlink</a>', $linkedPage->Link()),
$this->get($page->RelativeLink())->getBody(),
$this->get($page->Link())->getBody(),
'"sitetree_link" shortcodes get parsed properly'
);
}
Expand Down
41 changes: 26 additions & 15 deletions tests/model/RedirectorPageTest.php
View file
Expand Up @@ -2,47 +2,58 @@

class RedirectorPageTest extends FunctionalTest {
protected static $fixture_file = 'RedirectorPageTest.yml';
protected static $use_draft_site = true;

protected $autoFollowRedirection = false;


public function setUp() {
parent::setUp();
$this->useDraftSite(false);
$this->logInWithPermission('ADMIN');
foreach (SiteTree::get() as $page) {
$page->doPublish();
}
Config::inst()->update('Director', 'alternate_base_url', '/');
}

public function testGoodRedirectors() {
/* For good redirectors, the final destination URL will be returned */
$this->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());
$this->assertEquals("/redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->redirectionLink());
$this->assertEquals("/redirection-dest/", $this->objFromFixture('RedirectorPage','goodinternal')->Link());
}

public 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());
$this->assertEquals('/bad-external/', $page1->Link());

/* An error message will be shown if you visit it */
$content = $this->get(Director::makeRelative($page1->Link()))->getBody();
$content = $this->get($page1->Link())->getBody();
$this->assertContains('message-setupWithoutRedirect', $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->assertEquals('/bad-internal/', $page2->Link());
$content = $this->get($page2->Link())->getBody();
$this->assertContains('message-setupWithoutRedirect', $content);
}

public function testReflexiveAndTransitiveInternalRedirectors() {
/* Reflexive redirectors are those that point to themselves. They should behave the same as an empty redirector */
/** @var RedirectorPage $page */
$page = $this->objFromFixture('RedirectorPage','reflexive');
$this->assertEquals(Director::baseURL() . 'reflexive/', $page->Link());
$content = $this->get(Director::makeRelative($page->Link()))->getBody();
$this->assertEquals('/reflexive/', $page->Link());
$content = $this->get($page->Link())->getBody();
$this->assertContains('message-setupWithoutRedirect', $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());
$page = $this->objFromFixture('RedirectorPage', 'transitive');
$this->assertEquals('/good-internal/', $page->Link());

$this->autoFollowRedirection = false;
$response = $this->get(Director::makeRelative($page->Link()));
$this->assertEquals(Director::baseURL() . "redirection-dest/", $response->getHeader("Location"));
$response = $this->get($page->Link());
$this->assertEquals("/redirection-dest/", $response->getHeader("Location"));
}

public function testExternalURLGetsPrefixIfNotSet() {
Expand Down

0 comments on commit dfdaac4

Please sign in to comment.