Convert session_test callbacks to a new style controller

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

#1: 1987572-session-test-get-route-1.patch queued for re-testing.

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

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.

#6: 1987572-convert-session-tests-to-routes.patch queued for re-testing.

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

Working on this as part of London sprint.

Please, take a look my patch.

New version.

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

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.

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.

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.

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

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

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.

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

Got it.

Thank you!

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

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

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

Follow-up: #2090969: Minor code-style issues in SessionTestController