Merge pull request #338 from creative-commoners/pulls/2.0/fix-validat…

…ion-error Pulls/2.0/fix validation error
silverstripe · Feb 1, 2018 · 73943af · 73943af
2 parents cde41c4 + 34c0c49
commit 73943af
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 40 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -46,7 +46,7 @@ before_script:
 script:
   - if [[ $PHPUNIT_TEST ]]; then vendor/bin/phpunit; fi
   - if [[ $PHPUNIT_COVERAGE_TEST ]]; then phpdbg -qrr vendor/bin/phpunit --coverage-clover=coverage.xml; fi
-  - if [[ $PHPCS_TEST ]]; then vendor/bin/phpcs --standard=vendor/silverstripe/framework/phpcs.xml.dist src tests *.php --ignore=host-map.php; fi
+  - if [[ $PHPCS_TEST ]]; then vendor/bin/phpcs src tests *.php --ignore=host-map.php; fi
   - if [[ $BEHAT_TEST ]]; then vendor/bin/behat @subsites; fi
 
 after_success:

diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ruleset name="SilverStripe">
+    <description>CodeSniffer ruleset for SilverStripe coding conventions.</description>
+
+    <!-- base rules are PSR-2 -->
+    <rule ref="PSR2" >
+        <!-- Current exclusions -->
+        <exclude name="PSR1.Methods.CamelCapsMethodName" />
+        <exclude name="PSR1.Files.SideEffects.FoundWithSymbols" />
+    </rule>
+</ruleset>
diff --git a/src/Extensions/GroupSubsites.php b/src/Extensions/GroupSubsites.php
@@ -187,7 +187,8 @@ public function augmentSQL(SQLSelect $query, DataQuery $dataQuery = null)
                 }
             }
 
