Updated: Comment #103

Problem/Motivation

Moving comment reply to a new route controller

Proposed resolution

Conversion of routes to controllers and forms

Remaining tasks

None

User interface changes

Previously this registered a menu entry foe comment/reply/%node.
I don't see a use case to have a comment/reply/%node menu link, as it is dynamic anyway, so the menu link is being removed.

API changes

Comment Breadcrumb Builder is now a service.

#2061917: Add Test for CommentBreadcrumbBuilder

Original report by @shanethehat

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Meta issue: #1971384: [META] Convert page callbacks to controllers

Files: 
CommentFileSizeAuthor
#118 drupal8.comment-module.1978958-118.patch15.71 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,316 pass(es).
[ View ]
#118 interdiff.txt9.52 KBjibran
#115 drupal8.comment-module.1978958-115.patch15.97 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,472 pass(es).
[ View ]
#115 interdiff.txt750 bytesjibran
#113 drupal8.comment-module.1978958-113.patch16.03 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 57,260 pass(es), 320 fail(s), and 213 exception(s).
[ View ]
#112 drupal8.comment-module.1978958-112.patch13.96 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,353 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#110 drupal8.comment-module.1978958-110.patch15.94 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,406 pass(es).
[ View ]
#106 drupal8.comment-module.1978958-106.patch15.76 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.1978958-106.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#106 interdiff.txt892 bytesdisasm
#101 drupal8.comment-module.1978958-101.patch15.71 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,508 pass(es), 0 fail(s), and 930 exception(s).
[ View ]
#101 interdiff.txt1.06 KBdisasm
#97 drupal8.comment-module.1978958-97.patch15.66 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,611 pass(es).
[ View ]
#97 interdiff.txt664 bytesdisasm
#93 drupal8.comment-module.1978958-93.patch16.28 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,207 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#93 interdiff.txt1.41 KBdisasm
#90 1978958-90.patch15.54 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,260 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#90 interdiff.txt1.23 KBjibran
#87 1978958-87.patch15.57 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#87 interdiff.txt1.23 KBjibran
#85 1978958-route-comment-reply-85.patch14.79 KBwamilton
PASSED: [[SimpleTest]]: [MySQL] 58,118 pass(es).
[ View ]
#82 1978958-route-comment-reply-82.patch14.78 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 56,418 pass(es), 322 fail(s), and 214 exception(s).
[ View ]
#82 1978958-diff-79-82.txt1.11 KBvijaycs85
#79 1978958-79.patch14.78 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,517 pass(es).
[ View ]
#79 interdiff.txt1.88 KBjibran
#76 1978958-76.patch14.23 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,164 pass(es).
[ View ]
#76 interdiff.txt5.25 KBjibran
#74 1978958-74.patch13.16 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,148 pass(es).
[ View ]
#74 interdiff.txt1.97 KBjibran
#73 1978958-73.patch13.07 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,424 pass(es).
[ View ]
#67 1978958-67.patch13.04 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978958-67_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#67 interdiff-64-67.txt8.06 KBPancho
#64 interdiff-55-64.txt1.81 KBPancho
#64 1978958-64.patch14.21 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 57,176 pass(es).
[ View ]
#55 1978958-55.patch14.16 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#55 interdiff.txt1.91 KBjibran
#52 1978958-52.patch13.58 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,012 pass(es).
[ View ]
#52 interdiff.txt2.05 KBjibran
#44 interdiff.txt1.33 KBjibran
#43 1978958-43.patch12.37 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 56,988 pass(es).
[ View ]
#41 1978958-40.patch12.42 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/Controller/CommentController.php.
[ View ]
#37 1978958-37.patch12.13 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978958-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 interdiff.txt990 bytesjibran
#36 1978958-36.patch12.31 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#35 1978958-35.patch8.08 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#32 comment-1978958-32.patch12.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,159 pass(es).
[ View ]
#32 interdiff.txt777 bytesdawehner
#27 1978958-27.patch12.4 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,258 pass(es).
[ View ]
#27 interdiff.txt753 bytesjibran
#25 1978958-25.patch12.39 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,982 pass(es).
[ View ]
#25 interdiff.txt1.96 KBjibran
#23 1978958-22.patch12.1 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,968 pass(es).
[ View ]
#23 interdiff.txt3.82 KBjibran
#20 1978958-20.patch12.07 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 56,918 pass(es), 0 fail(s), and 25 exception(s).
[ View ]
#20 interdiff.txt4.03 KBjibran
#17 1978958-17.patch11.93 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 56,885 pass(es), 392 fail(s), and 80 exception(s).
[ View ]
#17 interdiff.txt6.81 KBjibran
#14 comment-reply-1978958-14.patch9.63 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 56,891 pass(es), 388 fail(s), and 80 exception(s).
[ View ]
#14 interdiff.txt1.23 KBkgoel
#12 comment-reply-1978958-12.patch9.59 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 56,740 pass(es), 392 fail(s), and 80 exception(s).
[ View ]
#12 interdiff.txt4.01 KBkgoel
#7 1978958_7.patch9.46 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 54,551 pass(es), 383 fail(s), and 90 exception(s).
[ View ]
#3 1978958-route-comment-reply-1.patch9.29 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,644 pass(es), 376 fail(s), and 90 exception(s).
[ View ]

