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.

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

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

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

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

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

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

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.