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.

Files: 
CommentFileSizeAuthor
#42 drupal-1987572-42.patch12.6 KBmrded
PASSED: [[SimpleTest]]: [MySQL] 58,971 pass(es).
[ View ]
#42 interdiff-1987572-33-42.txt1.5 KBmrded
#33 drupal-1987572-33.patch12.54 KBmrded
PASSED: [[SimpleTest]]: [MySQL] 58,182 pass(es).
[ View ]
#33 interdiff-1987572-28-33.txt754 bytesmrded
#28 drupal-1987572-28.patch12.67 KBmrded
PASSED: [[SimpleTest]]: [MySQL] 58,095 pass(es).
[ View ]
#23 drupal-1987572-23.patch12.83 KBmrded
PASSED: [[SimpleTest]]: [MySQL] 58,464 pass(es).
[ View ]
#20 1987572-diff-16-19.txt3.29 KBvijaycs85
#19 drupal-1987572-19.patch11.67 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]
#19 1987572-diff-16-19.txt11.67 KBvijaycs85
#16 drupal-1987572-16.patch12.79 KBmrded
PASSED: [[SimpleTest]]: [MySQL] 58,471 pass(es).
[ View ]
#13 drupal-1987572-13.patch12.16 KBmrded
FAILED: [[SimpleTest]]: [MySQL] 58,163 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 1987572-convert-session-tests-to-routes.patch10.92 KBSean Buscay
FAILED: [[SimpleTest]]: [MySQL] 58,094 pass(es), 25 fail(s), and 2 exception(s).
[ View ]
#1 1987572-session-test-get-route-1.patch3.08 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 55,717 pass(es).
[ View ]

Comments

Component:syslog.module» system.module
Status:Active» Needs review
StatusFileSize
new3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,717 pass(es).
[ View ]

Initial patch...

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.phpundefined
@@ -0,0 +1,41 @@
+class SessionTestController implements ControllerInterface {

As there is no need for any injection, there is no need for the Interface as well.

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.phpundefined
@@ -0,0 +1,41 @@
+  /**
+   * Implements \Drupal\Core\ControllerInterface::create().
+   */
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('database'));

What is the point in using the database here? I guess this was just a c&p

