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

CommentFileSizeAuthor
#118 drupal8.comment-module.1978958-118.patch15.71 KBjibran
#118 interdiff.txt9.52 KBjibran
#115 drupal8.comment-module.1978958-115.patch15.97 KBjibran
#115 interdiff.txt750 bytesjibran
#113 drupal8.comment-module.1978958-113.patch16.03 KBjibran
#112 drupal8.comment-module.1978958-112.patch13.96 KBjibran
#110 drupal8.comment-module.1978958-110.patch15.94 KBdisasm
#106 drupal8.comment-module.1978958-106.patch15.76 KBdisasm
#106 interdiff.txt892 bytesdisasm
#101 drupal8.comment-module.1978958-101.patch15.71 KBdisasm
#101 interdiff.txt1.06 KBdisasm
#97 drupal8.comment-module.1978958-97.patch15.66 KBdisasm
#97 interdiff.txt664 bytesdisasm
#93 drupal8.comment-module.1978958-93.patch16.28 KBdisasm
#93 interdiff.txt1.41 KBdisasm
#90 1978958-90.patch15.54 KBjibran
#90 interdiff.txt1.23 KBjibran
#87 1978958-87.patch15.57 KBjibran
#87 interdiff.txt1.23 KBjibran
#85 1978958-route-comment-reply-85.patch14.79 KBwamilton
#82 1978958-route-comment-reply-82.patch14.78 KBvijaycs85
#82 1978958-diff-79-82.txt1.11 KBvijaycs85
#79 1978958-79.patch14.78 KBjibran
#79 interdiff.txt1.88 KBjibran
#76 1978958-76.patch14.23 KBjibran
#76 interdiff.txt5.25 KBjibran
#74 1978958-74.patch13.16 KBjibran
#74 interdiff.txt1.97 KBjibran
#73 1978958-73.patch13.07 KBjibran
#67 1978958-67.patch13.04 KBPancho
#67 interdiff-64-67.txt8.06 KBPancho
#64 interdiff-55-64.txt1.81 KBPancho
#64 1978958-64.patch14.21 KBPancho
#55 1978958-55.patch14.16 KBjibran
#55 interdiff.txt1.91 KBjibran
#52 1978958-52.patch13.58 KBjibran
#52 interdiff.txt2.05 KBjibran
#44 interdiff.txt1.33 KBjibran
#43 1978958-43.patch12.37 KBjibran
#41 1978958-40.patch12.42 KBjibran
#37 1978958-37.patch12.13 KBjibran
#37 interdiff.txt990 bytesjibran
#36 1978958-36.patch12.31 KBjibran
#35 1978958-35.patch8.08 KBjibran
#32 comment-1978958-32.patch12.42 KBdawehner
#32 interdiff.txt777 bytesdawehner
#27 1978958-27.patch12.4 KBjibran
#27 interdiff.txt753 bytesjibran
#25 1978958-25.patch12.39 KBjibran
#25 interdiff.txt1.96 KBjibran
#23 1978958-22.patch12.1 KBjibran
#23 interdiff.txt3.82 KBjibran
#20 1978958-20.patch12.07 KBjibran
#20 interdiff.txt4.03 KBjibran
#17 1978958-17.patch11.93 KBjibran
#17 interdiff.txt6.81 KBjibran
#14 comment-reply-1978958-14.patch9.63 KBkgoel
#14 interdiff.txt1.23 KBkgoel
#12 comment-reply-1978958-12.patch9.59 KBkgoel
#12 interdiff.txt4.01 KBkgoel
#7 1978958_7.patch9.46 KBcosmicdreams
#3 1978958-route-comment-reply-1.patch9.29 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kgoel’s picture

Assigned: Unassigned » kgoel

Going to work on this.

kgoel’s picture

Assigned: kgoel » Unassigned
vijaycs85’s picture

Status: Active » Needs review
FileSize
9.29 KB

Issuing initial patch...

Status: Needs review » Needs work

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

cosmicdreams’s picture

+++ 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.

cosmicdreams’s picture

Assigned: Unassigned » cosmicdreams

taking this one over.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
9.46 KB

Here's the corrections in action.

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

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

dawehner’s picture

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

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

dawehner’s picture

+++ 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.

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.01 KB
9.59 KB

Status: Needs review » Needs work

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

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
9.63 KB

