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
#53 interdiff.txt655 bytespcambra
#53 comment-approve-1978948-53.patch5.46 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,563 pass(es).
[ View ]
#51 interdiff.txt1.91 KBpcambra
#51 comment-approve-1978948-51.patch5.45 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,015 pass(es).
[ View ]
#49 comment-approve-1978948-49.patch4.93 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 56,724 pass(es).
[ View ]
#49 interdiff.txt2.94 KBpcambra
#45 interdiff.txt1.35 KBpcambra
#45 comment-approve-1978948-45.patch4.89 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,101 pass(es).
[ View ]
#43 interdiff.txt595 bytespcambra
#43 comment-approve-1978948-43.patch4.82 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,173 pass(es).
[ View ]
#39 comment-approve-1978948-39.patch4.81 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,101 pass(es).
[ View ]
#37 interdiff.txt706 bytespcambra
#37 comment-approve-1978948-37.patch89.29 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 56,395 pass(es).
[ View ]
#35 interdiff.txt1.48 KBpcambra
#35 comment-approve-1978948-35.patch4.8 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 57,746 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#29 comment-approve-1978948-29.patch4.73 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 57,925 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#27 comment-approve-1978948-27.patch4.54 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,758 pass(es).
[ View ]
#21 comment-1978948-10-convert-comment-approve-to-a-controller-3.patch4.52 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 55,730 pass(es).
[ View ]
#21 interdiff.comment.18.txt666 bytesoenie
#18 comment-1978948-10-convert-comment-approve-to-a-controller-2.patch4.47 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 56,036 pass(es).
[ View ]
#18 interdiff.comment.10.txt650 bytesoenie
#10 comment-1978948-10-convert-comment-approve-to-a-controller.patch4.41 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 55,820 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#10 interdiff.patch866 bytesmparker17
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 comment-1978948-7-convert-comment-approve-to-a-controller.patch4.37 KBrgristroph
Test request sent.
[ View ]
#5 1978948-5-convert-comment-approve-to-a-controller.patch4.34 KBrgristroph
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#2 1978948-2-convert-comment-approve-to-a-controller.patch4.1 KBrgristroph
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Comments

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

Title:Convert comment_approve() to a ControllerInitial attempt
StatusFileSize
new4.1 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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

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

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

Restoring the original title.

Status:Needs work» Needs review
StatusFileSize
new4.34 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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 ?

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

StatusFileSize
new4.37 KB
Test request sent.
[ View ]

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

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.

Assigned:Unassigned» mparker17

I'll help!

StatusFileSize
new866 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.41 KB
FAILED: [[SimpleTest]]: [MySQL] 55,820 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Applied changes specified in #8.

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.

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.

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.

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.

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

StatusFileSize
new650 bytes
new4.47 KB
PASSED: [[SimpleTest]]: [MySQL] 56,036 pass(es).
[ View ]

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

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

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

StatusFileSize
new666 bytes
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 55,730 pass(es).
[ View ]

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

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

--Rob

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

Status:Needs review» Reviewed & tested by the community

I have tested and patch looks good to me.

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

Status:Needs work» Needs review
StatusFileSize
new4.54 KB
PASSED: [[SimpleTest]]: [MySQL] 55,758 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,925 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

ok, one more try

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

Status:Needs review» Needs work

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.

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

Status:Needs work» Needs review
StatusFileSize
new4.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,746 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.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.

Status:Needs work» Needs review
StatusFileSize
new89.29 KB
PASSED: [[SimpleTest]]: [MySQL] 56,395 pass(es).
[ View ]
new706 bytes

This should be in much better shape now

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

StatusFileSize
new4.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,101 pass(es).
[ View ]

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

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

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

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

Status:Needs work» Needs review
StatusFileSize
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,173 pass(es).
[ View ]
new595 bytes

Addressed #42

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.

Status:Needs work» Needs review
StatusFileSize
new4.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,101 pass(es).
[ View ]
new1.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.

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

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

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

StatusFileSize
new2.94 KB
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,724 pass(es).
[ View ]

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

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:

<?php
$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. :-(

Status:Needs work» Needs review
StatusFileSize
new5.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,015 pass(es).
[ View ]
new1.91 KB

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

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

<?php
return new static($container->get('url_generator'));
?>

Status:Needs work» Needs review
StatusFileSize
new5.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,563 pass(es).
[ View ]
new655 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.

Status:Needs review» Reviewed & tested by the community

Great!
suppose @argument only for services

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

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.