Comments

Assigned:Unassigned» kgoel

Going to work on this.

Assigned:kgoel» Unassigned

Status:Active» Needs review
StatusFileSize
new9.29 KB
FAILED: [[SimpleTest]]: [MySQL] 55,644 pass(es), 376 fail(s), and 90 exception(s).
[ View ]

Issuing initial patch...

Status:Needs review» Needs work

The last submitted patch, 1978958-route-comment-reply-1.patch, failed testing.

+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -1,7 +1,13 @@
+      _controller: '\Drupal\comment\Controller\CommentController::getReply'

_content instead of _controller since it's returning a renderable array

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,104 @@
+            drupal_goto("node/$node->nid");

use a RedirectResponse instead.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,104 @@
+          drupal_goto("node/$node->nid");

RedirectResponse instead.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,104 @@
+        drupal_goto("node/$node->nid");

Use a RedirectResponse instead

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,104 @@
+        drupal_goto("node/$node->nid");

Use a RedirectResponse instead.

Assigned:Unassigned» cosmicdreams

taking this one over.

Status:Needs work» Needs review
StatusFileSize
new9.46 KB
FAILED: [[SimpleTest]]: [MySQL] 54,551 pass(es), 383 fail(s), and 90 exception(s).
[ View ]

Here's the corrections in action.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1978958_7.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

#7: 1978958_7.patch queued for re-testing.

