Part of #1971384: [META] Convert page callbacks to controllers

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

CommentFileSizeAuthor
#62 overlay-1978938-62.patch9.35 KBpfrenssen
#62 interdiff.txt2.14 KBpfrenssen
#58 overlay-1978938-58-test_only.patch4.6 KBpfrenssen
#58 overlay-1978938-58.patch9.17 KBpfrenssen
#58 interdiff.txt2.49 KBpfrenssen
#56 overlay-1978938-56.patch9.07 KBdawehner
#52 overlay-1978938-52.patch9.1 KBdawehner
#52 interdiff.txt893 bytesdawehner
#50 overlay-1978938-50.patch9.64 KBmrded
#50 interdiff-47-50.txt1.54 KBmrded
#47 overlay-1978938-47.patch8.71 KBLetharion
#43 overlay-1978938-43.patch8.71 KBLetharion
#40 overlay-1978938-40.patch8.68 KBdawehner
#40 interdiff.txt1.24 KBdawehner
#37 overlay-1978938-37.patch8.96 KBdawehner
#34 overlay-1978938-34.patch2.14 KBdawehner
#25 convert_overlay_user_dismiss-1978938-24.patch6.5 KBsomepal
#24 overlay-1978938-24.patch6.84 KBdawehner
#24 interdiff.txt2.91 KBdawehner
#20 drupal-convert_overlay_user_dismiss-1978938-20.patch6.51 KBdisasm
#20 interdiff.txt937 bytesdisasm
#18 drupal-convert_overlay_user_dismiss-1978938-18.patch6.61 KBpguillard
#16 drupal-convert_overlay_user_dismiss-1978938-16.patch6.51 KBdisasm
#16 interdiff.txt2.61 KBdisasm
#15 drupal-convert_overlay_user_dismiss-1978938-15.patch6.61 KBdisasm
#13 drupal-1978938-13.patch6.58 KBlaurentchardin
#10 overlay_user_message-1978938-10.patch6.59 KBsidharthap
#8 overlay_user_message-1978938-8.patch6.63 KBsidharthap
#5 overlay_user_message-1978938-5.patch6.87 KBsidharthap
#4 overlay_user_message-1978938-4.patch12.08 KBsidharthap
#1 page_callback_controllers-1978938-1.patch6.22 KBsidharthap
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sidharthap’s picture

Status: Active » Needs review
FileSize
6.22 KB

Here is the initial patch created. we may required to correct this thing if not works.

dawehner’s picture

+++ b/core/modules/overlay/lib/Drupal/overlay/Access/overlayDismissMessage.phpundefined
@@ -0,0 +1,43 @@
+ * Access check for menu link delete routes.

C&P error :)

+++ b/core/modules/overlay/lib/Drupal/overlay/Access/overlayDismissMessage.phpundefined
@@ -0,0 +1,43 @@
+class overlayDismissMessage implements AccessCheckInterface {

Should be probably more something like OverlayDismissMessageAccessCheck or something shorter?

+++ b/core/modules/overlay/lib/Drupal/overlay/Access/overlayDismissMessage.phpundefined
@@ -0,0 +1,43 @@
+    return array_key_exists('_access_overlay_user_dismiss_message', $route->getRequirements());

_access at the front seems to be redundant.

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,55 @@
+  /**
+   * Constructs a OverlayController object.
+   */
+  public function __construct() {

no need for the constructor if nothing is injected.

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,55 @@
+    ¶

+++ b/core/modules/overlay/overlay.routing.ymlundefined
@@ -0,0 +1,7 @@
+    ¶

some hidden spaces :) These should be removed according to http://drupal.org/node/1354

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

The request object should be injected.

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,55 @@
+    drupal_container()->get('user.data')->set('overlay', $user->uid, 'message_dismissed', 1);

the user data should be injected as well.

ParisLiakos’s picture

Status: Needs review » Needs work

hi:)
an addition to above

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,55 @@
+    drupal_goto('user/' . $user->uid . '/edit');

+++ b/core/modules/overlay/overlay.routing.ymlundefined
@@ -0,0 +1,7 @@
+    _content: '\Drupal\overlay\Controller\OverlayController::overlayMessage'

should be _controller instead of _content and new RedirectResponse instead of drupal_goto

sidharthap’s picture

