Problem/Motivation

The Drupal 8 version of Ajax Comments needs ajax reply functionality enabled.

Proposed resolution

The attached patch file adds reply functionality and accompanying automated tests (plus cleans up some of the existing tests).

Remaining tasks

Needs community review.

User interface changes

Clicking on the 'reply' links for Ajax Comments-enabled comment fields results in comment reply form being ajax-loaded into the page.

API changes

Some changes to the existing routes for semantic consistency, and some minor new functionality in the TempStore helper class.

Data model changes

None.

CommentFileSizeAuthor
#7 interdiff.txt1.98 KBdanmuzyka
#7 ajax_comments-enable_ajax_comments_reply-2776977-7.patch43.57 KBdanmuzyka
#4 interdiff.txt349 bytesdanmuzyka
#4 ajax_comments-enable_ajax_comments_reply-2776977-4.patch43.04 KBdanmuzyka
ajax_comments-enable_ajax_comments_reply.patch43.08 KBdanmuzyka

Comments

danmuzyka created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, ajax_comments-enable_ajax_comments_reply.patch, failed testing.

danmuzyka’s picture

danmuzyka commented
Status: Needs work » Needs review

Test failure was a fluke. This is ready for community review.

danmuzyka’s picture

danmuzyka commented
StatusFileSize
new ajax_comments-enable_ajax_comments_reply-2776977-4.patch43.04 KB
new interdiff.txt349 bytes

Minor change to routes.

mroycroft’s picture

mroycroft commented

Pulled in the patch and tested it, reply feature works great. Changes look good to me.

kevin.dutra’s picture

kevin.dutra commented
Status: Needs review » Needs work

Overall things look really good, just a couple minor things that I noticed.

  1. +++ b/src/Controller/AjaxCommentsController.php
@@ -526,6 +581,133 @@ class AjaxCommentsController extends ControllerBase {
+      // to persist for the update() method, where the form returned

    Nitpick: Didn't update() get renamed to save()? 

  2. +++ b/src/Tests/AjaxCommentsTest.php
@@ -57,15 +58,18 @@ class AjaxCommentsTest extends CommentTestBase {
+    $this->node = Node::load($this->node->id());

    Technically, I think that the static entity loading methods are discouraged from within OO code. (https://www.drupal.org/node/2720343) I'm not sure it's worth doing a huge cleanup for this review though, especially since it's happening within a test. 

  3. +++ b/src/Tests/AjaxCommentsTest.php
@@ -231,6 +238,66 @@ class AjaxCommentsTest extends CommentTestBase {
+    // Path.

    Nitpick: Need one more indent.

danmuzyka’s picture

danmuzyka commented
Status: Needs work » Needs review
StatusFileSize
new ajax_comments-enable_ajax_comments_reply-2776977-7.patch43.57 KB
new interdiff.txt1.98 KB

I've attached an updated patch to address #1 and #3.

Per #2, I currently see at least 64 test classes in core that call Node::load(), specifically (I searched using: find core/ -name \*Test\.php | xargs grep "Node::load").

I'm also not seeing any specific policies about using the static load methods in the coding standards documentation at https://www.drupal.org/coding-standards and https://www.drupal.org/node/608152 , so at this point I agree that it doesn't seem worth refactoring the tests to avoid use of Node::load().

kevin.dutra’s picture

kevin.dutra commented
Status: Needs review » Reviewed & tested by the community

Cool beans. Between what @mroycroft and I have covered, I'd say this is RTBC.

  • danmuzyka authored cdfe5d3 on 8.x-1.x 
    Issue #2776977 by danmuzyka, kevin.dutra, mroycroft: Enable ajax comment...
danmuzyka’s picture

danmuzyka commented
Status: Reviewed & tested by the community » Fixed

Great! I've committed the patch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.