+++ b/core/modules/system/tests/modules/session_test/session_test.moduleundefined
@@ -6,8 +6,7 @@
   $items['session-test/get'] = array(
     'title' => 'Session value',
-    'page callback' => '_session_test_get',
-    'access arguments' => array('access content'),
+    'route_name' => 'session_test_get',
     'type' => MENU_CALLBACK,

Afaik we don't need the title, so there should be no reason to keep the hook_menu entry.

Status:Needs review» Closed (won't fix)

Closing as it is test module issue.

Assigned:Unassigned» Sean Buscay
Status:Closed (won't fix)» Active

Status:Active» Needs review
StatusFileSize
new10.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,094 pass(es), 25 fail(s), and 2 exception(s).
[ View ]

This patch converts all menu call backs in session_test.module to routes, and adds the controller for the tests.

Status:Needs review» Needs work

The last submitted patch, 1987572-convert-session-tests-to-routes.patch, failed testing.

The tests failed at a part of the code which the patch did not touch. The tests which the patch relies on have intermittently failed on local test systems but this is the first known time they have failed with the test bot. The tests have been re-run to see if the fail occurs each test run or just intermittently.

The next step will be to determine if the patch introduced something that caused the fail with the test bot test, or whether the fail was due to the previously non reproducible fails from some local systems.

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

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

The last submitted patch, 1987572-convert-session-tests-to-routes.patch, failed testing.

Title:Convert _session_test_get() to a new style controllerConvert session_test callbacks to a new style controller
Assigned:Sean Buscay» Unassigned

Unassigning since this hasn't been worked on in a while.

Assigned:Unassigned» mrded
Issue tags:+LONDON_2013_AUGUST

Working on this as part of London sprint.

Status:Needs work» Needs review
StatusFileSize
new12.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,163 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Please, take a look my patch.

Nice one @mrded, few comments at first look.

  1. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,114 @@
    +   * Page callback, prints the stored session value to the screen.
    ...
    +   * Menu callback: print the current session ID.
    ...
    +   * Menu callback: print the current session ID as read from the cookie.
    ...
    +   * Page callback, stores a value in $_SESSION['session_test_value'].
    ...
    +   * Menu callback: turns off session saving and then tries to save a value
    ...
    +   * Menu callback, sets a message to me displayed on the following page.
    ...
    +   * Menu callback, sets a message but call drupal_save_session(FALSE).
    ...
    +   * Menu callback, stores a value in $_SESSION['session_test_value'] without
    +   * having started the session in advance.
    ...
    +   * Menu callback, only available if current user is logged in.

    Minor: remove prefix 'Page callback/ Menu callback' ?

  2. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,114 @@
    +    drupal_set_message(t('This is a dummy message.'));

    I guess, we can inject t()

Status:Needs review» Needs work

The last submitted patch, drupal-1987572-13.patch, failed testing.

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

New version.

good work @mrded. Looks good to me.

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
@@ -0,0 +1,118 @@
+class SessionTestController implements ContainerInjectionInterface {
+  /**
+   * var \Drupal\Core\Language\LanguageManager
+   */
+  protected $translationManager;
+
+  public function __construct(TranslationManager $translation_manager) {
+    $this->translationManager = $translation_manager;
+  }
...
+      : t('The current value of the stored session variable is: %val', array('%val' => $_SESSION['session_test_value']));
...
+    return $this->translationManager->translate('The current value of the stored session variable has been set to %val', array('%val' => $test_value));
...
+    return $this->translationManager->translate('session saving was disabled, and then %val was set', array('%val' => $test_value));
...
+    print $this->translationManager->translate('A message was set.');
...
+      $this->set($this->translationManager->translate('Session was not started'));
...
+    return $this->translationManager->translate('User is logged in.');

If you use ControllerBase you could directly use $this->t()

StatusFileSize
new11.67 KB
new11.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]

Quick one for #18

StatusFileSize
new3.29 KB

wrong diff file...

  1. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,111 @@
    + * Defines a default implementation for controllers.

    Ha, someone was lazy here :)

  2. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,111 @@
    +  /**
    +   * Prints the stored session value to the screen.
    +   */
    +  public function get() {
    ...
    +  /**
    +   * Print the current session ID.
    +   */
    +  public function getId() {
    ...
    +
    +  /**
    +   * Print the current session ID as read from the cookie.
    +   */
    +  public function getIdFromCookie(Request $request) {
    ...
    +
    +  /**
    +   * Stores a value in $_SESSION['session_test_value'].
    +   */
    +  public function set($test_value) {
    ...
    +  /**
    +   * Turns off session saving and then tries to save a value
    +   * anyway.
    +   */
    +  public function noSet($test_value) {
    ...
    +  /**
    +   * Sets a message to me displayed on the following page.
    +   */
    +  public function setMessage() {
    ...
    +  /**
    +   * Sets a message but call drupal_save_session(FALSE).
    +   */
    +  public function setMessageButDontSave() {
    ...
    +  /**
    +   * Stores a value in $_SESSION['session_test_value'] without
    +   * having started the session in advance.
    +   */
    +  public function setNotStarted() {
    ...
    +  /**
    +   * Only available if current user is logged in.
    +   */
    +  public function isLoggedIn() {
    +    return $this->t('User is logged in.');

    Let's just add a @return statement and @param on some of them.

  3. +++ b/core/modules/system/tests/modules/session_test/session_test.module
    @@ -5,145 +5,46 @@
    function session_test_menu() {
       $items['session-test/get'] = array(
    -    'title' => 'Session value',
    -    'page callback' => '_session_test_get',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_get',
       );
       $items['session-test/id'] = array(
    -    'title' => 'Session ID',
    -    'page callback' => '_session_test_id',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_id',
       );
       $items['session-test/id-from-cookie'] = array(
    -    'title' => 'Session ID from cookie',
    -    'page callback' => '_session_test_id_from_cookie',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_id_from_cookie',
       );
       $items['session-test/set/%'] = array(
    -    'title' => 'Set session value',
    -    'page callback' => '_session_test_set',
    -    'page arguments' => array(2),
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_set',
       );
       $items['session-test/no-set/%'] = array(
    -    'title' => 'Set session value but do not save session',
    -    'page callback' => '_session_test_no_set',
    -    'page arguments' => array(2),
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_no_set',
       );
       $items['session-test/set-message'] = array(
    -    'title' => 'Set message',
    -    'page callback' => '_session_test_set_message',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_set_message',
       );
       $items['session-test/set-message-but-dont-save'] = array(
    -    'title' => 'Set message but do not save session',
    -    'page callback' => '_session_test_set_message_but_dont_save',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_set_message_but_dont_save',
       );
       $items['session-test/set-not-started'] = array(
    -    'title' => 'Set message when session is not started',
    -    'page callback' => '_session_test_set_not_started',
    -    'access arguments' => array('access content'),
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_set_not_started',
       );
       $items['session-test/is-logged-in'] = array(
    -    'title' => 'Check if user is logged in',
    -    'page callback' => '_session_test_is_logged_in',
    -    'access callback' => 'user_is_logged_in',
         'type' => MENU_CALLBACK,
    +    'route_name' => 'session_test_is_logged_in',
       );
       return $items;
    }

    The full hook_menu can be killed as we just have MENU_CALLBACKS on there.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new12.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,464 pass(es).
[ View ]

There is a new patch.

  1. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,143 @@
    +   * @return string
    +   */
    ...
    +   * @return string
    +   */
    ...
    +   * @return string
    +   */
    ...
    +   *
    +   * @param string $test_value
    +   *
    +   * @return string
    +   */
    ...
    +   *
    +   * @param string $test_value
    +   *
    +   * @return string
    +   */
    ...
    +   *
    +   * @return string
    +   */
    ...
    +   * having started the session in advance.
    +   *
    +   * @return string
    +   */
    ...
    +   *
    +   * @return string
    ...
    +  public function isLoggedIn() {
    +    return $this->translationManager->translate('User is logged in.');

    The point is to also describe what the values are instead of just adding the @param/@return statements.

  2. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,143 @@
    +    return $this->translationManager->translate('The current value of the stored session variable has been set to %val', array('%val' => $test_value));
    ...
    +    return $this->translationManager->translate('session saving was disabled, and then %val was set', array('%val' => $test_value));
    ...
    +    drupal_set_message($this->translationManager->translate('This is a dummy message.'));
    +    print $this->translationManager->translate('A message was set.');
    ...
    +      $this->set($this->translationManager->translate('Session was not started'));

    If you extend from ControllerBase you can use just $this->t() and you should.

Status:Needs review» Needs work

If you extend from ControllerBase you can use just $this->t() and you should.

No, I cannot. I got the follow error:
( ! ) Fatal error: Call to undefined method Drupal\session_test\Controller\SessionTestController::t() in /Users/mrded/Sites/drupal8/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php on line 74

mrded, here is how you extend ControllerBase so you can use $this->t() in the class:

  1. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,143 @@
    +use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    +use Drupal\Core\StringTranslation\TranslationManager;
    +use Symfony\Component\DependencyInjection\ContainerInterface;

    This becomes:
    use Drupal\Core\Controller\ControllerBase;

  2. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
    @@ -0,0 +1,143 @@
    +class SessionTestController implements ContainerInjectionInterface {
    +  /**
    +   * var \Drupal\Core\Language\LanguageManager
    +   */
    +  protected $translationManager;
    +
    +  public function __construct(TranslationManager $translation_manager) {
    +    $this->translationManager = $translation_manager;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static($container->get('string_translation'));
    +  }

    replace this with:
    class SessionTestController extends ControllerBase {

    Then the constructor and create are unneeded, as well as $translationManager.

Status:Needs work» Needs review
StatusFileSize
new12.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,095 pass(es).
[ View ]

@disasm, thank you for your help!
There is a new patch, hopefully it will be the final version :)

Status:Needs review» Needs work

The last submitted patch, drupal-1987572-28.patch, failed testing.

Status:Needs work» Needs review

@mrded please attach an interdiff when you make changes. This makes it easier on reviewers to look at what's different between the patch in #23 and #28. I'm retesting this because having troubles seeing how a test that doesn't use the session_test module is failing.

#28: drupal-1987572-28.patch queued for re-testing.

Status:Needs review» Needs work

Looking good! One minor changge, and then this is ready.

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
@@ -0,0 +1,144 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }

Since nothing is being injected, this block can be completely removed.

Status:Needs work» Needs review
StatusFileSize
new754 bytes
new12.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,182 pass(es).
[ View ]

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
@@ -0,0 +1,137 @@
+    print $this->t('A message was set.');
+    // Do not return anything, so the current request does not result in a themed
+    // page with messages. The message will be displayed in the following request
+    // instead.

Are you sure this should not rather use a Response object?

Are you sure this should not rather use a Response object?

No :) Testing results is same. Can you advise me what is the best way?

If it still passes tests, lets switch it to a response object. If that causes a fail to happen, lets keep it the way it is.

Just to be sure, when you say "response object" do you mean changing $this->t() to $this->translationManager->translate() ?

No, I rather suggest return new Response($this->t(...)); and that's it.

Should I wrap all returns to new Response() ? Where can I read about it? Thank you.

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.php
@@ -0,0 +1,137 @@
+    print $this->t('A message was set.');
+    // Do not return anything, so the current request does not result in a themed
+    // page with messages. The message will be displayed in the following request
+    // instead.

dawehner is only referring to this one. It's printing, instead of returning a Response object. In general, you should never print data from a controller. return a string, or an array is fine, but never print data.

this would become return new Response($this->t('A message was set.'));

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.5 KB
new12.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,971 pass(es).
[ View ]

Got it.

Status:Needs review» Reviewed & tested by the community

Thank you!

Finally! Thank you guys for your help with it! :)

#42: drupal-1987572-42.patch queued for re-testing.

Status:Reviewed & tested by the community» Fixed

+++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Controller/SessionTestController.phpundefined
@@ -0,0 +1,138 @@
+    return empty($_SESSION['session_test_value'])
...
+    $_SESSION['test'] = 'test';
...
+    $_SESSION['session_test_value'] = $test_value;

We're trying to remove calls to global variables throughout D8 core. However, I grepped and couldn't find much in the way of lower-case "session" and I found much in the way of "SESSION." So I think this is fine for now.

+++ b/core/modules/system/tests/modules/session_test/session_test.routing.ymlundefined
@@ -0,0 +1,78 @@
+    _permission: 'access content'
+
+
+session_test_no_set:

(minor) two newlines rather than one.

Fixed on commit.

Committed and pushed to 8.x. Thanks!

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