SilverStripe 4 compatibility #195

robbieaverill · 2017-01-17T03:48:15Z

Aside from the expected changes such as namespacing, there are some other changes made in this pull request to ensure the module works with the SS4 framework.

API changes

CommentList is removed and replaced with a polymorphic has_one relationship in the CommentsExtension
The default route is changed from /CommentingController/ to /comments/
RSS feed URLs include the class name which is not fully qualified. Until the framework has a more generic method for encoding and decoding this, CommentingController::encodeClassName and decodeClassName are added to swap \ for - to ensure they work
The BaseClass property of Comment is removed as it was conflicting with the concrete method DataObject::baseClass. Since SilverStripe 4.0, the faux poly-has-one CommentList is no longer required, so this property is removed and replaced with the native ParentClass that comes with the has_one: Parent
- There is a migration task to both convert existing BaseClass values to FQ class names (since BaseClass != ClassName so not covered by DatabaseAdmin.classname_value_remapping, and move the value of the field to ParentClass (the new field). The numeric ParentID field has a different purpose semantically, but remains the same in terms of the public API.
- The migration task can be run via sake dev/tasks/MigrateCommentParentsTask

Deprecations

Comment::getParent is deprecated, use Comment::Parent (has_one relationship) instead

Test/build changes

The CommentingControllerTest class has been updated to not automatically follow redirects unless specified to. This is to ensure that our assertions are accurate reflections of the code wherever possible
Removed the "FIXME" null assertions. Whatever they were (ghosts!) they're gone now.
Travis builds PHP 5.5 to 7.1 now

Questions

Is there a replacement for including i18n.js for the frontend?
Moderating comments (from the frontend) that are not instances of SiteTree does not with (Link() does not exist) - existing issue
@colymba's module still needs more work than I've done in terms of styling and admin grid functionality. Some of it is replaced with OOB functionality in SS4. I've applied a few bootstrap styles to some of it. How far do we go with it for this module, considering that it's blocking SilverStripe 4 compatibility silverstripe-blog#421? (Despite not being a literal dependency of it)

Things I didn't do

I haven't bothered too much with the default presentation of the template (related: FIX Reintroduce lost changes to CompositeField_holder.ss after namespacing silverstripe-framework#6511 in alpha.4)

Blockers

SilverStripe 4 compatibility silverstripe-gridfield-bulk-editing-tools#136 (will prevent the builds from passing)

Resolves #194

…eadme

* Remove CommentList and replace with a polymorphic has_one relationship * Tweaks for unit tests. Add tests for encode/decodeClassName.

…s build

tractorcow · 2017-01-17T03:58:40Z

.travis.yml


 before_install:
  - pip install --user codecov

 env:
  global:
-    - DB=MYSQL CORE_RELEASE=3.1
+    - DB=MYSQL CORE_RELEASE=4


CORE_RELEASE needs to be a branch name (i.e. master) not 4.

tractorcow · 2017-01-17T03:59:53Z

_config.php

@@ -1,6 +1,6 @@
 <?php

-Deprecation::notification_version('2.0', 'comments');
+\SilverStripe\Dev\Deprecation::notification_version('2.0', 'comments');


Needs bumping to 3.0.

tractorcow · 2017-01-17T04:00:39Z

_config/routes.yml

-    'CommentingController//$Action/$ID/$OtherID': 'CommentingController'
-    'PageComments/$Action/$ID/$OtherID': 'CommentingController'
-    'PageComments_Controller/$Action/$ID/$OtherID': 'CommentingController'
+    CommentingController//$Action/$ID/$OtherID: SilverStripe\Comments\Controllers\CommentingController


This is a duplicate route to the comments above. I suggest moving this to a unified route.

tractorcow · 2017-01-17T04:01:03Z

_config/routes.yml

-    'PageComments_Controller/$Action/$ID/$OtherID': 'CommentingController'
+    CommentingController//$Action/$ID/$OtherID: SilverStripe\Comments\Controllers\CommentingController
+    PageComments/$Action/$ID/$OtherID: SilverStripe\Comments\Controllers\CommentingController
+    PageCommentsController/$Action/$ID/$OtherID: SilverStripe\Comments\Controllers\CommentingController


Do we need these two still?

tractorcow · 2017-01-17T04:01:59Z

src/Admin/CommentsGridFieldBulkAction.php

+/**
+ * @package comments
+ */
+class CommentsGridFieldBulkAction extends GridFieldBulkActionHandler


What's the value in this? Just use GridFieldBulkActionHandler.

tractorcow · 2017-01-17T04:05:20Z

src/Admin/CommentsGridFieldBulkAction/Handler.php

 {
-
+    /**
+     * {@inheritDoc}


{@inheritdoc} with nothing else is redundant; docs automatically inherit.

tractorcow · 2017-01-17T04:05:47Z

src/Admin/CommentsGridFieldConfig.php

+use SilverStripe\Forms\GridField\GridFieldDataColumns;
+
+/**
+ * @package comments


I don't think we're doing @package anymore. Namespaces are enough.

tractorcow · 2017-01-17T04:06:08Z

src/Commenting.php

@@ -66,7 +74,7 @@ public static function remove($class)
    public static function has_commenting($class)
    {
        Deprecation::notice('2.0', 'Using Commenting::has_commenting is deprecated. Please use the config API instead');


Anything 2.0 deprecated can be removed.

tractorcow · 2017-01-17T04:07:10Z

src/Extensions/CommentsExtension.php

@@ -414,7 +444,7 @@ public function getRssLink()
     */
    public function getCommentRSSLink()
    {
-        return Controller::join_links(Director::baseURL(), 'CommentingController/rss');


You can now do Director::absoluteLink('comments/rss') and it will automatically work with base url.

Updated - assumed you meant Director::absoluteURL

tractorcow · 2017-01-17T04:09:51Z

src/Tasks/MigrateCommentParentsTask.php

+    public function run($request)
+    {
+        // Set the class names to fully qualified class names first
+        $remapping = Config::inst()->get('SilverStripe\\ORM\\DatabaseAdmin', 'classname_value_remapping');


You need a check to see if BaseClass is a field on Comment; If it's not skip the upgrade.

… tweaks to class names. Use Director::absoluteURL.

robbieaverill · 2017-01-17T04:40:06Z

Thanks @tractorcow - I've updated with your comments and pushed

This namespace changes once BulkManager gets updated to ss4.

FIX namespacing for BulkManager

tractorcow · 2017-01-25T04:52:02Z

Tests don't pass due to issues upstream with the bulk editing tool dependency, but I think this should be merged, and we mark master as unstable for the time being.

robbieaverill added 3 commits January 16, 2017 18:10

SS4 compat: Update composer and YAML config, rename classes, update r…

c698632

…eadme

SS4 compatibility updates - ORM, Form, Controller changes

ab3f352

* Remove CommentList and replace with a polymorphic has_one relationship * Tweaks for unit tests. Add tests for encode/decodeClassName.

FIX Update upgrade mapping, add purifier back into composer and travi…

6aeab9f

…s build

tractorcow suggested changes Jan 17, 2017

View reviewed changes

Remove [email protected]. Update Migrate task to check table name. Minor…

7a92b09

… tweaks to class names. Use Director::absoluteURL.

andrewandante and others added 2 commits January 19, 2017 13:23

FIX namespacing for BulkManager

279d47f

This namespace changes once BulkManager gets updated to ss4.

Merge pull request #1 from andrewandante/patch-1

d4e5831

FIX namespacing for BulkManager

tractorcow approved these changes Jan 25, 2017

View reviewed changes

Fix incorrect namespace

978879e

tractorcow merged commit 19224d9 into silverstripe:master Jan 25, 2017

robbieaverill deleted the feature/ss4-compat branch January 25, 2017 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SilverStripe 4 compatibility #195

SilverStripe 4 compatibility #195

robbieaverill commented Jan 17, 2017 •

edited

Loading

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

robbieaverill Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

tractorcow Jan 17, 2017

robbieaverill Jan 17, 2017

tractorcow Jan 25, 2017

tractorcow Jan 17, 2017

robbieaverill commented Jan 17, 2017

tractorcow commented Jan 25, 2017

SilverStripe 4 compatibility #195

SilverStripe 4 compatibility #195

Conversation

robbieaverill commented Jan 17, 2017 • edited Loading

API changes

Deprecations

Test/build changes

Questions

Things I didn't do

Blockers

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

robbieaverill commented Jan 17, 2017

tractorcow commented Jan 25, 2017

robbieaverill commented Jan 17, 2017 •

edited

Loading