Status: Needs work » Needs review
FileSize
12.08 KB
sidharthap’s picture

Corrected class name and removed other changes.

ParisLiakos’s picture

diff --git a/core/modules/overlay/lib/Drupal/overlay/Access/DismissMessageAccessCheck.php b/core/modules/overlay/lib/Drupal/overlay/Access/DismissMessageAccessCheck.php
new file mode 100755
...
diff --git a/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php
new file mode 100755
...
diff --git a/core/modules/overlay/overlay.module b/core/modules/overlay/overlay.module
old mode 100644
new mode 100755
...
diff --git a/core/modules/overlay/overlay.routing.yml b/core/modules/overlay/overlay.routing.yml
new file mode 100755
...
diff --git a/core/modules/overlay/overlay.services.yml b/core/modules/overlay/overlay.services.yml
old mode 100644
new mode 100755

files should be 0644

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,50 @@
+    return new RedirectResponse(url('user/' . $user->uid . '/edit'));

the url needs to be absolute for this to work
array('absolute' => TRUE)

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -36,8 +36,7 @@ function overlay_menu() {
   $items['overlay/dismiss-message'] = array(
     'title' => '',
-    'page callback' => 'overlay_user_dismiss_message',
-    'access callback' => 'overlay_user_dismiss_message_access',
+    'route_name' => 'overlay_message',
     'type' => MENU_CALLBACK,

you can totally remove the menu item now

+++ b/core/modules/overlay/overlay.routing.ymlundefined
@@ -0,0 +1,6 @@
+    _access_overlay_dismiss_message: 'TRUE'
\ No newline at end of file

+++ b/core/modules/overlay/overlay.services.ymlundefined
@@ -3,3 +3,8 @@ services:
+      - { name: access_check }
\ No newline at end of file

newline missing

dawehner’s picture

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,50 @@
+class OverlayController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}.
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();

As long we don't inject anything you don't have to implement the interface.

sidharthap’s picture

Status: Needs review » Needs work

The last submitted patch, overlay_user_message-1978938-8.patch, failed testing.

sidharthap’s picture

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

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,41 @@
+   * Dismisses the overlay accessibility message for this user..

No need for two dots here :)

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,41 @@
+   * @return string
+   *   A render array for a page containing a list of content.

It actually returns a redirect response ...

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,41 @@
+    // @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.

I guess we should update the comment to match the new code. Maybe check whether the new access system can deal with it already.

laurentchardin’s picture

Adding a link to http://drupal.org/node/1978932 since we are both introducing the same controller.

laurentchardin’s picture

FileSize
6.58 KB

Added dawehner's return + updated issue url for CSRF support in routing (not there yet) + added a todo about ?destination support here.

ParisLiakos’s picture

Status: Needs review » Needs work

Thanks for your patience. There are a few more nitpicks left:

