PHP Fatal error: Call to a member function getId() on null in web/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php

SessionCacheContext does not check returned value before calling getId on it. This assumes that session will always be there.

Motivation:

  1. Drupal should support integration with drupal console or any other command line tool that bootstraps drupal.
  2. Make cache handling more graceful.

Steps required to reproduce the bug:

1. Create custom block:

public function build() {
    $cachable_metadata = new CacheableMetadata();
    $cachable_metadata->addCacheContexts(['session']);
    return array(
      '#markup' => '',
      '#cache' => [
        'contexts' => $cachable_metadata->getCacheContexts(),
        'tags' => $cachable_metadata->getCacheTags(),
        'max-age' => $cachable_metadata->getCacheMaxAge(),
      ],
    );
  }

2. clear cache
3. run any drupal console command. (example: drupal ror )

Expected behaviour:

Command executes without error.

What happened instead:
Got:

PHP Fatal error: Call to a member function getId() on null in web/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php

Remaining tasks

CommentFileSizeAuthor
#52 interdiff-46-52.txt622 bytesbenjifisher
#52 2881348-52.patch4.07 KBbenjifisher
#52 2881348-52-test-only.patch3.37 KBbenjifisher
#48 2881348-46-test-only.patch3.39 KBbenjifisher
#46 interdiff-39-46.txt953 bytesbenjifisher
#46 2881348-46.patch4.1 KBbenjifisher
#43 interdiff-31-39.txt2.57 KBbenjifisher
#41 interdiff-38-39.txt847 bytesanya_m
#40 2881348-39.patch4.7 KBanya_m
#38 2881348-38.patch4.67 KBDinesh18
#38 interdiff.txt2.75 KBDinesh18
#31 2881348-31.patch3.78 KBjofitz
#27 2881348-27.patch2.52 KBjofitz
#24 cache-context-verify-session-2881348-24-test-only.patch2.55 KBbenjifisher
#13 cache-context-verify-session-2881348-13.patch3.82 KBguncha25
#11 cache-context-verify-session-2881348-11.patch1.29 KBguncha25
#8 interdiff-2881348-4-8.txt801 bytesguncha25
#8 cache-context-verify-session-2881348-8.patch989 bytesguncha25
#7 interdiff.txt422 bytespritish.kumar
#7 cache-context-verify-session-2881348-7.patch971 bytespritish.kumar
#4 cache-context-verify-session-2881348-4.patch943 bytesguncha25
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guncha25 created an issue. See original summary.

James Nesbitt’s picture

It seems clear that the cli is not properly initializing the core request with a session, probably a cli issue.

However, it seems acceptable to put some safety catch into the \Drupal\Core\Cache\Context\SessionCacheContext. Any proper deeper core fix would be in symfony,

Here is the code that triggers an error:

  /**
   * {@inheritdoc}
   */
  public function getContext() {
    $sid = $this->requestStack->getCurrentRequest()->getSession()->getId();
    return Crypt::hashBase64($sid);
  }

suggestions?

James Nesbitt’s picture

Issue summary: View changes

reformatted the issue body

guncha25’s picture

cilefen’s picture

Status: Active » Needs review
James Nesbitt’s picture

Status: Needs review » Needs work

@guncha25 can you please make the method return some static predictable hash if the session $sid is not found.

Some predictable hash that is static across the request would be good.

pritish.kumar’s picture

Status: Needs work » Needs review
FileSize
971 bytes
422 bytes

Returned the Crypt::hashBase64(1) as predictable hash.

guncha25’s picture

larowlan’s picture