Status: Needs review » Needs work

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

dawehner’s picture

+++ 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.

jibran’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
11.93 KB

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.

dawehner’s picture

+++ 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.

jibran’s picture

Status: Needs work » Needs review
FileSize
4.03 KB
12.07 KB

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.82 KB
12.1 KB

This should be green.

dawehner’s picture

+++ 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.

jibran’s picture

FileSize
1.96 KB
12.39 KB

Thanks @dawehner for the comment suggestions.

dawehner’s picture

+++ 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.

jibran’s picture

FileSize
753 bytes
12.4 KB

:)

jibran’s picture

dawehner’s picture

+++ 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.

dawehner’s picture

So this is not really a blocker at the moment?

jibran’s picture

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 :).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
777 bytes
12.42 KB

I just did it.

jibran’s picture

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

alexpott’s picture

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
jibran’s picture

Status: Needs work » Needs review
FileSize
8.08 KB

reroll

jibran’s picture

FileSize
12.31 KB

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

jibran’s picture

FileSize
990 bytes
12.13 KB

some minors

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yay for .inc removal

jibran’s picture

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.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.42 KB

reroll after permalink issue.

Status: Reviewed & tested by the community » Needs work

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

jibran’s picture

Assigned: cosmicdreams » Unassigned
Status: Needs work » Needs review
FileSize
12.37 KB
jibran’s picture

FileSize
1.33 KB

interdiff.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #38.

YesCT’s picture

Issue tags: +RTBC July 1

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

larowlan’s picture

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

jibran’s picture

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

dawehner’s picture

+++ 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.

jibran’s picture

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

larowlan’s picture

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>

jibran’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.05 KB
13.58 KB

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

larowlan’s picture

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.

Pancho’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
14.16 KB

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.

dawehner’s picture

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.

jibran’s picture

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.

larowlan’s picture

Issue tags: -Needs tests

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

Pancho’s picture

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.

Pancho’s picture

Issue tags: -Needs tests

Sorry, crossposted.

Pancho’s picture

Status: Needs work » Needs review
FileSize
14.21 KB
1.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.

Pancho’s picture

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()?

dawehner’s picture

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.

Pancho’s picture

FileSize
8.06 KB
13.04 KB

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.

jibran’s picture

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

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1 rtbc

catch’s picture

Issue tags: -WSCCI-conversion, -RTBC July 1

#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.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.07 KB

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.

jibran’s picture

FileSize
1.97 KB
13.16 KB

Replaced user_access with $account->hasPermission.

alexpott’s picture

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

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.25 KB
14.23 KB

Now with string translation service injection.

catch’s picture

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::

larowlan’s picture

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

jibran’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
14.78 KB

Fixed #77.

dawehner’s picture

+++ 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

jibran’s picture

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.

vijaycs85’s picture

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.

jibran’s picture

+++ 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');

wamilton’s picture

Status: Needs work » Needs review
FileSize
14.79 KB

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.

andypost’s picture

@@ -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

jibran’s picture

Status: Needs review » Needs work

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

andypost’s picture

+++ 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')

jibran’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
15.54 KB

Here we go.

Status: Needs review » Needs work

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

larowlan’s picture

disasm’s picture

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.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

andypost’s picture

+++ 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

disasm’s picture

Status: Needs work » Needs review
FileSize
664 bytes
15.66 KB

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

disasm’s picture

Assigned: Unassigned » disasm
jibran’s picture

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.

dawehner’s picture

+++ 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.

disasm’s picture

Implementing request in #101.

dawehner’s picture

+++ 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.

jibran’s picture

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

disasm’s picture

@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.

disasm’s picture

Status: Needs work » Needs review
FileSize
892 bytes
15.76 KB

adding isset back in to stop exceptions.

jibran’s picture

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.

jibran’s picture

disasm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.94 KB

reroll!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

jibran’s picture

jibran’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
750 bytes
15.97 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The rerole looked fine

webchick’s picture

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()?

jibran’s picture

Status: Needs work » Needs review
FileSize
9.52 KB
15.71 KB

hmmm here comes the ControllerBase.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a080a52 and pushed to 8.x. Thanks!

jibran’s picture

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

andypost’s picture

Thanx a lot! Now we can finish comment field conversion

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

Anonymous’s picture

Issue summary: View changes

Needs issue summary