-            // WORKAROUND for databases that complain about an ORDER BY when the column wasn't selected (e.g. SQL Server)
+            // WORKAROUND for databases that complain about an ORDER BY when the column wasn't selected
+            // (e.g. SQL Server)
             $select = $query->getSelect();
             if (isset($select[0]) && !$select[0] == 'COUNT(*)') {
                 $query->addOrderBy('AccessAllSubsites', 'DESC');
@@ -230,7 +231,10 @@ public function providePermissions()
         return [
             'SECURITY_SUBSITE_GROUP' => [
                 'name' => _t(__CLASS__ . '.MANAGE_SUBSITES', 'Manage subsites for groups'),
-                'category' => _t('SilverStripe\\Security\\Permission.PERMISSIONS_CATEGORY', 'Roles and access permissions'),
+                'category' => _t(
+                    'SilverStripe\\Security\\Permission.PERMISSIONS_CATEGORY',
+                    'Roles and access permissions'
+                ),
                 'help' => _t(
                     __CLASS__ . '.MANAGE_SUBSITES_HELP',
                     'Ability to limit the permissions for a group to one or more subsites.'

diff --git a/src/Extensions/SiteTreeSubsites.php b/src/Extensions/SiteTreeSubsites.php
@@ -62,7 +62,10 @@ public function augmentSQL(SQLSelect $query, DataQuery $dataQuery = null)
         }
 
         // If you're querying by ID, ignore the sub-site - this is a bit ugly...
-        // if(!$query->where || (strpos($query->where[0], ".\"ID\" = ") === false && strpos($query->where[0], ".`ID` = ") === false && strpos($query->where[0], ".ID = ") === false && strpos($query->where[0], "ID = ") !== 0)) {
+        // if(!$query->where
+        // || (strpos($query->where[0], ".\"ID\" = ") === false
+        // && strpos($query->where[0], ".`ID` = ") === false && strpos($query->where[0], ".ID = ") === false
+        // && strpos($query->where[0], "ID = ") !== 0)) {
         if ($query->filtersOnID()) {
             return;
         }
@@ -111,7 +114,7 @@ public function updateCMSFields(FieldList $fields)
         // Master page edit field (only allowed from default subsite to avoid inconsistent relationships)
         $isDefaultSubsite = $this->owner->SubsiteID == 0 || $this->owner->Subsite()->DefaultSite;
 
-        if ($isDefaultSubsite && $subsitesMap) {
+        if ($isDefaultSubsite && $subsitesMap->count()) {
             $fields->addFieldToTab(
                 'Root.Main',
                 ToggleCompositeField::create(
@@ -418,7 +421,10 @@ public function augmentSyncLinkTracking()
                         Subsite::disable_subsite_filter(true);
                         $candidatePage = DataObject::get_one(
                             SiteTree::class,
-                            "\"URLSegment\" = '" . Convert::raw2sql(urldecode($rest)) . "' AND \"SubsiteID\" = " . $subsiteID,
+                            "\"URLSegment\" = '"
+                            . Convert::raw2sql(urldecode($rest))
+                            . "' AND \"SubsiteID\" = "
+                            . $subsiteID,
                             false
                         );
                         Subsite::disable_subsite_filter($origDisableSubsiteFilter);

diff --git a/src/Forms/GridFieldSubsiteDetailForm.php b/src/Forms/GridFieldSubsiteDetailForm.php
@@ -6,5 +6,5 @@
 
 class GridFieldSubsiteDetailForm extends GridFieldDetailForm
 {
-    protected $itemRequestClass = GridFieldSubsiteDetailForm_ItemRequest::class;
+    protected $itemRequestClass = GridFieldSubsiteDetailFormItemRequest::class;
 }
diff --git a/...ridFieldSubsiteDetailForm_ItemRequest.php → ...GridFieldSubsiteDetailFormItemRequest.php b/...ridFieldSubsiteDetailForm_ItemRequest.php → ...GridFieldSubsiteDetailFormItemRequest.php
@@ -7,7 +7,7 @@
 use SilverStripe\Forms\GridField\GridFieldDetailForm_ItemRequest;
 use SilverStripe\Subsites\Model\Subsite;
 
-class GridFieldSubsiteDetailForm_ItemRequest extends GridFieldDetailForm_ItemRequest
+class GridFieldSubsiteDetailFormItemRequest extends GridFieldDetailForm_ItemRequest
 {
 
     private static $allowed_actions = [

diff --git a/src/Model/Subsite.php b/src/Model/Subsite.php
@@ -74,14 +74,14 @@ class Subsite extends DataObject
      *
      * @array
      */
-    private static $_cache_accessible_sites = [];
+    protected static $cache_accessible_sites = [];
 
     /**
      * Memory cache of subsite id for domains
      *
      * @var array
      */
-    private static $_cache_subsite_for_domain = [];
+    protected static $cache_subsite_for_domain = [];
 
     /**
      * Numeric array of all themes which are allowed to be selected for all subsites.
@@ -268,8 +268,8 @@ public static function getSubsiteIDForDomain($host = null, $checkPermissions = t
 
             $currentUserId = Security::getCurrentUser() ? Security::getCurrentUser()->ID : 0;
             $cacheKey = implode('_', [$host, $currentUserId, static::config()->get('check_is_public')]);
-            if (isset(self::$_cache_subsite_for_domain[$cacheKey])) {
-                return self::$_cache_subsite_for_domain[$cacheKey];
+            if (isset(self::$cache_subsite_for_domain[$cacheKey])) {
+                return self::$cache_subsite_for_domain[$cacheKey];
             }
 
             $SQL_host = Convert::raw2sql($host);
@@ -319,7 +319,7 @@ public static function getSubsiteIDForDomain($host = null, $checkPermissions = t
         }
 
         if ($cacheKey) {
-            self::$_cache_subsite_for_domain[$cacheKey] = $subsiteID;
+            self::$cache_subsite_for_domain[$cacheKey] = $subsiteID;
         }
 
         return $subsiteID;
@@ -355,8 +355,8 @@ public static function disable_subsite_filter($disabled = true)
      */
     public static function on_db_reset()
     {
-        self::$_cache_accessible_sites = [];
-        self::$_cache_subsite_for_domain = [];
+        self::$cache_accessible_sites = [];
+        self::$cache_subsite_for_domain = [];
     }
 
     /**
@@ -462,8 +462,8 @@ public static function accessible_sites(
 
         // Cache handling
         $cacheKey = $SQL_codes . '-' . $member->ID . '-' . $includeMainSite . '-' . $mainSiteTitle;
-        if (isset(self::$_cache_accessible_sites[$cacheKey])) {
-            return self::$_cache_accessible_sites[$cacheKey];
+        if (isset(self::$cache_accessible_sites[$cacheKey])) {
+            return self::$cache_accessible_sites[$cacheKey];
         }
 
         /** @skipUpgrade */
@@ -480,7 +480,9 @@ public static function accessible_sites(
             )
             ->innerJoin(
                 'Permission',
-                "\"Group\".\"ID\"=\"Permission\".\"GroupID\" AND \"Permission\".\"Code\" IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
+                "\"Group\".\"ID\"=\"Permission\".\"GroupID\" 
+                AND \"Permission\".\"Code\" 
+                IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
             );
 
         if (!$subsites) {
@@ -504,7 +506,9 @@ public static function accessible_sites(
             ->innerJoin('PermissionRole', '"Group_Roles"."PermissionRoleID"="PermissionRole"."ID"')
             ->innerJoin(
                 'PermissionRoleCode',
-                "\"PermissionRole\".\"ID\"=\"PermissionRoleCode\".\"RoleID\" AND \"PermissionRoleCode\".\"Code\" IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
+                "\"PermissionRole\".\"ID\"=\"PermissionRoleCode\".\"RoleID\" 
+                AND \"PermissionRoleCode\".\"Code\" 
+                IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
             );
 
         if (!$subsites && $rolesSubsites) {
@@ -535,7 +539,7 @@ public static function accessible_sites(
             }
         }
 
-        self::$_cache_accessible_sites[$cacheKey] = $subsites;
+        self::$cache_accessible_sites[$cacheKey] = $subsites;
 
         return $subsites;
     }
@@ -628,10 +632,12 @@ public static function hasMainSitePermission($member = null, $permissionCodes =
         $groupCount = DB::query("
             SELECT COUNT(\"Permission\".\"ID\")
             FROM \"Permission\"
-            INNER JOIN \"Group\" ON \"Group\".\"ID\" = \"Permission\".\"GroupID\" AND \"Group\".\"AccessAllSubsites\" = 1
-            INNER JOIN \"Group_Members\" ON \"Group_Members\".\"GroupID\" = \"Permission\".\"GroupID\"
-            WHERE \"Permission\".\"Code\" IN ('$SQL_perms')
-            AND \"Group_Members\".\"MemberID\" = {$memberID}
+            INNER JOIN \"Group\"
+            ON \"Group\".\"ID\" = \"Permission\".\"GroupID\" AND \"Group\".\"AccessAllSubsites\" = 1
+            INNER JOIN \"Group_Members\"
+            ON \"Group_Members\".\"GroupID\" = \"Permission\".\"GroupID\"
+            WHERE \"Permission\".\"Code\"
+            IN ('$SQL_perms') AND \"Group_Members\".\"MemberID\" = {$memberID}
         ")->value();
 
         // Count this user's groups which have a role that can access the main site

diff --git a/src/Model/SubsiteDomain.php b/src/Model/SubsiteDomain.php
@@ -166,15 +166,12 @@ public function Link()
     public function getFullProtocol()
     {
         switch ($this->Protocol) {
-            case self::PROTOCOL_HTTPS: {
+            case self::PROTOCOL_HTTPS:
                 return 'https://';
-            }
-            case self::PROTOCOL_HTTP: {
+            case self::PROTOCOL_HTTP:
                 return 'http://';
-            }
-            default: {
+            default:
                 return Director::protocol();
-            }
         }
     }
 

diff --git a/src/Pages/SubsitesVirtualPage.php b/src/Pages/SubsitesVirtualPage.php
@@ -141,9 +141,18 @@ public function fieldLabels($includerelations = true)
     {
         $labels = parent::fieldLabels($includerelations);
         $labels['CustomMetaTitle'] = _t('SilverStripe\\Subsites\\Model\\Subsite.CustomMetaTitle', 'Title');
-        $labels['CustomMetaKeywords'] = _t('SilverStripe\\Subsites\\Model\\Subsite.CustomMetaKeywords', 'Keywords');
-        $labels['CustomMetaDescription'] = _t('SilverStripe\\Subsites\\Model\\Subsite.CustomMetaDescription', 'Description');
-        $labels['CustomExtraMeta'] = _t('SilverStripe\\Subsites\\Model\\Subsite.CustomExtraMeta', 'Custom Meta Tags');
+        $labels['CustomMetaKeywords'] = _t(
+            'SilverStripe\\Subsites\\Model\\Subsite.CustomMetaKeywords',
+            'Keywords'
+        );
+        $labels['CustomMetaDescription'] = _t(
+            'SilverStripe\\Subsites\\Model\\Subsite.CustomMetaDescription',
+            'Description'
+        );
+        $labels['CustomExtraMeta'] = _t(
+            'SilverStripe\\Subsites\\Model\\Subsite.CustomExtraMeta',
+            'Custom Meta Tags'
+        );
 
         return $labels;
     }

diff --git a/tests/php/SiteTreeSubsitesTest.php b/tests/php/SiteTreeSubsitesTest.php
@@ -288,7 +288,7 @@ public function testValidateURLSegment()
         $mainHome->Content = '<p>Some new content</p>';
         $mainHome->write();
         $this->assertEquals('home', $mainHome->URLSegment);
-        $mainHome->doPublish();
+        $mainHome->publishRecursive();
         $mainHomeLive = Versioned::get_one_by_stage('Page', 'Live', sprintf('"SiteTree"."ID" = \'%d\'', $mainHome->ID));
         $this->assertEquals('home', $mainHomeLive->URLSegment);
 
@@ -298,7 +298,7 @@ public function testValidateURLSegment()
         $subsite1Home->Content = '<p>In subsite 1</p>';
         $subsite1Home->write();
         $this->assertEquals('home', $subsite1Home->URLSegment);
-        $subsite1Home->doPublish();
+        $subsite1Home->publishRecursive();
         $subsite1HomeLive = Versioned::get_one_by_stage(
             'Page',
             'Live',
@@ -315,7 +315,7 @@ public function testValidateURLSegment()
         $subsite1NewPage->URLSegment = 'important-page'; // Also exists in main subsite
         $subsite1NewPage->write();
         $this->assertEquals('important-page', $subsite1NewPage->URLSegment);
-        $subsite1NewPage->doPublish();
+        $subsite1NewPage->publishRecursive();
         $subsite1NewPageLive = Versioned::get_one_by_stage(
             'Page',
             'Live',
@@ -330,7 +330,7 @@ public function testValidateURLSegment()
         $subsite1NewPage2->URLSegment = 'important-page'; // Also exists in main subsite
         $subsite1NewPage2->write();
         $this->assertEquals('important-page-2', $subsite1NewPage2->URLSegment);
-        $subsite1NewPage2->doPublish();
+        $subsite1NewPage2->publishRecursive();
         $subsite1NewPage2Live = Versioned::get_one_by_stage(
             'Page',
             'Live',
@@ -430,6 +430,9 @@ public function provideAlternateAbsoluteLink()
      */
     public function testAlternateAbsoluteLink($pageFixtureName, $action, $expectedAbsoluteLink)
     {
+        // Setting a control value, in case base url is set for the installation under test
+        Config::modify()->set(Director::class, 'alternate_base_url', 'http://localhost/');
+
         /** @var Page $page */
         $page = $this->objFromFixture(Page::class, $pageFixtureName);
 

diff --git a/tests/php/SubsiteAdminTest.php b/tests/php/SubsiteAdminTest.php
@@ -39,7 +39,8 @@ public function testBasicView()
 
         // Confirm that this URL gets you the entire page, with the edit form loaded
         $response2 = Director::test(
-            "admin/subsites/SilverStripe-Subsites-Model-Subsite/EditForm/field/SilverStripe-Subsites-Model-Subsite/item/$subsite1ID/edit",
+            "admin/subsites/SilverStripe-Subsites-Model-Subsite/EditForm/field/"
+            ."SilverStripe-Subsites-Model-Subsite/item/$subsite1ID/edit",
             null,
             $this->adminLoggedInSession()
         );

diff --git a/tests/php/SubsiteTest.php b/tests/php/SubsiteTest.php
@@ -57,7 +57,8 @@ public function testSubsiteCreation()
         $tmplStaff = $this->objFromFixture('Page', 'staff');
         $tmplHome = DataObject::get_one('Page', "\"URLSegment\" = 'home'");
 
-        // Publish all the pages in the template, testing that DataObject::get only returns pages from the chosen subsite
+        // Publish all the pages in the template, testing that DataObject::get only returns pages
+        // from the chosen subsite
         $pages = DataObject::get(SiteTree::class);
         $totalPages = $pages->count();
         foreach ($pages as $page) {
@@ -193,7 +194,8 @@ public function testStrictSubdomainMatching()
         $this->assertEquals(
             $subsite1->ID,
             Subsite::getSubsiteIDForDomain('www.example.org'),
-            'Matches without strict checking when using www prefix, still matching first domain regardless of www prefix  (falling back to subsite primary key ordering)'
+            'Matches without strict checking when using www prefix, '
+            .'still matching first domain regardless of www prefix  (falling back to subsite primary key ordering)'
         );
         $this->assertEquals(
             $subsite1->ID,