Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Component: syslog.module » system.module
Status: Active » Needs review
FileSize
3.08 KB

Initial patch...

dawehner’s picture

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

kim.pepper’s picture

vijaycs85’s picture

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

Closing as it is test module issue.

Sean Buscay’s picture

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

Status: Active » Needs review
FileSize
10.92 KB

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.

Sean Buscay’s picture

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.

Sean Buscay’s picture

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.

disasm’s picture

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

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

mrded’s picture

Assigned: Unassigned » mrded
Issue tags: +LONDON_2013_AUGUST

Working on this as part of London sprint.

mrded’s picture

Status: Needs work » Needs review
FileSize
12.16 KB

Please, take a look my patch.

vijaycs85’s picture

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.

mrded’s picture

Status: Needs work » Needs review
FileSize
12.79 KB

New version.

vijaycs85’s picture

good work @mrded. Looks good to me.

dawehner’s picture

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

vijaycs85’s picture

Quick one for #18

vijaycs85’s picture

FileSize
3.29 KB

wrong diff file...

dawehner’s picture

  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.

disasm’s picture

Status: Needs review » Needs work
mrded’s picture

Status: Needs work » Needs review
FileSize
12.83 KB

There is a new patch.

dawehner’s picture

  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.

disasm’s picture

Status: Needs review » Needs work
mrded’s picture

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

disasm’s picture

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.

mrded’s picture

Status: Needs work » Needs review
FileSize
12.67 KB

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

disasm’s picture

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.

disasm’s picture

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

disasm’s picture

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.

mrded’s picture

Status: Needs work » Needs review
FileSize
754 bytes
12.54 KB
dawehner’s picture

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

mrded’s picture

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?

disasm’s picture

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.

mrded’s picture

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

dawehner’s picture

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

mrded’s picture

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

disasm’s picture

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

disasm’s picture

Status: Needs review » Needs work
mrded’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
12.6 KB

Got it.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

mrded’s picture

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

webchick’s picture

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

webchick’s picture

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!

tstoeckler’s picture

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