Merge pull request #247 from tractorcow/pulls/1.2/fix-subsites-urlsegment

BUG Fix issue with urlsegment being renamed in subsites

code/SubsitesVirtualPage.php

@@ -170,46 +170,6 @@ class SubsitesVirtualPage extends VirtualPage
             $this->ExtraMeta = $this->ContentSource()->ExtraMeta ? $this->ContentSource()->ExtraMeta : $this->ExtraMeta;
         }
     }
-    
-    public function validURLSegment()
-    {
-        $isValid = parent::validURLSegment();
-        
-        // Veto the validation rules if its false. In this case, some logic
-        // needs to be duplicated from parent to find out the exact reason the validation failed.
-        if (!$isValid) {
-            $IDFilter = ($this->ID) ? "AND \"SiteTree\".\"ID\" <> $this->ID" :  null;
-            $parentFilter = null;
-
-            if (Config::inst()->get('SiteTree', 'nested_urls')) {
-                if ($this->ParentID) {
-                    $parentFilter = " AND \"SiteTree\".\"ParentID\" = $this->ParentID";
-                } else {
-                    $parentFilter = ' AND "SiteTree"."ParentID" = 0';
-                }
-            }
-            
-            $origDisableSubsiteFilter = Subsite::$disable_subsite_filter;
-            Subsite::$disable_subsite_filter = true;
-            $existingPage = DataObject::get_one(
-                'SiteTree',
-                "\"URLSegment\" = '$this->URLSegment' $IDFilter $parentFilter",
-                false // disable cache, it doesn't include subsite status in the key
-            );
-            Subsite::$disable_subsite_filter = $origDisableSubsiteFilter;
-            $existingPageInSubsite = DataObject::get_one(
-                'SiteTree',
-                "\"URLSegment\" = '$this->URLSegment' $IDFilter $parentFilter",
-                false // disable cache, it doesn't include subsite status in the key
-            );
-
-            // If URL has been vetoed because of an existing page,
-            // be more specific and allow same URLSegments in different subsites
-            $isValid = !($existingPage && $existingPageInSubsite);
-        }
-        
-        return $isValid;
-    }
 }
 
 class SubsitesVirtualPage_Controller extends VirtualPage_Controller

code/extensions/SiteTreeSubsites.php

@@ -326,6 +326,33 @@ class SiteTreeSubsites extends DataExtension
         $this->owner->CrossSubsiteLinkTracking()->setByIDList($linkedPages);
     }
 
+	/**
+	 * Ensure that valid url segments are checked within the correct subsite of the owner object,
+	 * even if the current subsiteID is set to some other subsite.
+	 *
+	 * @return null|bool Either true or false, or null to not influence result
+	 */
+	public function augmentValidURLSegment()
+	{
+		// If this page is being filtered in the current subsite, then no custom validation query is required.
+		$subsite = Subsite::$force_subsite ?: Subsite::currentSubsiteID();
+		if (empty($this->owner->SubsiteID) || $subsite == $this->owner->SubsiteID) {
+			return null;
+		}
+
+		// Backup forced subsite
+		$prevForceSubsite = Subsite::$force_subsite;
+		Subsite::$force_subsite = $this->owner->SubsiteID;
+
+		// Repeat validation in the correct subsite
+		$isValid = $this->owner->validURLSegment();
+
+		// Restore
+		Subsite::$force_subsite = $prevForceSubsite;
+
+		return (bool)$isValid;
+	}
+
     /**
      * Return a piece of text to keep DataObject cache keys appropriately specific
      */

code/model/Subsite.php

@@ -99,7 +99,6 @@ class Subsite extends DataObject
      *
      * @todo Pass $request object from controller so we don't have to rely on $_GET
      *
-     * @param boolean $cache
      * @return int ID of the current subsite instance
      */
     public static function currentSubsiteID()

tests/BaseSubsiteTest.php

@@ -6,6 +6,7 @@ class BaseSubsiteTest extends SapphireTest
         parent::setUp();
 
         Subsite::$use_session_subsiteid = true;