+++ b/core/modules/overlay/lib/Drupal/overlay/Access/DismissMessageAccessCheck.phpundefined
@@ -0,0 +1,43 @@
+use Symfony\Component\HttpFoundation\Request;
+
+
+/**

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+
+
+/**

Lets remove the one newline

+++ b/core/modules/overlay/lib/Drupal/overlay/Access/DismissMessageAccessCheck.phpundefined
@@ -0,0 +1,43 @@
+  }
+}

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+  }
+}

Add a newline here inbetween

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+<?php
+/**

we need a newline inbetween here

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+    // @todo add ?destination support since this response will redirect to the user profile *always*

i believe this is not needed anymore. ?destination now works

disasm’s picture

Status: Needs work » Needs review
FileSize
6.61 KB

reroll!

disasm’s picture

Attached patch addresses Paris's comments above, and also cleans up some other coding standards.

ParisLiakos’s picture

thanks! but, found more:/

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+
+
+/**

nit! still need one less newline here:)

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+ * Controller routines for book routes.

D'oh i just saw the comment:) probably C&P err from book module

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,40 @@
+   * @return \\Symfony\Component\HttpFoundation\RedirectResponse

ah, two backslashes here, should be one

pguillard’s picture

ParisLiakos’s picture

hmm i think #18is based on #15 but #16 is a lot better

disasm’s picture

attached patch and interdiff fixes requested changes above.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks:)

YesCT’s picture

Issue tags: +RTBC July 1

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/overlay/lib/Drupal/overlay/Access/DismissMessageAccessCheck.phpundefined
@@ -0,0 +1,43 @@
+    global $user;

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,39 @@
+    global $user;

Deprecated :) use the request instead...

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
6.84 KB

Fix that and the access checker return values.

somepal’s picture

Status: Needs review » Needs work
FileSize
6.5 KB

considering #23, re-roll of #20

somepal’s picture

Status: Needs work » Needs review
alexpott’s picture

@somepal what's the difference between #24 and #25?

somepal’s picture

@alexpott while #25 contains

$GLOBALS['user']->uid

to get the current user, @dawehner in #24 prefers it should be as,

$account = $request->attributes->get('account');
$account->id()

Both the patches accidentally arrived at the same time. So #25 is not modification of #24 in any way.

alexpott’s picture

Issue tags: -WSCCI-conversion

No worries @somepal xposts happen... the usage of globals is now deprecated so the patch in #24 is correct whilst the patch in #25 is not actually changing anything :) see https://drupal.org/node/2032447

I consider the patch in #24 rtbc... but if I do that then a commit might take longer :)

somepal’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott ok then we are gtg with #24 :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed #24... 1021ebb and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

dawehner’s picture

Status: Closed (fixed) » Needs work
+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,37 @@
+    $request->attributes->get('user.data')->set('overlay', $GLOBALS['user']->uid, 'message_dismissed', 1);

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -220,56 +214,6 @@ function overlay_page_alter(&$page) {
-  Drupal::service('user.data')->set('overlay', $user->uid, 'message_dismissed', 1);

This line seriously can't work, sorry. I guess we also need testcoverage then.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -RTBC July 1 +Needs tests
FileSize
2.14 KB

still needs tests

Crell’s picture

Code looks fine; not sure what sort of tests we want to add here... (No more webtests, please...)

dawehner’s picture

To be honest I think that a unit test ensures less here ... We could have unit tested that code without actually knowing whether it works.

dawehner’s picture

Category: task » bug
Priority: Normal » Major
Issue tags: -Needs tests
FileSize
8.96 KB

Wrote some proper tests and injected quite a bunch of service.

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, overlay-1978938-37.patch, failed testing.

jibran’s picture

  1. +++ b/core/core.services.yml
    @@ -26,7 +26,8 @@ services:
    +      - { name: cache.bin
    +
    

    what?

  2. +++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php
    @@ -62,16 +101,14 @@ public function regionRender($region) {
         // @todo Integrate CSRF link token directly into routing system: http://drupal.org/node/1798296.
    

    80 char limit.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
8.68 KB

oh, no wonder that this fails.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

It is green. It has unit tests. It is major. Let's get this in.

webchick’s picture

Not sure why this is major, but I'd like to hold it for a couple of days in the hopes that #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface gets committed.

Letharion’s picture

FileSize
8.71 KB

Hoping that #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface does get in, I'm jumping the gun and updating the patch. Not setting back to needs review as the patch doesn't apply, yet.

So just to be really clear, deliberately not changing status, but this patch is not the one that has been RTBCd.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, overlay-1978938-43.patch, failed testing.

Letharion’s picture

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

Edit: Ops, didn't realize testbot was gonna ruin the status setting. Oh well, leaving as is now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, overlay-1978938-43.patch, failed testing.

Letharion’s picture

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

Status: Needs review » Reviewed & tested by the community

Let's get back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php
@@ -62,16 +101,15 @@ public function regionRender($region) {
+    return new RedirectResponse($this->urlGenerator()->generateFromPath('user/' . $this->account->id() . '/edit', array('absolute' => TRUE)));

This can use the new $this->redirect method that is on ControllerBase now that we're extending from it.

mrded’s picture

Assigned: Unassigned » mrded
Status: Needs work » Needs review
FileSize
1.54 KB
9.64 KB

Status: Needs review » Needs work

The last submitted patch, overlay-1978938-50.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
893 bytes
9.1 KB

Sorry to just fix that, but explaining would need the same amount of time.

Status: Needs review » Needs work
Issue tags: -LONDON_2013_AUGUST

The last submitted patch, overlay-1978938-52.patch, failed testing.

h3rj4n’s picture

Status: Needs work » Needs review

#52: overlay-1978938-52.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +LONDON_2013_AUGUST

The last submitted patch, overlay-1978938-52.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.07 KB

Reroll.

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: -LONDON_2013_AUGUST
+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -57,21 +95,20 @@ public function regionRender($region) {
    * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    *   Thrown when a non valid token was specified.
-   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   * @return \Drupal\Core\Controller\ControllerBase

Add a blank line before the @return.

+++ b/core/modules/overlay/tests/Drupal/overlay/Tests/Controller/OverlayControllerTest.phpundefined
@@ -0,0 +1,152 @@
+use Drupal\overlay\Controller\OverlayController;
+use Drupal\Tests\UnitTestCase;
+  use Symfony\Component\DependencyInjection\ContainerBuilder;
+  use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Request;
+

Indentation.

+++ b/core/modules/overlay/tests/Drupal/overlay/Tests/Controller/OverlayControllerTest.phpundefined
@@ -0,0 +1,152 @@
+    $this->overlayController = new OverlayController($this->userData, $this->csrfGenerator, $this->account);
+
+
+    $translation = $this->getMock('Drupal\Core\StringTranslation\TranslationInterface');

Remove one of the blank lines.

+++ b/core/modules/overlay/tests/Drupal/overlay/Tests/Controller/OverlayControllerTest.phpundefined
@@ -0,0 +1,152 @@
+  }
+
+}
+
+}

Indicating bad indentation. It's the namespace wrapper.

+++ b/core/modules/overlay/tests/Drupal/overlay/Tests/Controller/OverlayControllerTest.phpundefined
@@ -0,0 +1,152 @@
+namespace {
+  // @todo Convert once drupal_set_message is an object
+  if (!function_exists('drupal_set_message')) {
+    function drupal_set_message($message) {
+    }
+  }
+}
+
+

What's this? Does not seem to be needed.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
9.17 KB
4.6 KB

I addressed the remarks from above. I also noticed that when I run the tests without the patch applied that I do not get any tests results back. Is that how these unit tests are supposed to behave?

dawehner’s picture

Status: Needs review » Needs work

I addressed the remarks from above. I also noticed that when I run the tests without the patch applied that I do not get any tests results back. Is that how these unit tests are supposed to behave?

Per default the phpunit test runner just tell you about failed assertions. I would recommend to not run them via the simpletest UI as their are tons of bogs in the integration. (For example try to let them run twice)

+++ b/core/modules/overlay/tests/Drupal/overlay/Tests/Controller/OverlayControllerTest.php
@@ -5,12 +5,12 @@
-namespace Drupal\overlay\Tests\Controller {

@@ -138,15 +137,3 @@ public function testOverlayMessageWithInvalidToken() {
 }
-
-}
-
-namespace {
-  // @todo Convert once drupal_set_message is an object
-  if (!function_exists('drupal_set_message')) {
-    function drupal_set_message($message) {
-    }
-  }
-}
-

Sorry but this change lets the phpunit test fail in a phpunit test only enviroment. Please add that bit back.

pfrenssen’s picture

Ok I will address this when I get back home this evening. Is there some documentation available on running PHPUnit tests so I can try this locally?

pfrenssen’s picture

Here is a bit of documentation on running PHP Unit tests, in the change record: Test framework PHPUnit has been added. So the call to drupal_set_message() needs to be overridden in the test because PHPUnit does not include the procedural Drupal code by default, it relies on methods in classes by the autoloader.

pfrenssen’s picture

FileSize
2.14 KB
9.35 KB

Ok I get it. We can't post a 'test_only' patch to prove that the test works, because the old code uses the drupal_valid_token() function from the global namespace, which the unit test cannot use.

The weird indentation of the test is because we expect to remove the override of drupal_set_message() in the future, and this would make the future patch much more readable as we wouldn't need to reformat the entire class. So it was actually fine like it was.

Sorry for the confusion, I was not familiar with this approach. This is a new patch, with interdiff against dawehner's last patch from #56. Ignore the one from #58.

pfrenssen’s picture

Status: Needs work » Needs review
mrded’s picture

Assigned: mrded » Unassigned
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PHPUnit, +WSCCI Conversions

Sorry for the confusion, I was not familiar with this approach. This is a new patch, with interdiff against dawehner's last patch from #56. Ignore the one from #58.

Yeah this is sad and confusing that we need to do that.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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