SilverStripe 4 Compatibility #289

Cheddam · 2017-08-28T10:31:36Z

This giant PR, in short, makes Subsites function on SilverStripe 4 Beta 2.

It does not include:

Passing tests
Design updates to match SS4
An in-depth review of the architecture (this is very lift-and-shift, and I'm sure there are a lot of improvements in SS4 we're not yet taking advantage of)

Shout out to @wernerkrauss for completing a significant portion of the work!

(cherry picked from commit 2f99e51)

(cherry picked from commit e201462)

- moving all phpunit tests into tests/php - moving all extensions from _config.php into config.yml and removing obsolete _config.php - moving GridFieldSubsiteDetailForm_ItemRequest into own file (cherry picked from commit ee02828)

call extension directly in test, as Versioned now is also applied to File and has this method

Updating config and i18n calls

robbieaverill

Epic work @Cheddam @wernerkrauss and everyone else involved! I've left a few comments, most are things that occur a lot throughout this pull request, so should be updated in bulk. If we can get this pull request to a point where the tests are at least running on Travis (not necessarily passing) then we can look at merging this as a basis then work incrementally in smaller, more manageable pull requests from then on.

robbieaverill · 2017-08-28T22:01:29Z

.travis.yml

@@ -4,6 +4,8 @@ language: php

 sudo: false

+sudo: false
+


We should create a new Travis config to replace this, see silverstripe/silverstripe-taxonomy#36 for an example

robbieaverill · 2017-08-28T22:04:20Z

code/admin/SubsiteAdmin.php

@@ -0,0 +1,42 @@
+<?php
+
+namespace SilverStripe\Subsites\Admin;


Ideally the file path would match the namespace for PSR-4, e.g. Admin rather than admin

👍 for this, you can address this using git mv to ensure the case sensitivity is emitted to the log.

I'd not even mind if you renamed 'code' to 'src' (new standard) as a means of addressing this, but the important thing is that it's PSR-4. :)

Yeah, if you move code to src then you can also capitalise the subfolders at the same time. Otherwise you'll need to do something like git mv admin blah && git mv blah Admin since it won't pick up the case change otherwise (at least on macOS anyway)

robbieaverill · 2017-08-28T22:05:54Z

code/admin/SubsiteAdmin.php

