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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

This one a bit depends on #1798296: Integrate CSRF link token directly into routing system but I see no way for now get entity and token same time

rgristroph’s picture

Title: Convert comment_approve() to a Controller » Initial attempt
FileSize
4.1 KB

This is a first cut at this.

Perhaps someone could comment on the access related check and exception in the CommentController::commentApprovalPage(). Should that be in some sort of separate access method ?

--Rob

oenie’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,54 @@
+    $token = drupal_container()->get('request')->query->get('token');

Request will be the first parameter for your method. Replace the drupal_container call.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,54 @@
+      throw new AccessDeniedHttpException();

Add an @throws in the doxygen for the method with info on when it's thrown.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,54 @@
+    drupal_goto('node/' . $comment->nid->target_id);

Replace this by returning a RedirectResponse object. (Don't forget to specify it in the doxygen of the function)

oenie’s picture

Title: Initial attempt » Convert comment_approve() to a Controller
Status: Active » Needs work

Restoring the original title.

rgristroph’s picture

Status: Needs work » Needs review
FileSize
4.34 KB

I think this should address the issues from oenie, except that the first one, I'm not getting the request object from the method call, I get it from \Drupal::request() . Is that right ? Should I change the method to take the request as an argument instead ?

dawehner’s picture

Thank you!

+++ b/core/modules/comment/comment.moduleundefined
@@ -239,12 +239,7 @@ function comment_menu() {
-    'weight' => 10,

we should better not remove the weight of the menu link.

+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -5,3 +5,12 @@ comment_edit_page:
+       ¶
+          ¶

Let's not put empty lines/whitespace in here.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,59 @@
+   * ¶
...
+    return new RedirectResponse(url('node/' . $comment->nid->target_id, array('absolute' => TRUE)));    ¶

Unneeded whitespace here.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,59 @@
+    $token = \Drupal::request()->query->get('token');

You can just put the Request $request in the method and it will automatically passed in, so you don't have to get via \Drupal::request, see #1987816: Convert system_batch_page() to a new style controller

rgristroph’s picture

I think I have addressed all of those points with this one.

Does the devel coder review module work for d8 ? I will investigate that so I can check my own code and avoid the whitespace and syntax issues.

--Rob

oenie’s picture

Status: Needs review » Needs work

Almost there ...

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,60 @@
+  function commentApprovePage(Request $request, Comment $comment) {

Your parameter Comment needs a use Drupal\comment\Plugin\Core\Entity\Comment; at the top of the file. Otherwise it will use a wrong type of Comment object.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,60 @@
+   * Page callback: Publishes the specified comment.

While we're changing stuff, this isn't really a 'Page callback:' anymore, so you you can drop that from the documentation.

mparker17’s picture

Assigned: Unassigned » mparker17

I'll help!

mparker17’s picture

Applied changes specified in #8.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review

I'll need to unassign it and mark it "needs review". :S

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

mparker17’s picture

Status: Needs work » Needs review

Nope! Just because my improperly-named interdiff failed testing doesn't mean that this isn't ready for someone else to review.

oenie’s picture

Status: Needs review » Needs work

@mparker17: whrn you add an interdiff, be sure to give it anything but .patch as extension.
Otherwise (what happened now) testbot will test it, and it will always fail.

mparker17’s picture

Status: Needs work » Needs review

@oenie, I was aware that I should name the file "interdiff.txt", I just made a mistake.

The real patch will need to be tested and reviewed, however.

rgristroph’s picture

I looked over this, and also tested it functionally making comments as anonymous and approving them as admin, and it looks ready to me.

--Rob

oenie’s picture

oenie’s picture

Added a missing use clause. Should fix the last problem.

andypost’s picture

+++ b/core/modules/comment/comment.pages.incundefined
@@ -99,26 +99,3 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
-  drupal_set_message(t('Comment approved.'));
-  drupal_goto('node/' . $comment->nid->target_id);

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,62 @@
+    return new RedirectResponse(url('node/' . $comment->nid->target_id, array('absolute' => TRUE)));

drupal_set_message() is lost?

rgristroph’s picture

I think I accidently removed the drupal_set_message(), I had some custom stuff in there when I first rolled this to make sure it was the new router that was being called not the old callback, and I probably thought it was a debug message and removed it. Putting it back . . .

--Rob

oenie’s picture

@andypost: Good catch, missed that the first reviews.
Rerolling ...

rgristroph’s picture

Yeah, I reviewed / tested this again and everything is working, looks good, and the message is back.

--Rob

andypost’s picture

RTBC +1
@rgristroph thanx, after patch will green feel free to rtbc

kgoel’s picture

Status: Needs review » Reviewed & tested by the community

I have tested and patch looks good to me.

andypost’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -5,3 +5,10 @@ comment_edit_page:
     requirements:
         _entity_access: comment.update
 
+comment_approve_page:
+    pattern: 'comment/{comment}/approve'
+    defaults:
+      _content: '\Drupal\comment\Controller\CommentController::commentApprovePage'
+      entity_type: 'comment'
+    requirements:
+      _entity_access: comment.approve

the indent should be 2 spaces

+++ b/core/modules/comment/comment.routing.ymlundefined
--- /dev/null
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,63 @@
+ * Contains \Drupal\comment\CommentController

doc block is wrong

andypost’s picture

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.54 KB
dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -5,3 +5,10 @@ comment_edit_page:
+    pattern: 'comment/{comment}/approve'
+    defaults:
+        _content: '\Drupal\comment\Controller\CommentController::commentApprovePage'
+        entity_type: 'comment'
+    requirements:
+        _entity_access: comment.approve

Still too many spaces, sorr y. (yes the rest of teh comment file is also wrong but please still do something same).

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

ok, one more try

pfrenssen’s picture

+    // @todo CSRF tokens are validated in page callbacks rather than access
+    //   callbacks, because access callbacks are also invoked during menu link
+    //   generation. Add token support to routing: http://drupal.org/node/755584.

The text of this @todo is no longer valid. It is no longer using page callbacks or access callbacks.

andypost’s picture

Status: Needs review » Needs work
andypost’s picture

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

#29: comment-approve-1978948-29.patch queued for re-testing.

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

The last submitted patch, comment-approve-1978948-29.patch, failed testing.

andypost’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,63 @@
+    //   generation. Add token support to routing: http://drupal.org/node/755584.

Should point to #1798296: Integrate CSRF link token directly into routing system

pcambra’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
1.48 KB

Here's re-roll and a try to improve the comment as for #30 & #34

Status: Needs review » Needs work

The last submitted patch, comment-approve-1978948-35.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
89.29 KB
706 bytes

This should be in much better shape now

andypost’s picture

@pcambra PLease re-roll patch without bits of #1981314: Drop procedural usage of fields in field module

pcambra’s picture

Ouch, interdiff is fine, here's the patch with the correct rebase, sorry.

kgoel’s picture

@pcambra - patch doesn't have updated @todo doc.

pcambra’s picture

@kgoel I do see the todo comment changed in #39, it might not be the best comment though, I'll wait for review of that.

andypost’s picture

Status: Needs review » Needs work

Should be fine except

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,65 @@
+   * @param \Drupal\comment\Plugin\Core\Entity\Comment $comment
...
+  function commentApprove(Request $request, Comment $comment) {

CommentInterface

pcambra’s picture

Status: Needs work » Needs review
FileSize
4.82 KB
595 bytes

Addressed #42

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,65 @@
+   * Publishes the specified comment.
...
+   * @param \Drupal\comment\Plugin\Core\Entity\Comment $comment
...
+   *
+   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse.
+   */
+  function commentApprove(Request $request, Comment $comment) {
+  }
+
+}

I think andypost talked about this piece of code here.

pcambra’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
1.35 KB

Oops sorry, fixed.

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

The last submitted patch, comment-approve-1978948-45.patch, failed testing.

dawehner’s picture

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

#45: comment-approve-1978948-45.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -1,7 +1,14 @@
+  pattern: 'comment/{comment}/edit'
...
+  pattern: 'comment/{comment}/approve'

These should start with /

+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -1,7 +1,14 @@
+    _entity_form: comment.default
...
+    _entity_access: comment.update
...
+    _entity_access: comment.approve

In most places we've been putting single quotes around these, let's do that here and be consistent

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,66 @@
+use Drupal\Core\Controller\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
...
+class CommentController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a CommentController object.
+   */
+  public function __construct() {

Please swap the order, __construct should be the first method

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,66 @@
+  function commentApprove(Request $request, CommentInterface $comment) {

public function

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,66 @@
+    // @todo CRSF tokens are validated in the content controller until it gets
+    // moved to the access layer:
+    // Integrate CSRF link token directly into routing system:
+    // https://drupal.org/node/1798296.

This should be rewrapped, the following lines of an @todo are indented 2 extra spaces. See the original comment in the removed function.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,66 @@
+    return new RedirectResponse(url('node/' . $comment->nid->target_id, array('absolute' => TRUE)));

Crell point out in IRC that we now have $comment->permalink(), and it might make sense to use that here instead of hardcoding 'node/' again

pcambra’s picture

I think I've addressed Tim's comment in #48, including using permalink method instead hardcoding node/ path

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -0,0 +1,67 @@
+    drupal_set_message(t('Comment approved.'));
+    $permalink_uri = $comment->permalink();
+    return new RedirectResponse($permalink_uri['path'], array('absolute' => TRUE));
+  }
+
+}

You are losing $permalink_uri['options'] here.

What you need to do is pass the generator into this controller as a dependency, then:

$permalink_uri = $comment->permalink();
$permalink_uri['options']['absolute'] = TRUE;
$url = $this->generator->generateFromPath($permalink_uri['path'], $permalink_uri['options']);
return new RedirectResponse($url);

It's a bit verbose. Sorry. :-(

pcambra’s picture

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

Thanks for the input @Crell, here's a version of the patch addressing #49 *I think* :).

andypost’s picture

Status: Needs review » Needs work

A bit more

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -0,0 +1,80 @@
+  public static function create(ContainerInterface $container) {
+    return new static(\Drupal::urlGenerator());
return new static($container->get('url_generator'));
pcambra’s picture

Status: Needs work » Needs review
FileSize
5.46 KB
655 bytes

Changed that, thanks!

I'm wondering if arguments: ['@url_generator'] should be added to the routing.yml file or that's just for services.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!
suppose @argument only for services

Crell’s picture

Yes, the routing file doesn't do @arguments. That's specific to the container.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e49b129 and pushed to 8.x. Thanks!

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