+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -22,8 +23,11 @@ public static function getLabel() {
+    return Crypt::hashBase64(1);

Returning a hash of a constant value seems wrong and could lead to an artificial sense of this being something secure. Why not just return 0 or 'none' here? If there is no session, then there is no session context?

Also I note that SessionCacheContext is used as a cache context, but doesn't implement \Drupal\Core\Cache\Context\CacheContextInterface - perhaps it should, otherwise it's missing methods might cause other fatals namely getCacheableMetadata. Perhaps that is a different issue though.

znerol’s picture

Please use $request->hasSession() to test for the presence of a session on the request.

guncha25’s picture

  • Added CacheContextInterface as one of points here was to make cache handling more graceful.
  • Checked for session with hasSession()
  • Returned 'none' if no session.
larowlan’s picture

Do we need a test?

+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -22,8 +23,18 @@ public static function getLabel() {
+    if ($this->requestStack->getCurrentRequest()->hasSession()) {
+      $sid = $this->requestStack->getCurrentRequest()->getSession()->getId();

we could store the current request in a local variable to simplify here

guncha25’s picture

dawehner’s picture

Do we need a test?

I think this is always a good indicator to add a test :)

+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -22,8 +23,18 @@ public static function getLabel() {
   public function getContext() {
-    $sid = $this->requestStack->getCurrentRequest()->getSession()->getId();
-    return Crypt::hashBase64($sid);
+    $request = $this->requestStack->getCurrentRequest();
+    if ($request->hasSession()) {
+      return Crypt::hashBase64($request->getSession()->getId());
+    }
+    return 'none';
+  }

Do we have an issue to actually hash the values in the session instead of just hashing the session ID?

Added CacheContextInterface as one of points here was to make cache handling more graceful.

Is this required to fix this particular problem?

guncha25’s picture

It is not required to solve this error - no. If you suggest to remove it i can do that. Idea was just to make this cache context more stable. Other than that test is created and passing.

kenorb’s picture

I've tested the patch, but it doesn't solve the error in my case. Here is the error after applied the patch:

$ DRUSH_PHP=php70 drush ev "new \Drupal\om_1t_interface\IntegrationServices\TaskQueueRepairHistory;"
Error: Call to a member function getId() on null in Drupal\user\PrivateTempStore->getOwner() (line 210 of                                                   [error]
docroot/core/modules/user/src/PrivateTempStore.php) #0
docroot/core/modules/user/src/PrivateTempStore.php(200): Drupal\user\PrivateTempStore->getOwner()
#1 docroot/core/modules/user/src/PrivateTempStore.php(102): Drupal\user\PrivateTempStore->createkey('Token')
#2 docroot/modules/custom/om_1t_interface/src/Helper/UserSession.php(72):
Drupal\user\PrivateTempStore->get('Token')
#3 docroot/modules/custom/om_1t_interface/src/Helper/UserSession.php(134):
Drupal\om_1t_interface\Helper\UserSession::getPropertyValue('Token')
#4
docroot/modules/custom/om_1t_interface/src/IntegrationServices/FirstTouchServiceAbstract.php(73):
Drupal\om_1t_interface\Helper\UserSession::getToken()
#5
docroot/modules/custom/om_1t_interface/src/IntegrationServices/FirstTouchServiceAbstract.php(38):
Drupal\om_1t_interface\IntegrationServices\FirstTouchServiceAbstract->setSecurityHeader()
#6 docroot/modules/custom/om_1t_interface/src/IntegrationServices/Status.php(25):
Drupal\om_1t_interface\IntegrationServices\FirstTouchServiceAbstract->__construct('IdentityService', 'GetStatus')
#7 docroot/modules/custom/om_1t_interface/src/IntegrationServices/Status.php(46):
Drupal\om_1t_interface\IntegrationServices\Status->__construct()
#8 docroot/modules/custom/om_1t_interface/src/IntegrationServices/TaskServiceAbstract.php(36):
Drupal\om_1t_interface\IntegrationServices\Status::checkToken()
#9 vendor/drush/drush/commands/core/core.drush.inc(1168) : eval()'d code(1):
Drupal\om_1t_interface\IntegrationServices\TaskServiceAbstract->__construct()
#10 vendor/drush/drush/commands/core/core.drush.inc(1168): eval()
#11 vendor/drush/drush/includes/command.inc(422): drush_core_php_eval('new \\Drupal\\om_...')
#12 vendor/drush/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#13 vendor/drush/drush/includes/command.inc(199): drush_command('new \\Drupal\\om_...')
#14 vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#15 vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#16 vendor/drush/drush/drush.php(12): drush_main()
#17 {main}.
Error: Call to a member function getId() on null in docroot/core/modules/user/src/PrivateTempStore.php on line 210 #0 docroot/core/modules/user/src/PrivateTempStore.php(200): Drupal\user\PrivateTempStore->getOwner()
#1 docroot/core/modules/user/src/PrivateTempStore.php(102): Drupal\user\PrivateTempStore->createkey('Token')
#2 docroot/modules/custom/om_1t_interface/src/Helper/UserSession.php(72): Drupal\user\PrivateTempStore->get('Token')
#3 docroot/modules/custom/om_1t_interface/src/Helper/UserSession.php(134): Drupal\om_1t_interface\Helper\UserSession::getPropertyValue('Token')
#4 docroot/modules/custom/om_1t_interface/src/IntegrationServices/FirstTouchServiceAbstract.php(73): Drupal\om_1t_interface\Helper\UserSession::getToken()
#5 docroot/modules/custom/om_1t_interface/src/IntegrationServices/FirstTouchServiceAbstract.php(38): Drupal\om_1t_interface\IntegrationServices\FirstTouchServiceAbstract->setSecurityHeader()
#6 docroot/modules/custom/om_1t_interface/src/IntegrationServices/Status.php(25): Drupal\om_1t_interface\IntegrationServices\FirstTouchServiceAbstract->__construct('IdentityService', 'GetStatus')
#7 docroot/modules/custom/om_1t_interface/src/IntegrationServices/Status.php(46): Drupal\om_1t_interface\IntegrationServices\Status->__construct()
#8 docroot/modules/custom/om_1t_interface/src/IntegrationServices/TaskServiceAbstract.php(36): Drupal\om_1t_interface\IntegrationServices\Status::checkToken()
#9 vendor/drush/drush/commands/core/core.drush.inc(1168) : eval()'d code(1): Drupal\om_1t_interface\IntegrationServices\TaskServiceAbstract->__construct()
#10 vendor/drush/drush/commands/core/core.drush.inc(1168): eval()
#11 vendor/drush/drush/includes/command.inc(422): drush_core_php_eval('new \\Drupal\\om_...')
#12 vendor/drush/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#13 vendor/drush/drush/includes/command.inc(199): drush_command('new \\Drupal\\om_...')
#14 vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#15 vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#16 vendor/drush/drush/drush.php(12): drush_main()
#17 {main}
Error: Call to a member function getId() on null in Drupal\user\PrivateTempStore->getOwner() (line 210 of docroot/core/modules/user/src/PrivateTempStore.php).
Drush command terminated abnormally due to an unrecoverable error.                                                                                          [error]

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

moonray’s picture

Search API indexing wasn't updating for me during cron runs. After this patch everything ran once more.
Not sure if I should mark as reviewed due to comment #16.

vprocessor’s picture

Got this issue:

Error: Call to a member function getId() on null in Drupal\Core\Cache\Context\SessionCacheContext->getContext() (line 25 of                                                           [error]
/var/www/html/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php) #0 /var/www/html/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php(118):
Drupal\Core\Cache\Context\SessionCacheContext->getContext(NULL)
#1 /var/www/html/core/lib/Drupal/Core/Render/RenderCache.php(307): Drupal\Core\Cache\Context\CacheContextsManager->convertTokensToKeys(Array)
#2 /var/www/html/core/lib/Drupal/Core/Render/RenderCache.php(93): Drupal\Core\Render\RenderCache->createCacheID(Array)
#3 /var/www/html/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php(127): Drupal\Core\Render\RenderCache->set(Array, Array)
#4 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(525): Drupal\Core\Render\PlaceholderingRenderCache->set(Array, Array)
#5 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(195): Drupal\Core\Render\Renderer->doRender(Array, true)
#6 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(151): Drupal\Core\Render\Renderer->render(Array, true)
#7 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(574): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
#8 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(152): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#9 /var/www/html/core/modules/node/src/Plugin/Search/NodeSearch.php(480): Drupal\Core\Render\Renderer->renderPlain(Array)
#10 /var/www/html/core/modules/node/src/Plugin/Search/NodeSearch.php(452): Drupal\node\Plugin\Search\NodeSearch->indexNode(Object(Drupal\node\Entity\Node))
#11 /var/www/html/core/modules/search/search.module(195): Drupal\node\Plugin\Search\NodeSearch->updateIndex()
#12 [internal function]: search_cron()
#13 /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php(391): call_user_func_array('search_cron', Array)
#14 /var/www/html/core/lib/Drupal/Core/Cron.php(223): Drupal\Core\Extension\ModuleHandler->invoke('search', 'cron')
#15 /var/www/html/core/lib/Drupal/Core/Cron.php(122): Drupal\Core\Cron->invokeCronHandlers()
#16 /var/www/html/core/lib/Drupal/Core/ProxyClass/Cron.php(75): Drupal\Core\Cron->run()
#17 phar:///usr/bin/drush/commands/core/core.drush.inc(674): Drupal\Core\ProxyClass\Cron->run()
#18 phar:///usr/bin/drush/includes/command.inc(422): drush_core_cron()
#19 phar:///usr/bin/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#20 phar:///usr/bin/drush/includes/command.inc(199): drush_command()
#21 phar:///usr/bin/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#22 phar:///usr/bin/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#23 phar:///usr/bin/drush/includes/startup.inc(458): drush_main()
#24 phar:///usr/bin/drush/includes/startup.inc(365): drush_run_main(false, '/', 'Phar detected. ...')
#25 phar:///usr/bin/drush/drush(114): drush_startup(Array)
#26 /usr/bin/drush(10): require('phar:///usr/bin...')
#27 {main}.

Error: Call to a member function getId() on null in /var/www/html/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php on line 25 #0 /var/www/html/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php(118): Drupal\Core\Cache\Context\SessionCacheContext->getContext(NULL)
#1 /var/www/html/core/lib/Drupal/Core/Render/RenderCache.php(307): Drupal\Core\Cache\Context\CacheContextsManager->convertTokensToKeys(Array)
#2 /var/www/html/core/lib/Drupal/Core/Render/RenderCache.php(93): Drupal\Core\Render\RenderCache->createCacheID(Array)
#3 /var/www/html/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php(127): Drupal\Core\Render\RenderCache->set(Array, Array)
#4 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(525): Drupal\Core\Render\PlaceholderingRenderCache->set(Array, Array)
#5 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(195): Drupal\Core\Render\Renderer->doRender(Array, true)
#6 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(151): Drupal\Core\Render\Renderer->render(Array, true)
#7 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(574): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
#8 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(152): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#9 /var/www/html/core/modules/node/src/Plugin/Search/NodeSearch.php(480): Drupal\Core\Render\Renderer->renderPlain(Array)
#10 /var/www/html/core/modules/node/src/Plugin/Search/NodeSearch.php(452): Drupal\node\Plugin\Search\NodeSearch->indexNode(Object(Drupal\node\Entity\Node))
#11 /var/www/html/core/modules/search/search.module(195): Drupal\node\Plugin\Search\NodeSearch->updateIndex()
#12 [internal function]: search_cron()
#13 /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php(391): call_user_func_array('search_cron', Array)
#14 /var/www/html/core/lib/Drupal/Core/Cron.php(223): Drupal\Core\Extension\ModuleHandler->invoke('search', 'cron')
#15 /var/www/html/core/lib/Drupal/Core/Cron.php(122): Drupal\Core\Cron->invokeCronHandlers()
#16 /var/www/html/core/lib/Drupal/Core/ProxyClass/Cron.php(75): Drupal\Core\Cron->run()
#17 phar:///usr/bin/drush/commands/core/core.drush.inc(674): Drupal\Core\ProxyClass\Cron->run()
#18 phar:///usr/bin/drush/includes/command.inc(422): drush_core_cron()
#19 phar:///usr/bin/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#20 phar:///usr/bin/drush/includes/command.inc(199): drush_command()
#21 phar:///usr/bin/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#22 phar:///usr/bin/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#23 phar:///usr/bin/drush/includes/startup.inc(458): drush_main()
#24 phar:///usr/bin/drush/includes/startup.inc(365): drush_run_main(false, '/', 'Phar detected. ...')
#25 phar:///usr/bin/drush/drush(114): drush_startup(Array)
#26 /usr/bin/drush(10): require('phar:///usr/bin...')
#27 {main}
Error: Call to a member function getId() on null in Drupal\Core\Cache\Context\SessionCacheContext->getContext() (line 25 of /var/www/html/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php).

This patch helped me.

Drupal core 8.3.5

matzAB’s picture

I had this issue with a sub requests and the patch fixed it. Thx

andypost’s picture

+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -22,8 +23,18 @@ public static function getLabel() {
+    if ($request->hasSession()) {
+      return Crypt::hashBase64($request->getSession()->getId());

what if session is not started?

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

Like @moonray in #18, I ran into this when running search_cron(). I was running cron via drush, so there was no session. I am not sure why only one node out of 2700 or so caused a problem, but this patch fixed it for me.

The problem in #16 is from a different part of the code. This issue is not supposed to solve all bugs in Drupal core that lead to Error: Call to a member function getId() on null.

Outstanding issues:

  1. In #9, @larowlan suggested implementing CacheContextInterface (with the caveat "Perhaps that is a different issue though"). The current patch implements that suggestion, but in #14 @dawehner questioned that decision.
  2. In #22, @andypost asks, "what if session is not started?"

I am not qualified to decide whether (1) is a good idea.

For (2), I see the following in Request.php, although I am still using Drupal 8.3 so Symfony 2.8 or whatever ... this may change with Drupal 8.4 and Symfony 3.2 or whatever.

    public function getSession()
    {
        return $this->session;
    }
    public function hasSession()
    {
        return null !== $this->session;
    }

So I think it is pretty clear (for this version of Symfony) that if hasSession() returns true, then getSession() will not return null.

Update: I checked with Drupal 8.4.x, and the code above has not changed in symfony/http-foundation v3.2.8.

benjifisher’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.55 KB

I did not review the test. I am adding a test-only version of @gunch25's patch from #13. If this patch fails, then it should be considered a success for the real patch.

Status: Needs review » Needs work
benjifisher’s picture

Issue summary: View changes
Issue tags: +Needs reroll, +Novice

I am re-testing with Drupal 8.3.x as a sanity check. (Maybe just my sanity.) This patch will need a re-roll for 8.4. That is a novice task.

jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice
FileSize
2.52 KB

Re-rolled.

benjifisher’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice

@Jo Fitzgerald: It looks as though you re-rolled the test-only patch, so we expect a failure. (I checked the failure on my test-only patch from #24, and it failed in the expected way.

We still need re-roll of the patch from #13.

andypost’s picture

@benjifisher in my comment I mean that session always exists in core & you need additionally to check for \Symfony\Component\HttpFoundation\Session\SessionInterface::isStarted()

benjifisher’s picture

@andypost I am afraid I do not understand what you mean. According to the error message (before the patch) getSession() is returning null. So what exactly do you mean by "session always exists in core"?

Remember, we only see this error when using CLI tools (drupal console and drush).

I am pretty sure that the patch works: if getSession() returns null, then hasSession() returns false, so this should work:

+    if ($request->hasSession()) {
+      return Crypt::hashBase64($request->getSession()->getId());
+    }
+    return 'none';

My only quibble is that I would turn it around:

+    if (!$request->hasSession()) {
+      return 'none';
+    }
+    return Crypt::hashBase64($request->getSession()->getId());

or maybe even a ternary

+    return $request->hasSession() ? Crypt::hashBase64($request->getSession()->getId()) : 'none';
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice
FileSize
3.78 KB

Really sorry, I must've been half asleep when I did the re-roll. Here is a re-rolled version of the patch in #13.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

I tried to create an interdiff for the patches in #13 and #31, and it came up empty. I guess that is the expected behavior when the only changes are in the context lines of the patch (replacing \PHPUnit_Framework_TestCase with UnitTestCase).

Based on my review in #23 above, I am marking this RTBC.

benjifisher’s picture

@andypost: I noticed the following code in core/lib/Drupal/Core/StackMiddleware/Session.php:

  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
    if ($type === self::MASTER_REQUEST && PHP_SAPI !== 'cli') {
      $session = $this->container->get($this->sessionServiceName);
      $session->start();
      $request->setSession($session);
    }
    // ...
    return $result;
  }

So in this case, the session (on the Request object) is started and set at the same time, and neither happens when Drupal is run from the CLI. Possibly there are other code paths where the session is set but not started.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Core/Cache/Context/SessionCacheContextTest.php
@@ -35,21 +42,22 @@ class SessionCacheContextTest extends UnitTestCase {
-    $this->cacheContext = new SessionCacheContext($this->requestStack);

I don't think we should be storing and using $this->cacheContext on the test class anymore. Each test is just creating it's own cache context object so it can just be a local variable ie $cache_context - which is nice because it is the thing under test.

benjifisher’s picture

Issue tags: +Novice

@alexpott So you want to remove the $cacheContext property from this test class, remove the line that sets it in the setUp() method, and then do

    $cacheContext = new SessionCacheContext($this->requestStack);

in each test method, and replace $this->cacheContext with $cacheContext in those methods?

If I have it right, then this looks like a novice task.

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/Cache/Context/SessionCacheContextTest.php
@@ -35,21 +42,22 @@ class SessionCacheContextTest extends UnitTestCase {
+    $this->cacheContext = new SessionCacheContext($this->requestStack);

@@ -65,6 +73,9 @@ public function testSameContextForSameSession() {
+    $this->cacheContext = new SessionCacheContext($this->requestStack);

@@ -83,4 +94,15 @@ public function testDifferentContextForDifferentSession() {
+    $this->cacheContext = new SessionCacheContext($this->requestStack);

It is already happening in every test. The ::setUp() used to set it but now it does not because of changes made by this patch. Therefore the change is in scope.

benjifisher’s picture

Sorry, I misunderstood. I guess what I said was half right, or maybe 1/3 right.

  1. Remove the $cacheContext property from this test class.
  2. Replace $this->cacheContext with a local variable $cache_context in each test method.
Dinesh18’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
4.67 KB

Here is an updated patch and interdiff as per comment #37

benjifisher’s picture

Issue summary: View changes
Status: Needs review » Needs work

@Dinesh18:

When you provide a patch, please also upload an interdiff comparing it to the previous version. (If you add a new patch, please compare it to the one in #31, not your patch in #38.) Instructions are here: Creating an interdiff.

@@ -32,31 +39,31 @@ class SessionCacheContextTest extends UnitTestCase {
    *
    * @var \Drupal\Core\Cache\Context\SessionCacheContext
    */
-  protected $cacheContext;

When you remove the property, you should also remove its code comment. After applying your patch, I see this in the code:

  /**
   * The session cache context.
   *
   * @var \Drupal\Core\Cache\Context\SessionCacheContext
   */

  public function setUp() {
    // ...
  }

I am surprised that there is no doc block for the setUp(). I would expect the standard {@inheritdoc}. Please leave it as is for this issue: fixing it would be out of scope, and it might even be correct as it is. There are already some exceptions for documenting PHPUnit classes, although leaving off the method's doc block is not listed at https://www.drupal.org/node/2116043. (See "Required PHPDoc metadata for test discoverability" near the bottom of the page.)

anya_m’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
4.7 KB
anya_m’s picture

FileSize
847 bytes
Dinesh18’s picture

@benjifisher, I have provided the interdiff.txt in #38

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.57 KB

@Dinesh18 Thanks.

@anya_m This looks like one of your first patches. Welcome! As I said in #39, I think an interdiff against the patch in #31 will be more useful. I will attach one.

I have reviewed the patch, and I think it does what @alexpott requested. I am marking this issue RTBC, and I will be a little embarrassed if the testbot has a different opinion.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice +Needs followup
+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -3,13 +3,14 @@
+class SessionCacheContext extends RequestStackCacheContextBase implements CacheContextInterface {
 

@@ -22,8 +23,18 @@ public static function getLabel() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheableMetadata() {
+    return new CacheableMetadata();
   }

This is not actually part of this issue. As @larowlan noted in #9

Also I note that SessionCacheContext is used as a cache context, but doesn't implement \Drupal\Core\Cache\Context\CacheContextInterface - perhaps it should, otherwise it's missing methods might cause other fatals namely getCacheableMetadata. Perhaps that is a different issue though.

I think the addition of the interface should go in a separate issue. Afaics from the code because the ID does not contain a "." or ":" we'll never call getCacheableMetadata(). If this change doesn't have the interface addition then it is a bugfix that can get resolved in 8.4.x as well as 8.5.x. Which is good because it is fixing a problem that people are facing.

benjifisher’s picture

Issue tags: -Needs followup

I added the follow-up issue #2915594: SessionCacheContext class should implement CacheContextInterface.

@alexpott, I see that you removed the Novice tag. I am happy to supply a new patch, although I prefer not to review my own patches.

benjifisher’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
4.1 KB
953 bytes

New patch attached, and moving back to 8.4.x, according to @alexpott's comment in #44.

If you reverse the interdiff, it might be good enough for #2915594: SessionCacheContext class should implement CacheContextInterface.

SessionCacheContextTest is passing locally. It would be embarrassing if it failed.

andypost’s picture

Follow-up filed & all feedback addressed

benjifisher’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.39 KB

Thanks, @andypost!

Looking at the list of files you hid, I realize that we should have an updated test-only patch. It is attached.

We expect this patch to fail on SessionCacheContextTest. If it does, then we can revert the testbot's NW to RTBC.

Status: Needs review » Needs work

The last submitted patch, 48: 2881348-46-test-only.patch, failed testing. View results

benjifisher’s picture

Status: Needs work » Reviewed & tested by the community

Yay, the test-only patch failed in the expected way. Back to RTBC as in #47.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Core/Cache/Context/SessionCacheContextTest.php
@@ -75,12 +79,22 @@ public function testDifferentContextForDifferentSession() {
+   * @runInSeparateProcess

I'm really sorry I should have noticed this before. I'm don't see why this is necessary. As far as I can see there are no globals being affected here. If it is necessary there should be some documentation as to why.

@benjifisher thanks for uploading a test-only patch - if you do that you also need to upload the rtbc patch too so that is last on the files list. This means that the rtbc re-test job won't set the issue back to needs works unnecessarily.

benjifisher’s picture

Version: 8.5.x-dev » 8.4.x-dev
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.37 KB
4.07 KB
622 bytes

@alexpott: OK, next time I will be less considerate of the testbot. I hate to give the poor bot extra work.

The @runInSeparateProcess was added by @guncha in #13, the first patch on this issue that touched the test class. Maybe I should get to know the testing system better before I next mark an issue RTBC.

I have added a new patch, a test-only version (in the correct order) and an interdiff.

The last submitted patch, 52: 2881348-52-test-only.patch, failed testing. View results

andypost’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs review » Reviewed & tested by the community

rtbc again)

alexpott’s picture

Ticking reviewer credit boxes for reviewers that have influenced the direction of the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 0c0c63e9af to 8.5.x and 3c7f090ea2 to 8.4.x. Thanks!

  • alexpott committed 0c0c63e on 8.5.x
    Issue #2881348 by benjifisher, guncha25, Jo Fitzgerald, anya_m, Dinesh18...

  • alexpott committed 3c7f090 on 8.4.x
    Issue #2881348 by benjifisher, guncha25, Jo Fitzgerald, anya_m, Dinesh18...
benjifisher’s picture

Yay! Now my cron jobs will work without a custom patch.

I just posted a patch on the follow-up issue #2915594: SessionCacheContext class should implement CacheContextInterface. All I did was reverse interdiff-39-46.txt from this issue.

Status: Fixed » Closed (fixed)

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

vidhatanand’s picture