Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- Drupal should support integration with drupal console or any other command line tool that bootstraps drupal.
- 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
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff-46-52.txt | 622 bytes | benjifisher |
#52 | 2881348-52.patch | 4.07 KB | benjifisher |
#52 | 2881348-52-test-only.patch | 3.37 KB | benjifisher |
Comments
Comment #2
James Nesbitt CreditAttribution: James Nesbitt commentedIt 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:
suggestions?
Comment #3
James Nesbitt CreditAttribution: James Nesbitt commentedreformatted the issue body
Comment #4
guncha25 CreditAttribution: guncha25 commentedComment #5
cilefen CreditAttribution: cilefen commentedComment #6
James Nesbitt CreditAttribution: James Nesbitt at Wunder commented@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.
Comment #7
pritish.kumar CreditAttribution: pritish.kumar at OpenSense Labs commentedReturned the Crypt::hashBase64(1) as predictable hash.
Comment #8
guncha25 CreditAttribution: guncha25 commentedComment #9
larowlanReturning 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.Comment #10
znerol CreditAttribution: znerol commentedPlease use
$request->hasSession()
to test for the presence of a session on the request.Comment #11
guncha25 CreditAttribution: guncha25 commentedhasSession()
Comment #12
larowlanDo we need a test?
we could store the current request in a local variable to simplify here
Comment #13
guncha25 CreditAttribution: guncha25 commentedAdded test.
Comment #14
dawehnerI think this is always a good indicator to add a test :)
Do we have an issue to actually hash the values in the session instead of just hashing the session ID?
Is this required to fix this particular problem?
Comment #15
guncha25 CreditAttribution: guncha25 commentedIt 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.
Comment #16
kenorb CreditAttribution: kenorb commentedI've tested the patch, but it doesn't solve the error in my case. Here is the error after applied the patch:
Comment #18
moonray CreditAttribution: moonray at Chapter Three commentedSearch 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.
Comment #19
vprocessor CreditAttribution: vprocessor at Skilld commentedGot this issue:
This patch helped me.
Drupal core 8.3.5
Comment #20
matzAB CreditAttribution: matzAB commentedI had this issue with a sub requests and the patch fixed it. Thx
Comment #21
andypostComment #22
andypostwhat if session is not started?
Comment #23
benjifisherLike @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:
CacheContextInterface
(with the caveat "Perhaps that is a different issue though"). The current patch implements that suggestion, but in #14 @dawehner questioned that decision.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.So I think it is pretty clear (for this version of Symfony) that if
hasSession()
returnstrue
, thengetSession()
will not returnnull
.Update: I checked with Drupal 8.4.x, and the code above has not changed in
symfony/http-foundation
v3.2.8.Comment #24
benjifisherI 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.
Comment #26
benjifisherI 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.
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #28
benjifisher@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.
Comment #29
andypost@benjifisher in my comment I mean that session always exists in core & you need additionally to check for
\Symfony\Component\HttpFoundation\Session\SessionInterface::isStarted()
Comment #30
benjifisher@andypost I am afraid I do not understand what you mean. According to the error message (before the patch)
getSession()
is returningnull
. So what exactly do you mean by "session always exists in core"?Remember, we only see this error when using CLI tools (
drupal console
anddrush
).I am pretty sure that the patch works: if
getSession()
returnsnull
, thenhasSession()
returnsfalse
, so this should work:My only quibble is that I would turn it around:
or maybe even a ternary
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commentedReally sorry, I must've been half asleep when I did the re-roll. Here is a re-rolled version of the patch in #13.
Comment #32
benjifisherI 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
withUnitTestCase
).Based on my review in #23 above, I am marking this RTBC.
Comment #33
benjifisher@andypost: I noticed the following code in
core/lib/Drupal/Core/StackMiddleware/Session.php
: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.
Comment #34
alexpottI 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.Comment #35
benjifisher@alexpott So you want to remove the
$cacheContext
property from this test class, remove the line that sets it in thesetUp()
method, and then doin each test method, and replace
$this->cacheContext
with$cacheContext
in those methods?If I have it right, then this looks like a novice task.
Comment #36
alexpottIt 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.Comment #37
benjifisherSorry, I misunderstood. I guess what I said was half right, or maybe 1/3 right.
$cacheContext
property from this test class.$this->cacheContext
with a local variable$cache_context
in each test method.Comment #38
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is an updated patch and interdiff as per comment #37
Comment #39
benjifisher@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.
When you remove the property, you should also remove its code comment. After applying your patch, I see this in the code:
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.)Comment #40
anya_m CreditAttribution: anya_m as a volunteer and at Skilld commentedComment #41
anya_m CreditAttribution: anya_m as a volunteer and at Skilld commentedComment #42
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commented@benjifisher, I have provided the interdiff.txt in #38
Comment #43
benjifisher@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.
Comment #44
alexpottThis is not actually part of this issue. As @larowlan noted in #9
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.
Comment #45
benjifisherI 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.
Comment #46
benjifisherNew 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.
Comment #47
andypostFollow-up filed & all feedback addressed
Comment #48
benjifisherThanks, @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.
Comment #50
benjifisherYay, the test-only patch failed in the expected way. Back to RTBC as in #47.
Comment #51
alexpottI'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.
Comment #52
benjifisher@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.
Comment #54
andypostrtbc again)
Comment #55
alexpottTicking reviewer credit boxes for reviewers that have influenced the direction of the patch.
Comment #56
alexpottCommitted and pushed 0c0c63e9af to 8.5.x and 3c7f090ea2 to 8.4.x. Thanks!
Comment #59
benjifisherYay! 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.
Comment #61
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commented