+++ b/core/modules/comment/comment.moduleundefined
@@ -256,14 +256,6 @@ function comment_menu() {
-  $items['comment/reply/%node'] = array(
-    'title' => 'Add new comment',
-    'page callback' => 'comment_reply',
-    'page arguments' => array(2),
-    'access callback' => 'node_access',
-    'access arguments' => array('view', 2),
-    'file' => 'comment.pages.inc',
-  );

so hook_menu() is still important to keep as you need it in order to have menu links.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+ * Definition of Drupal\comment\Controller\CommentPageController.

Should be "Contains \"...

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+namespace Drupal\comment\Controller;
+use Symfony\Component\HttpFoundation\RedirectResponse;

There should be an empty line between the two ones.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+ * Base for controller for comment forms.

What about "Controller for comment pages"

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+   * @param $pid

Let's tell people that it's a number.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+   * @return array

The documentation should also document that it might be a redirect response instead.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+  function getReply(EntityInterface $node, $pid = NULL) {

Are you sure the $pid will be passed in like that? The parent comment id is loaded all the time if it exists, so what about make this a parameter so it get's converted by the parameter converter automatically?

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+    $op = isset($_POST['op']) ? $_POST['op'] : '';

You can replace $_POST by the $request variable injected into the method.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+        return RedirectResponse::create("node/$node->nid");
...
+              return RedirectResponse::create("node/$node->nid");
...
+            return RedirectResponse::create("node/$node->nid");
...
+          return RedirectResponse::create("node/$node->nid");
...
+        return RedirectResponse::create("node/$node->nid");
...
+        return RedirectResponse::create("node/$node->nid");

As it's an interface you should use $node->id() better.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+

Let's remove this empty line.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,106 @@
+  }

Let's put an empty line here.

Status:Needs review» Needs work

The last submitted patch, 1978958_7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.01 KB
new9.59 KB
FAILED: [[SimpleTest]]: [MySQL] 56,740 pass(es), 392 fail(s), and 80 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, comment-reply-1978958-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
new9.63 KB
FAILED: [[SimpleTest]]: [MySQL] 56,891 pass(es), 388 fail(s), and 80 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, comment-reply-1978958-14.patch, failed testing.

+++ b/core/modules/comment/comment.moduleundefined
@@ -256,14 +256,6 @@ function comment_menu() {
   );
-  $items['comment/reply/%node'] = array(
-    'title' => 'Add new comment',
-    'page callback' => 'comment_reply',
-    'page arguments' => array(2),
-    'access callback' => 'node_access',
-    'access arguments' => array('view', 2),
-    'file' => 'comment.pages.inc',
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+    $op = \Drupal::request()->request->get('op');

This could be $request directly.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+    $op = ($op ? $op : '');

No need for this check. $request->request->get('op', ''); does the same.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+        $build['comment_form'] = comment_add($node, $pid);

comment_add is basically using the entity manager, should we leverage that?

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+        return RedirectResponse::create("node/$node->id()");
...
+              return RedirectResponse::create("node/$node->id()");
...
+            return RedirectResponse::create("node/$node->id()");
...
+        return RedirectResponse::create("node/$node->id()");
...
+        return RedirectResponse::create("node/$node->id()");

This should be using url() with absolute => TRUE

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+

This empty line looks unneeded.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+          $comment = comment_load($pid);

comment_load could use an injected comment storage controller.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+        $build['comment_node'] = node_view($node);

node_view could be replaced by an injected node entity render controller.

Status:Needs work» Needs review
StatusFileSize
new6.81 KB
new11.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,885 pass(es), 392 fail(s), and 80 exception(s).
[ View ]

All Injections and then some.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,111 @@
+    drupal_set_breadcrumb(array(l(t('Home'), NULL), l($node->label(), 'node/' . $node->id())));

I also want to convert this to service but waiting for #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service change notice.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,159 @@
+   * @param $pid

i guess pid is a int ?

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,159 @@
+   * @return array
+   *   An associative array containing:
+   *   - An array for rendering the node or parent comment.
+   *     - comment_node: If the comment is a reply to the node.
+   *     - comment_parent: If the comment is a reply to another comment.
+   *   - comment_form: The comment form as a renderable array.

It also returns a redirect response.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentPageController.phpundefined
@@ -0,0 +1,159 @@
+  function getReply(NodeInterface $node, $pid, Request $request) {

Should be public.

Status:Needs review» Needs work

The last submitted patch, 1978958-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new12.07 KB
FAILED: [[SimpleTest]]: [MySQL] 56,918 pass(es), 0 fail(s), and 25 exception(s).
[ View ]

Fixed #18 and renamed controller file.

Issue tags:+WSCCI-conversion

The last submitted patch, 1978958-20.patch, failed testing.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1978958-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.82 KB
new12.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,968 pass(es).
[ View ]

This should be green.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -66,21 +68,21 @@ public static function create(ContainerInterface $container) {
+   * @return array|Symfony\Component\HttpFoundation\RedirectResponse
    *   An associative array containing:

There should be some description as well.

StatusFileSize
new1.96 KB
new12.39 KB
PASSED: [[SimpleTest]]: [MySQL] 57,982 pass(es).
[ View ]

Thanks @dawehner for the comment suggestions.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,168 @@
+  public static function create(ContainerInterface $container) {
+  return new static($container->get('plugin.manager.entity'), $container->get('url_generator'));
+  }

Needs proper indentation.

StatusFileSize
new753 bytes
new12.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,258 pass(es).
[ View ]

:)

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,168 @@
+    return new static($container->get('plugin.manager.entity'), $container->get('url_generator'));

Let's make a new line per service :(

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,168 @@
+    drupal_set_breadcrumb(array(l(t('Home'), NULL), l($node->label(), $uri['path'])));

The new breadcrumb handling is in, i guess this line is not right anymore.

So this is not really a blocker at the moment?

I think #29-1 is not a RTBC blocker. For #29-2 see issue in #28. So if there is no real point to address can I get RTBC here :).

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new777 bytes
new12.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,159 pass(es).
[ View ]

I just did it.

Thank you very much for RTBC and fixing the issue. :)

Status:Reviewed & tested by the community» Needs work

Since 1978948

git ac https://drupal.org/files/comment-1978958-32.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 12714  100 12714    0     0   6478      0  0:00:01  0:00:01 --:--:--  8288
error: patch failed: core/modules/comment/comment.pages.inc:11
error: core/modules/comment/comment.pages.inc: patch does not apply
error: patch failed: core/modules/comment/comment.routing.yml:1
error: core/modules/comment/comment.routing.yml: patch does not apply
error: core/modules/comment/lib/Drupal/comment/Controller/CommentController.php: already exists in index

Status:Needs work» Needs review
StatusFileSize
new8.08 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

reroll

StatusFileSize
new12.31 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Removed core/modules/comment/comment.pages.inc yay!!!

StatusFileSize
new990 bytes
new12.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978958-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

some minors

Status:Needs review» Reviewed & tested by the community

yay for .inc removal

Issue tags:-WSCCI-conversion

#37: 1978958-37.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, 1978958-37.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new12.42 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/Controller/CommentController.php.
[ View ]

reroll after permalink issue.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1978958-40.patch, failed testing.

Assigned:cosmicdreams» Unassigned
Status:Needs work» Needs review
StatusFileSize
new12.37 KB
PASSED: [[SimpleTest]]: [MySQL] 56,988 pass(es).
[ View ]

StatusFileSize
new1.33 KB

interdiff.

Status:Needs review» Reviewed & tested by the community

RTBC as per #38.

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

This will collide with #731724: Convert comment settings into a field to make them work with CMI and non-node entities but I don't believe we should postpone it on that basis alone

#43: 1978958-43.patch queued for re-testing.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -132,4 +145,114 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+    // Set the breadcrumb trail.
+    drupal_set_breadcrumb(array(l(t('Home'), NULL), l($node->label(), $uri['path'])));
+    $op = $request->request->get('op');

It feels wrong to implement an already deprecated api in a new patch.

@dawehner see #28. This is a simple conversion issue.

We have a comment breadcrumb builder in #731724: Convert comment settings into a field to make them work with CMI and non-node entities, moving and adding it here would reduce the size of that patch. Also double standards, adding it there was in scope, same should apply here imo </rant>

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.05 KB
new13.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,012 pass(es).
[ View ]

Here is the patch for @dawehner's feelings :D. @larowlan now you have to reroll #731724: Convert comment settings into a field to make them work with CMI and non-node entities because of your rant :D

Status:Needs review» Reviewed & tested by the community

Thanks jibran, believe it or not that is actually better :)
The patch over there (at 400kb) has suffered from lack of reviews since November. Making it smaller is a win in my book.
jibran++

Back to RTBC unless bot disagrees.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Indentation and missing quotes (_entity_access):

+++ b/core/modules/comment/comment.routing.yml
@@ -19,3 +19,11 @@ comment_permalink:
     _controller: '\Drupal\comment\Controller\CommentController::commentPermalink'
   requirements:
     _entity_access: 'comment.view'
+
+comment_reply:
+    pattern: 'comment/reply/{node}/{pid}'
+    defaults:
+      _content: '\Drupal\comment\Controller\CommentController::getReply'
+      pid : ~
+    requirements:
+      _entity_access: node.view

Also, the 'comment_reply' route isn't registered in comment_menu().
How could it pass the tests? Obviously comment replying is uncovered, so needs a test...

+   * @return array|Symfony\Component\HttpFoundation\RedirectResponse

Missing backslash, should be "@return array|\Symfony\Component\HttpFoundation\RedirectResponse"

Also not sure if we really should replace comment_add() with the bulky entityManager call.
@dawehner had asked about this in #16:

comment_add is basically using the entity manager, should we leverage that?

However, comment_add() is not even deprecated, and other conversion issues (such as #731724: Convert comment settings into a field to make them work with CMI and non-node entities) continue using it. So for now we IMHO should leave it in, be it just so it can be found when converting these altogether in a consistent (and maybe improved) way at a later point.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new14.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

1) Fixed.
2) Fixed. Test pending.
3) Fixed.
4) I made the change because @dawehner asked for it so I let him answer this.