+		Subsite::$force_subsite = null;
     }
 
     /**

tests/SiteTreeSubsitesTest.php

@@ -202,6 +202,65 @@ class SiteTreeSubsitesTest extends BaseSubsiteTest
         $this->assertNotContains('SiteTreeSubsitesTest_ClassA', $classes);
         $this->assertNotContains('SiteTreeSubsitesTest_ClassB', $classes);
     }
+
+	/**
+	 * Tests that url segments between subsites don't conflict, but do conflict within them
+	 */
+	public function testValidateURLSegment() {
+		$this->logInWithPermission('ADMIN');
+		// Saving existing page in the same subsite doesn't change urls
+		$mainHome = $this->objFromFixture('Page', 'home');
+		$mainSubsiteID = $this->idFromFixture('Subsite', 'main');
+		Subsite::changeSubsite($mainSubsiteID);
+		$mainHome->Content = '<p>Some new content</p>';
+		$mainHome->write();
+		$this->assertEquals('home', $mainHome->URLSegment);
+		$mainHome->doPublish();
+		$mainHomeLive = Versioned::get_one_by_stage('Page', 'Live', sprintf('"SiteTree"."ID" = \'%d\'', $mainHome->ID));
+		$this->assertEquals('home', $mainHomeLive->URLSegment);
+
+		// Saving existing page in another subsite doesn't change urls
+		Subsite::changeSubsite($mainSubsiteID);
+		$subsite1Home = $this->objFromFixture('Page', 'subsite1_home');
+		$subsite1Home->Content = '<p>In subsite 1</p>';
+		$subsite1Home->write();
+		$this->assertEquals('home', $subsite1Home->URLSegment);
+		$subsite1Home->doPublish();
+		$subsite1HomeLive = Versioned::get_one_by_stage('Page', 'Live', sprintf('"SiteTree"."ID" = \'%d\'', $subsite1Home->ID));
+		$this->assertEquals('home', $subsite1HomeLive->URLSegment);
+
+		// Creating a new page in a subsite doesn't conflict with urls in other subsites
+        $subsite1ID = $this->idFromFixture('Subsite', 'subsite1');
+		Subsite::changeSubsite($subsite1ID);
+		$subsite1NewPage = new Page();
+		$subsite1NewPage->SubsiteID = $subsite1ID;
+		$subsite1NewPage->Title = 'Important Page (Subsite 1)';
+		$subsite1NewPage->URLSegment = 'important-page'; // Also exists in main subsite
+		$subsite1NewPage->write();
+		$this->assertEquals('important-page', $subsite1NewPage->URLSegment);
+		$subsite1NewPage->doPublish();
+		$subsite1NewPageLive = Versioned::get_one_by_stage('Page', 'Live', sprintf('"SiteTree"."ID" = \'%d\'', $subsite1NewPage->ID));
+		$this->assertEquals('important-page', $subsite1NewPageLive->URLSegment);
+
+		// Creating a new page in a subsite DOES conflict with urls in the same subsite
+		$subsite1NewPage2 = new Page();
+		$subsite1NewPage2->SubsiteID = $subsite1ID;
+		$subsite1NewPage2->Title = 'Important Page (Subsite 1)';
+		$subsite1NewPage2->URLSegment = 'important-page'; // Also exists in main subsite
+		$subsite1NewPage2->write();
+		$this->assertEquals('important-page-2', $subsite1NewPage2->URLSegment);
+		$subsite1NewPage2->doPublish();
+		$subsite1NewPage2Live = Versioned::get_one_by_stage('Page', 'Live', sprintf('"SiteTree"."ID" = \'%d\'', $subsite1NewPage2->ID));
+		$this->assertEquals('important-page-2', $subsite1NewPage2Live->URLSegment);
+
+		// Original page is left un-modified
+		$mainSubsiteImportantPageID = $this->idFromFixture('Page', 'importantpage');
+		$mainSubsiteImportantPage = Page::get()->byID($mainSubsiteImportantPageID);
+		$this->assertEquals('important-page', $mainSubsiteImportantPage->URLSegment);
+		$mainSubsiteImportantPage->Content = '<p>New Important Page Content</p>';
+		$mainSubsiteImportantPage->write();
+		$this->assertEquals('important-page', $mainSubsiteImportantPage->URLSegment);
+	}
 }
 
 class SiteTreeSubsitesTest_ClassA extends SiteTree implements TestOnly

tests/SubsiteTest.php

@@ -48,6 +48,7 @@ class SubsiteTest extends BaseSubsiteTest
 
         // Test that changeSubsite is working
         Subsite::changeSubsite($template->ID);
+		$this->assertEquals($template->ID, Subsite::currentSubsiteID());
         $tmplStaff = $this->objFromFixture('Page', 'staff');
         $tmplHome = DataObject::get_one('Page', "\"URLSegment\" = 'home'");
 

tests/SubsiteTest.yml

@@ -64,40 +64,52 @@ Page:
   mainSubsitePage:
     Title: MainSubsitePage
     SubsiteID: 0
+    URLSegment: mainsubsitepage
   home:
     Title: Home
     SubsiteID: =>Subsite.main
+    URLSegment: home
   about:
     Title: About
     SubsiteID: =>Subsite.main
+    URLSegment: about
   linky:
      Title: Linky
      SubsiteID: =>Subsite.main
+     URLSegment: linky
   staff:
     Title: Staff
     ParentID: =>Page.about
     SubsiteID: =>Subsite.main
+    URLSegment: staff
   contact:
     Title: Contact Us
     SubsiteID: =>Subsite.main
+    URLSegment: contact-us
   importantpage:
     Title: Important Page
     SubsiteID: =>Subsite.main
+    URLSegment: important-page
   subsite1_home:
     Title: Home (Subsite 1)
     SubsiteID: =>Subsite.subsite1
+    URLSegment: home
   subsite1_contactus:
     Title: Contact Us (Subsite 1)
     SubsiteID: =>Subsite.subsite1
+    URLSegment: contact-us
   subsite1_staff:
     Title: Staff
     SubsiteID: =>Subsite.subsite1
+    URLSegment: staff
   subsite2_home:
     Title: Home (Subsite 2)
     SubsiteID: =>Subsite.subsite2
+    URLSegment: home
   subsite2_contactus:
     Title: Contact Us (Subsite 2)
     SubsiteID: =>Subsite.subsite2
+    URLSegment: contact-us
 
 PermissionRoleCode:
   roleCode1:

tests/SubsitesVirtualPageTest.php

@@ -274,6 +274,16 @@ class SubsitesVirtualPageTest extends BaseSubsiteTest
             $subsite2Vp->URLSegment,
             $subsite1Page->URLSegment,
             "Does allow explicit URLSegment overrides when only existing in a different subsite"
+        );
+
+		// When changing subsites and re-saving this page, it doesn't trigger a change
+		Subsite::changeSubsite($subsite1->ID);
+		$subsite1Page->write();
+		$subsite2Vp->write();
+        $this->assertEquals(
+            $subsite2Vp->URLSegment,
+            $subsite1Page->URLSegment,
+            "SubsiteVirtualPage doesn't change urls when being written in another subsite"
         );
     }