+    {
+        $form = parent::getEditForm($id, $fields);
+
+        $grid = $form->Fields()->dataFieldByName(Subsite::class);


I think this is likely a mistake - dataFieldByName expects a field name string which wouldn't be a FQCN. This could have been an upgrader tool error, but should be replaced with 'Subsite'

This is the fault of the upgrader;

You can put /** @skipUpgrade */ on either the line before this statement, or in the getEditForm PHPDoc to suppress the upgrader making this subsitution.

robbieaverill · 2017-08-28T22:06:59Z

code/controller/SubsiteXHRController.php

     * Relax the access permissions, so anyone who has access to any CMS subsite can access this controller.
+     * @param null $member


It's better to type hint the expected type as well as, rather than the optional fallback e.g. @param Member|null $member

👍 to this.

robbieaverill · 2017-08-28T22:08:53Z

code/extensions/ControllerSubsites.php

@@ -8,11 +14,11 @@ public function controllerAugmentInit()
    {
        if ($subsite = Subsite::currentSubsite()) {
            if ($theme = $subsite->Theme) {
-                SSViewer::set_theme($theme);
+                SSViewer::set_themes([$theme, SSViewer::DEFAULT_THEME]);


This is an interesting situation, where in SS3 we would explicitly set the theme to a subsite's theme, but in SS4 we can use cascading/partial themes, so maybe we should just add_themes instead of setting it to subsite theme + default. What do you think @Cheddam?

How about moving this into getThemes() in the Subsite class, and user code can customise / modify this with an extension.

E.g.

public function getThemes() { $themes = [ $this->Theme, SSViewer::DEFAULT_THEME, ]; // Allow usercode to customise themes for this subsite $this->extend('updateThemes', $themes); return $themes; } // ... SSViewer::set_themes($subsite->getThemes());

robbieaverill · 2017-08-29T00:53:06Z

tests/php/SubsitesVirtualPageTest.php

@@ -136,24 +157,26 @@ public function testSubsiteVirtualPagesArentInappropriatelyPublished()
        $this->assertFalse($vp->IsModifiedOnStage);
    }

-	/**
+    /**


Above, switch doPublish for publishRecursive or publishSingle

robbieaverill · 2017-08-29T00:53:20Z

tests/php/SubsitesVirtualPageTest.php

    {
-        Config::inst()->update('StaticPublisher', 'disable_realtime', true);
+        Config::modify()->set('StaticPublisher', 'disable_realtime', true);


probably needs a todo for static publisher namespacing

robbieaverill · 2017-08-29T00:54:29Z

tests/php/WildcardDomainFieldTest.php

@@ -0,0 +1,87 @@
+<?php


tests/php/SubsitesVirtualPageTest.yml will need to have namespacing added to it

robbieaverill · 2017-08-29T00:54:43Z

tests/php/WildcardDomainFieldTest.php

+     * Check that valid domains are accepted
+     *
+     * @dataProvider validDomains
+     * @param $domain


missing type

robbieaverill · 2017-08-29T00:55:22Z

tests/php/WildcardDomainFieldTest.php

+    {
+        $field = new WildcardDomainField('DomainField');
+        $this->assertTrue($field->checkHostname($domain), "Validate that {$domain} is a valid domain wildcard");
+    }


We should just combine these three tests into one - testDomains with a dataprovider that also provides the expectation and message

tractorcow · 2017-08-29T01:57:06Z

code/admin/SubsiteAdmin.php

@@ -0,0 +1,42 @@
+<?php
+
+namespace SilverStripe\Subsites\Admin;


👍 for this, you can address this using git mv to ensure the case sensitivity is emitted to the log.

I'd not even mind if you renamed 'code' to 'src' (new standard) as a means of addressing this, but the important thing is that it's PSR-4. :)

tractorcow · 2017-08-29T01:57:32Z

code/admin/SubsiteAdmin.php

+/**
+ * Admin interface to manage and create {@link Subsite} instances.
+ *
+ * @package subsites


With namespaces, we prefer to dump all @package and @subpackage tags.

tractorcow · 2017-08-29T01:58:29Z

code/admin/SubsiteAdmin.php

+    {
+        $form = parent::getEditForm($id, $fields);
+
+        $grid = $form->Fields()->dataFieldByName(Subsite::class);


This is the fault of the upgrader;

You can put /** @skipUpgrade */ on either the line before this statement, or in the getEditForm PHPDoc to suppress the upgrader making this subsitution.

tractorcow · 2017-08-29T01:59:29Z

code/controller/SubsiteXHRController.php

     * Relax the access permissions, so anyone who has access to any CMS subsite can access this controller.
+     * @param null $member


👍 to this.

tractorcow · 2017-08-29T02:02:01Z

code/controller/SubsiteXHRController.php

@@ -51,6 +67,7 @@ public function getResponseNegotiator()
     */
    public function SubsiteList()
    {
-        return $this->renderWith('SubsiteList');
+        return $this->renderWith('Includes/SubsiteList');


Please use [ 'type' => 'Includes', 'SubsiteList'] instead of baking the variant into the path.

I would also suggest you consider namespacing templates. E.g. templates\SilverStripe\Subsites\Controller\Includes\SubsiteXHRController_subsitelist.ss. Then you could refer to the template using the below:

return $this->renderWith(['type' => 'Includes', self::class . '_subsitelist']);

tractorcow · 2017-08-29T02:34:53Z

code/reports/SubsiteReportWrapper.php

    ///////////////////////////////////////////////////////////////////////////////////////////
    // Querying

+    /**
+     * @param arary $params
+     * @return void


We don't need to explicitly declare voids.

tractorcow · 2017-08-29T02:35:32Z

composer.json

-        "silverstripe/cms": "^4.0@dev"
+        "silverstripe/framework": "~4.0",
+        "silverstripe/cms": "~4.0",
+        "silverstripe/asset-admin": "~1.0.0"
    },
    "require-dev": {
        "phpunit/PHPUnit": "~4.8@stable"


Core phpunit has been upgraded to ^5.7 please. :)

tractorcow · 2017-08-29T02:36:03Z

composer.json

-        "silverstripe/cms": "^4.0@dev"
+        "silverstripe/framework": "~4.0",
+        "silverstripe/cms": "~4.0",
+        "silverstripe/asset-admin": "~1.0.0"


We prefer semver ^1.0 vs ~1.0.0. Note that ^1.0 and ~1.0 are equivalent, but the former is preferred.

tractorcow · 2017-08-29T02:36:32Z

composer.json

-        "silverstripe/cms": "^4.0@dev"
+        "silverstripe/framework": "~4.0",
+        "silverstripe/cms": "~4.0",
+        "silverstripe/asset-admin": "~1.0.0"
    },
    "require-dev": {
        "phpunit/PHPUnit": "~4.8@stable"
    },


It turns out ^4@dev doesn't actually have any effect; ^4 is fine.

tractorcow · 2017-08-29T02:37:06Z

composer.json

    "extra": {
        "branch-alias": {
            "dev-master": "2.0.x-dev"
        }
-    },
-    "license": "BSD-3-Clause"


Why was license removed?

It's duplicated, towards the top of the file

tractorcow · 2017-08-29T03:33:07Z

@Cheddam I can merge, but before then, could you please get a .travis.yml minimal fix that will at least start the tests running? That way I can get a better picture of what's left to fix subsequent to this PR.

PHP needs to be changed from 5.5 to 5.6 and/or 7.x, and don't use travis support module anymore.

tractorcow · 2017-08-29T04:46:53Z

I'll just merge this so we can start PRing to get this faster.

wernerkrauss and others added 30 commits May 24, 2017 12:03

fix circular dependency error

5bb7182

upgrader: namespacing classes

2c84e62

upgrader: upgrade code

e33a5b4

remove VirtualPageController usage

2c3b5bf

move config to yml

ff6d28b

adding .upgrade.yml

80885a7

adding legacy.yml for remapping DataObjects

8aa6512

fixing double use declarations of Subsite class

fe6d93e

adding table names config

b71e544

config: fix excluded report

f814534

db query fixes

a7ef647

fix removed i18n::get_common_locales()

a49189e

replace config::inst()->update() with config::modify->set()

ef602ab

upgrade tests

2295501

reformat code and tests

c5f507b

adjusting test fixtures yml file

d1e8296

merge ss4 fixes by cheddam

9862cf5

merge #2

b8f9832

Updating editorconfig

8586749

(cherry picked from commit 2f99e51)

updating spacing and adding autoload key

d8e72f3

(cherry picked from commit e201462)

- namespacing all classes

5d3af16

- moving all phpunit tests into tests/php - moving all extensions from _config.php into config.yml and removing obsolete _config.php - moving GridFieldSubsiteDetailForm_ItemRequest into own file (cherry picked from commit ee02828)

fixing tests: adjusting yml file, update Config::modify usage

e2bdd5c

fixing FileSubsitesTest

492f437

call extension directly in test, as Versioned now is also applied to File and has this method

Get some more tests working

997459c

Updating config and i18n calls

fix SiteConfigSubsites AugmentSQL: get the right table name to filter

f348f5f

fix SubsiteAdminTest

709cbfa

SubsiteVirtualPage: SubsiteID should not be a virtual field

6f1e1ab

marking some tests as skipped that need more refactoring

212e479

fixing a namespaced ORM call

e03e7d9

fixing a PHP7 only statement to be 5.6 compatible

e86cc55

wernerkrauss and others added 21 commits June 1, 2017 15:32

fixing code analysis: removing unneeded else statements

17010f3

fixing code analysis: updating phpdoc

25754e1

fixing code analysis: phpunit: use assertInstanceOf

849c006

fixing code analysis: fix parameter in method call

9d3c450

fixing code analysis: use static::assertContains

fea1684

fixing code analysis: simplify if-return statements

9673c88

fixing code analysis: simplify if statements

55c7240

formatting code

a4a1ab6

Adapt SubsiteXHRController to LeftAndMain API changes.

21a8c56

Adapt to Permission::reset, DataObject::duplicate API changes.

49fbfcb

Fix misc. Subsite docblocks, remove DataObject::get_by_id call.

c248436

Fix Duplicate to Subsite functionality.

1975861

Add Subsite theme as main theme; allow cascading of themes

877f4f5

Updating README

2b7c3d1

Merge branch 'fix-ss-4-alpha-7' into fix-ss-4-master

32c45d0

Shift LeftAndMain_Menu template to force override.

438836b

Bring session / validation calls in line with SS4b1 API, replace icon.

5ec1c94

Merge branch 'master' into fix-ss-4-master

9a289a2

Add non-blank default to ThemeFieldEmptyString translation.

af6f499

Fix Subsite picker UI, reformat CSS.

ddefef1

Migrate excluded report to YAML config.

8be959f

robbieaverill added this to the 2.0.0 milestone Aug 28, 2017

robbieaverill added affects/v4 type/api-break labels Aug 28, 2017

robbieaverill suggested changes Aug 29, 2017

View reviewed changes

tractorcow suggested changes Aug 29, 2017

View reviewed changes

robbieaverill mentioned this pull request Aug 29, 2017

SS4 compatibility tracker - Subsites 2.0 #290

Closed

12 tasks

tractorcow merged commit 60725e7 into silverstripe:master Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SilverStripe 4 Compatibility #289

SilverStripe 4 Compatibility #289

Cheddam commented Aug 28, 2017

robbieaverill left a comment

robbieaverill Aug 28, 2017

robbieaverill Aug 28, 2017

tractorcow Aug 29, 2017

robbieaverill Aug 29, 2017 •

edited

Loading

robbieaverill Aug 28, 2017

tractorcow Aug 29, 2017

robbieaverill Aug 28, 2017

tractorcow Aug 29, 2017

robbieaverill Aug 28, 2017

tractorcow Aug 29, 2017

robbieaverill Aug 29, 2017

robbieaverill Aug 29, 2017

robbieaverill Aug 29, 2017

robbieaverill Aug 29, 2017

robbieaverill Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow Aug 29, 2017

robbieaverill Aug 29, 2017

tractorcow Aug 29, 2017

tractorcow commented Aug 29, 2017

tractorcow commented Aug 29, 2017

		* Relax the access permissions, so anyone who has access to any CMS subsite can access this controller.
		* @param null $member

SilverStripe 4 Compatibility #289

SilverStripe 4 Compatibility #289

Conversation

Cheddam commented Aug 28, 2017

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Aug 29, 2017

tractorcow commented Aug 29, 2017

robbieaverill Aug 29, 2017 •

edited

Loading