Status:Needs review» Needs work

The last submitted patch, 1978958-55.patch, failed testing.

Status:Needs work» Needs review

+++ b/core/modules/comment/comment.moduleundefined
@@ -234,11 +234,7 @@ function comment_menu() {
   $items['comment/reply/%node'] = array(
     'title' => 'Add new comment',
-    'page callback' => 'comment_reply',
-    'page arguments' => array(2),
-    'access callback' => 'node_access',
-    'access arguments' => array('view', 2),
-    'file' => 'comment.pages.inc',
+    'route_name' => 'comment_reply',
   );

All what this lines are doing is to register a menu entry fro comment/reply/%node, the routing is already always covered my the new routing yml. To be honest I don't see a usecase to have a comment/reply/%node menu link, as it is dynamic anyway. Can't we drop it instead?

Status:Needs review» Needs work
Issue tags:-Needs tests, -WSCCI-conversion, -RTBC July 1

The last submitted patch, 1978958-55.patch, failed testing.

Status:Needs work» Needs review

#55: 1978958-55.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +WSCCI-conversion, +RTBC July 1

The last submitted patch, 1978958-55.patch, failed testing.

Issue tags:-Needs tests

Agreed, we don't need a menu item here.
And there are tests, plenty of them.

Issue tags:+Needs tests

Re #55:

+++ b/core/modules/comment/comment.routing.yml
@@ -19,3 +19,11 @@ comment_permalink:
     _controller: '\Drupal\comment\Controller\CommentController::commentPermalink'
   requirements:
     _entity_access: 'comment.view'
+
+comment_reply:
+    pattern: 'comment/reply/{node}/{pid}'
+    defaults:
+      _content: '\Drupal\comment\Controller\CommentController::getReply'
+      pid : ~
+    requirements:
+      _entity_access: node.view

I meant the indentation of 'pattern', 'defaults' and 'requirements' being too large, not of 'pid' too small. :)
Also, the extra space after 'pid' should go away.

