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.

Comments

MegaChriz created an issue. See original summary.

megachriz’s picture

Assigned: megachriz » Unassigned
Status: Active » Needs review
StatusFileSize
new444 bytes
fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks good to me.

oriol_e9g’s picture

+1 RTBC

  • stefan.r committed d2d2c5f on 7.x
    Issue #2761285 by MegaChriz: _drupal_session_write() does not always...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x, thanks!

David_Rothstein’s picture

Status: Fixed » Reviewed & tested by the community

Hm, 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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Crosspost - sorry! All good :)

megachriz’s picture

Great! Thanks for committing. I think this was my first patch against D7 core.

Status: Fixed » Closed (fixed)

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