Follow-up of #2460833: _drupal_session_destroy() should return boolean
Problem/Motivation
When Drupal is not allowed to save the session, _drupal_session_write() returns nothing. This results into the following warnings in PHP 7:
Warning: session_write_close(): Session callback expects true/false return value
Warning: session_write_close(): Failed to write session data (user). Please verify that the current setting of session.save_path is correct
This happens in ± line 166:
if (!drupal_save_session()) {
// We don't have anything to do if we are not allowed to save the session.
return;
}
I found a similar situation in _drupal_session_destroy() but there TRUE is returned (± line 426):
// Nothing to do if we are not allowed to change the session.
if (!drupal_save_session()) {
return TRUE;
}
Steps to reproduce
drupal_save_session(FALSE);
session_write_close();
For background information, see also #2677808: compatible PHP7.
Proposed resolution
I think that _drupal_session_write() should return TRUE when it is not allowed to save the session, as that also happens in _drupal_session_destroy() in a similar situation.
Patch will follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | drupal-session-write-return-bool-2761285-2.patch | 444 bytes | megachriz |
Comments
Comment #2
megachrizComment #3
fabianx commentedRTBC - looks good to me.
Comment #4
oriol_e9g+1 RTBC
Comment #6
stefan.r commentedCommitted and pushed to 7.x, thanks!
Comment #7
David_Rothstein commentedHm, that is unfortunate - the documentation at http://php.net/manual/en/function.session-set-save-handler.php certainly implies that a return value is required for the "destroy" handler but not for the "write" handler. I think that's why changes to this function (which were originally in the earlier patch) were removed as that issue progressed... makes me wonder if there are any others lurking that need to return something too but that is not mentioned in the PHP documentation.
I think I'd be in favor of getting this into the 7.50 release. It's very simple and essentially zero risk, plus it's a followup to something we thought we fixed already.
Comment #8
David_Rothstein commentedCrosspost - sorry! All good :)
Comment #9
megachrizGreat! Thanks for committing. I think this was my first patch against D7 core.