Re #57: Yes, I think we should drop it.

Re #58/60: Why would the install fail twice? Locally, install succeeded.

Issue tags:-Needs tests

Sorry, crossposted.

Status:Needs work» Needs review
StatusFileSize
new14.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,176 pass(es).
[ View ]
new1.81 KB

Quicky tested the patch manually, and it seemed to work fine.

However, on 'comment/reply/1' (without pid), the title is "Comment permalink" now while it should be "Add new comment" just as with the given pid. This is probably because the 'comment_menu' entry didn't exactly match the route if the pid was ommitted, but as suggested by @dawehner and @larowlan, we're removing it anyway. So we need to manually set the title.

Rolled all other stuff in, except for the last point in #54 regarding comment_add(). That should be @dawehner's decision.

One more question: Is getReply() really the right, meaningful name for this?
Shouldn't it be either of getReplyForm(), or in line with the other ones in that controller, commentReply()?

Personally i think it is fine to remove the usage of such non-apiish helper functions while doing the conversions.

I like your suggest of your getReplyForm method name suggestion.

StatusFileSize
new8.06 KB
new13.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978958-67_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

OK, so I renamed the method to getReplyForm().

Also, the $request parameter should be preceding the optional $pid parameter, so it can be given a default of NULL. While technically the NULL is passed in by the router anyway, it's just more obvious what's happening.

