_drupal_session_destroy() should return boolean

Looks good.

+1 to this patch as well, and it's very simple. Setting to RTBC

See https://www.drupal.org/node/2638016

Logic: I found zero 'google' or 'drupal' solutions until this post @ https://www.drupal.org/node/2638016

I spent 36 hours searching :)

Then my post was closed as a duplicate, despite the fact that google placed it on the first page?

@Peter Bowey your issue has better SEO and ranks better in google but usually the older issue is the one that sticks, and new ones are closed as duplicate. Only when an issue has clearly more activity (more information, patches, reviews, etc) it can be kept instead of the older one.

I started using PHP 7.0.1 few days ago and since then I noticed PHP notice "Warning: session_destroy(): Session object destruction failed in drupal_session_commit() (line 314 of ....../includes/session.inc)."

I tried truncating the session table and some other attempts but none worked. Finally I started looking at PHP documentation, and, as mentioned by the original post, the customized function _drupal_session_destroy() was not returning a boolean value back as should.

Finally I made a patch quite similar to the original post. I'm not seeing the error being reported on my production server since then.

Highly suggest to get the patch included in next Drupal release. Thanks.

Changing this back to 'Needs Work' as #2638016: Drupal 7.51 + PHP 7 (release) user_logout() session_destroy() expects true/false return highlights other areas we need to return a Boolean

The patch is working for me.

Here is a new patch. It is a re-roll of patch on #11 because it was no longer applying. But also we now return false in some situations (when the old code returned null) because it seemed like the correct thing to do since NULL == FALSE and those situations were actually result of not being able to save the session.

Seems like there was an error on the automatic testing. Settings to needs review to try again.

The last patch broke Drupal for me so seems something is wrong with the logic and not the testbot?

The patch in #2460833-2: _drupal_session_destroy() should return boolean resolved specific php 7 issue for me ^_^

Let's up this to major (at least) since it's the main thing which prevents tests passing on PHP 7. See #2656548: Fully support PHP 7.0 in Drupal 7.

@David_Rothstein, how can we run the tests again? Seems like last time there was an error on the testing bot, not in the patch itself....

Ok, I see the error..... There was a syntax error in there.

Test results showing no session_destroy() failures, setting status for committer consideration.

Upon further reviewing this patch I found that some return statements were changed on functions that are not part of the session_handlers. The session_handlers functions can be recognized because the start with _ and because they are explicitly declared here:

session_set_save_handler('_drupal_session_open', '_drupal_session_close', '_drupal_session_read', '_drupal_session_write', '_drupal_session_destroy', '_drupal_session_garbage_collection');

Inside the `drupal_session_initialize` function. Only those functions need to return TRUE or FALSE in PHP 7, so I removed the changes to the other functions in this new patch.

As per #24, and the issue title. These patches are changing too much.

The session callbacks that need to return a boolean are:

• _drupal_session_open()
• _drupal_session_close()
• _drupal_session_destroy()
• _drupal_session_garbage_collection()

This means the only thing that needs to be fixed, as the title says, is _drupal_session_destroy().

Uploading new patch, since #24 missed some extra changes.

What @twistor said, just the two return statements, see http://php.net/manual/en/function.session-set-save-handler.php

I have no idea what return FALSE will do, but drupal_save_session() is just static internal flag. Based on other code in core;

    if (!drupal_save_session()) {
      // We don't have anything to do if we are not allowed to save the session.
      return TRUE;
    }

As it is used for impersonating users, etc,

  global $user;
  $user = user_load(1);
  drupal_save_session(FALSE);

So it isn't actually an error / failure here, I believe.

   // Nothing to do if we are not allowed to change the session.
   if (!drupal_save_session()) {
-    return;
+    return TRUE;
   }
 

Looks like all errors aren't related to this patch ...

Is there a solution for this error?

For now I am allowing PHP 7 to fail till I can take a better look at this. Putting out other fires associated with D7.43.

As per #32, this lighterweight version does the originally intended job.

I am writing a module right now that runs tests using the drupalLogout() in DrupalWebTestCase, previously this was of course throwing exceptions for PHP 7. I just added this script to my Travis CI tests and was able to get a pass.

https://travis-ci.org/taz77/drupal-facebook_tracking_pixel/jobs/112011755

+1 RTBC

Although oddly enough I started getting failures in PHP 5.6 after applying this patch in my tests for my module, investigating.

I just added 5.6 testing to the patch to see what happens here and requeued the other tests.

I really do not understand how/why the patch is failing testing.

My failure seems to be with an assertion in the module I am writing. I don't think it's associated with this patch at all.

Wonderful. This should be part of the next stable core release. Confirming RTBC and thank you very much for your work!

Works for me with PHP 7. Let's get it committed!

Tests still appear completely unrelated.

Been running this on at least 5 clients sites (PHP 7 on UNIX) without issue for months now :)

Tested patch #30, works flawlessly. Please get it committed.

Workss with php7 !!! Comitte Please.

Patch at #30 fixes this on two Drupal 7.43/PHP 7.0.x setups for me. Thank you very much. Please commit.

Commit please. There is still (and will be forever) some Drupal 7 people here.

Commit please :-) Still a lot of Drupal7 users here

+1 to RTBC

Assigning to maintainer.

Rooting for a commit!

Thank you Alan.
I had the same problem since the new installation of Ubuntu 16.04 and PHP 7.0.4-7 and MySQL 5.7.12-0 ( But not with Ubuntu 15.10 was with PHP 5.6.11 and MySQL 5.6.30 )
It's now resolved with Ubuntu 16.04.! Thank you so much!

The patch looks good -- this definitely looks on track to be committed and included in the next release.

For those interested in PHP 7, note that there are several other issues that still need reviews - see #2656548: Fully support PHP 7.0 in Drupal 7 for details. Hopefully we can get those all into the next release so that all Drupal core tests pass on PHP 7.

Woot!

Committed to 7.x - thanks!

Thank you very much.