While looking more closely at the code it became obvious that quite many lines are redundant. Also the many levels of indentations were quite hard to follow.
So I just reorganized the code a bit saving 20 lines of redundant functional code, and making the remaining code easier to grasp.

Now that it is easier to get an overview of the code, it becomes clear there is further room for improvement. However, I just reorganized/refactored the code - whatever would actually be changing code, should be covered by a followup.

Also rewrote most of the documentation, as the "we should do" style taken over from comment_reply() was wordy, slightly annoying and sometimes inconcise.

Tests remain passing, but another quick review would be fine.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion, -RTBC July 1

The last submitted patch, 1978958-67.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion, +RTBC July 1

#67: 1978958-67.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

+1 rtbc

#67: 1978958-67.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+WSCCI-conversion, +RTBC July 1

The last submitted patch, 1978958-67.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new13.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,424 pass(es).
[ View ]

Conflict in core/modules/comment/comment.pages.inc because of #2041287: Convert $node->nid to $node->id() and $node->isNew(). But we have already deleted that file. Here is a reroll.

StatusFileSize
new1.97 KB
new13.16 KB
PASSED: [[SimpleTest]]: [MySQL] 57,148 pass(es).
[ View ]

Replaced user_access with $account->hasPermission.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -132,4 +145,90 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+    drupal_set_title(t('Add new comment'));
...
+      drupal_set_message(t('You are not authorized to post comments.'), 'error');
...
+        drupal_set_message(t("This discussion is closed: you can't post new comments."), 'error');
...
+          drupal_set_message(t('You are not authorized to view comments.'), 'error');
...
+          drupal_set_message(t('The comment you are replying to does not exist.'), 'error');

The string translation service can injected too

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.25 KB
new14.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,164 pass(es).
[ View ]

Now with string translation service injection.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.phpundefined
@@ -0,0 +1,30 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function build(array $attributes) {
+    if (isset($attributes['_route']) && $attributes['_route'] == 'comment_reply') {
+      $node = $attributes['node'];
+      $uri = $node->uri();
+      $breadcrumb[] = l(t('Home'), NULL);
+      $breadcrumb[] = l($node->label(), $uri['path']);
+      return $breadcrumb;
+    }

No injection for t() here either. I didn't get a chance to review the breadcrumb patch before it was committed - is it not possible to use injected services in BreadcrumBuilder?

If not injection, this could at least use Drupal::

Each breadcrumb builder is a service (defined in x.services.yml) you can nominate the args to inject

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
new14.78 KB
PASSED: [[SimpleTest]]: [MySQL] 57,517 pass(es).
[ View ]

Fixed #77.

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.phpundefined
@@ -15,13 +16,30 @@
+   * @var \Drupal\Core\StringTranslation\TranslationManager
...
+   * @param \Drupal\Core\StringTranslation\TranslationManager $translation
...
+  public function __construct(TranslationManager $translation) {

Instead of call it the manager let's just use the TranslatorInterface

In general i am wondering whether someone actually reviewed the changes in comment controller and all the logic which got changes by pancho in #1978958-67: Convert comment_reply() to a Controller. Andypost's comment is not really clear about that (#1978958-70: Convert comment_reply() to a Controller). Personally I don't have enough idea about how it should exactly work.

+++ /dev/nullundefined
@@ -1,101 +0,0 @@
-        if ($comment->status->value == COMMENT_PUBLISHED) {
...
-        else {
-          drupal_set_message(t('The comment you are replying to does not exist.'), 'error');
-          return new RedirectResponse(url('node/' . $node->id(), array('absolute' => TRUE)));
-        }
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -132,4 +157,90 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+        if (($comment->status->value != COMMENT_PUBLISHED) || ($comment->nid->target_id != $node->id())) {
+          drupal_set_message($this->translation->translate('The comment you are replying to does not exist.'), 'error');
+          return new RedirectResponse($this->urlGenerator->generateFromPath($uri['path'], array('absolute' => TRUE)));
+        }

Mh, it seems odd that we have a message telling that an unpublished comment does actually not exist anymore. In general I would propose to use COMMENT_NOT_PUBLISHED because using "!" assumes internal value of the constant.

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.phpundefined
@@ -15,13 +16,30 @@
+   * @var \Drupal\Core\StringTranslation\TranslationManager
...
+   * @param \Drupal\Core\StringTranslation\TranslationManager $translation
...
+  public function __construct(TranslationManager $translation) {

Instead of call it the manager let's just use the TranslatorInterface

There is no TranslatorInterface see #2049709: TranslationManager::translate() should be on an interface. As @alexpott said in #1938888-50: Convert user_admin_permissions to a new-style Form object

I'm ignoring the injection of string translation until #2018411: Figure out a nice DX when working with injected translation lands and will continue this policy until that's resolved. We are going to have to second passes of all conversions due to drupal_set_message() and watchdog() anyway.

. So can we finalize how to move forward here.
We can use in-depth review of #70.

Issue tags:+LONDON_2013_JULY
StatusFileSize
new1.11 KB
new14.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,418 pass(es), 322 fail(s), and 214 exception(s).
[ View ]

Addressing 'COMMENT_NOT_PUBLISHED' part of #80 and other item in #80 addressed in #81

Status:Needs review» Needs work

The last submitted patch, 1978958-route-comment-reply-82.patch, failed testing.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -132,4 +157,90 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+    $account = $request->attributes->get('account');

This is now $account = $request->attributes->get('_account');

Status:Needs work» Needs review
StatusFileSize
new14.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,118 pass(es).
[ View ]

Ran a little bit of the test suite to verify suggestion from @jibran. His fix fixes the tests I ran, so I'll let the bot handle the rest.

@@ -232,14 +232,6 @@ function comment_menu() {
-    'title' => 'Add new comment',
@@ -19,3 +19,11 @@ comment_permalink:
+comment_reply:
+  pattern: 'comment/reply/{node}/{pid}'
+  defaults:

The title should now moved to _title according to https://drupal.org/node/2067859

StatusFileSize
new1.23 KB
new15.57 KB
FAILED: [[SimpleTest]]: [MySQL] 58,501 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1978958-87.patch, failed testing.

+++ b/core/modules/comment/comment.routing.yml
@@ -19,3 +19,12 @@ comment_permalink:
+    _title: 'Add new comment'
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -132,4 +169,90 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+    drupal_set_title($this->translation->translate('Add new comment'));
...
+    $build['comment_form'] = $this->entityManager->getForm($comment);

Just add a $build['#title'] = $this->translation->translate('Add new comment')

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
new15.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,260 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here we go.

Status:Needs review» Needs work

The last submitted patch, 1978958-90.patch, failed testing.

StatusFileSize
new1.41 KB
new16.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,207 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

From what I can tell, drupal_set_title can not override #title in a render array. I solved this by checking if the op is preview in the CommentController and setting the title there. Even if we did move this to _title in the route at some point, drupal_set_title in an entity form controller is pretty hacky.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978958-93.patch, failed testing.

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.php
@@ -0,0 +1,48 @@
+    if (isset($attributes['_route']) && $attributes['_route'] == 'comment_reply') {
+      $node = $attributes['node'];

Also needs check for 'node' in attributes

Status:Needs work» Needs review
StatusFileSize
new664 bytes
new15.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,611 pass(es).
[ View ]

Addressing #96. Also, adding drupal_set_title back into the FormController because it looks like it's dependent on it in other places.

Assigned:Unassigned» disasm

This RTBC for me but I have worked a lot on this patch let's wait for @andypost. According to #92 we have to add _title back to yml file.
@andypost $attributes['_route'] == 'comment_reply' has a default argument $node which will be always set in attributes.

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.php
@@ -0,0 +1,48 @@
+    if (isset($attributes['_route']) && $attributes['_route'] == 'comment_reply' && isset($attributes['node'])) {

It would be cool to use $attributes[RouteObjectInterface::ROUTE_NAME] instead.

StatusFileSize
new1.06 KB
new15.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,508 pass(es), 0 fail(s), and 930 exception(s).
[ View ]

Implementing request in #101.

+++ b/core/modules/comment/comment.module
@@ -232,14 +232,6 @@ function comment_menu() {
-  $items['comment/reply/%node'] = array(
-    'title' => 'Add new comment',
-    'page callback' => 'comment_reply',
-    'page arguments' => array(2),
-    'access callback' => 'node_access',
-    'access arguments' => array('view', 2),
-    'file' => 'comment.pages.inc',
-  );

I would love to see some comment in the issue summary why this can be removed as it is.

#2061917: Add Test for CommentBreadcrumbBuilder is postpone on this. @dawehner see #57 and #61.

@dawehner I paraphrased your comment in #57 issue summary template. Now we just need to get this committed!

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978958-101.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new892 bytes
new15.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.1978958-106.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

adding isset back in to stop exceptions.

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion, +RTBC July 1, +LONDON_2013_JULY

The last submitted patch, drupal8.comment-module.1978958-106.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new15.94 KB
PASSED: [[SimpleTest]]: [MySQL] 58,406 pass(es).
[ View ]

reroll!

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

StatusFileSize
new13.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,353 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

StatusFileSize
new16.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,260 pass(es), 320 fail(s), and 213 exception(s).
[ View ]

Sorry I messed up the reroll. Should be fine now.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.comment-module.1978958-113.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new750 bytes
new15.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,472 pass(es).
[ View ]

I can't believe it I have messed up re-roll twice :(

Status:Needs review» Reviewed & tested by the community

The rerole looked fine

Status:Reviewed & tested by the community» Needs work

Overall looks good. I notice comment/reply/%node is moving from a MENU_NORMAL_ITEM to a callback, but that's probably fine.

A couple of things:

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.php
@@ -0,0 +1,49 @@
+      $breadcrumb[] = l($this->translation->translate('Home'), NULL);
...
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -78,14 +115,14 @@ public function commentApprove(Request $request, CommentInterface $comment) {
+    drupal_set_message($this->translation->translate('Comment approved.'));

Should these not be $this->t()?

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -132,4 +169,93 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+    $account = $request->attributes->get('_account');

And that Drupal::currentUser()?

Status:Needs work» Needs review
StatusFileSize
new9.52 KB
new15.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,316 pass(es).
[ View ]

hmmm here comes the ControllerBase.

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Fixed

Committed a080a52 and pushed to 8.x. Thanks!

Yay!! it is committed to 8.x. Thank you all.

Thanx a lot! Now we can finish comment field conversion

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

Issue summary:View changes

Needs issue summary