Comments

colinmccabe’s picture

Status: Needs review » Active
colinmccabe’s picture

FileSize
770 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,170 pass(es). View
colinmccabe’s picture

Status: Active » Needs review
colinmccabe’s picture

Issue summary: View changes
checker’s picture

Issue tags: +PHP 7

Looks good.

brianV’s picture

Status: Needs review » Reviewed & tested by the community

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

Peter Bowey’s picture

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?

jackbravo’s picture

@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.

deminy’s picture

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.

brianV’s picture

Status: Reviewed & tested by the community » Needs work

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

checker’s picture

checker’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: drupal-session_destroy_return_bool-2460833-3-D7.patch, failed testing.

Neo13’s picture

The patch is working for me.

jackbravo’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 15: drupal-session_destroy_return_bool-2460833-15-D7.patch, failed testing.

jackbravo’s picture

Status: Needs work » Needs review

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

sylus’s picture

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 ^_^

David_Rothstein’s picture

Priority: Normal » Major

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.

jackbravo’s picture

@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....

Status: Needs review » Needs work

The last submitted patch, 21: drupal-session_destroy_return_bool-2460833-21-D7.patch, failed testing.

MustangGB’s picture

Status: Needs work » Reviewed & tested by the community

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

jackbravo’s picture

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.

twistor’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 25: drupal-session_destroy_return_bool-2460833-25.patch, failed testing.

The last submitted patch, 25: drupal-session_destroy_return_bool-2460833-25.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
Alan D.’s picture

Status: Needs review » Needs work

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;
   }
 

Status: Needs review » Needs work

The last submitted patch, 30: drupal-session_destroy_return_bool-2460833-30.patch, failed testing.

MrHaroldA’s picture

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

Paulset’s picture

Is there a solution for this error?

Perignon’s picture

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.

MustangGB’s picture

Status: Needs work » Reviewed & tested by the community

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

Perignon’s picture

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

Perignon’s picture

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.

Perignon’s picture

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.

Anybody’s picture

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

DamienMcKenna’s picture

Assigned: colinmccabe » Unassigned
joelstein’s picture

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

Alan D.’s picture

Tests still appear completely unrelated.

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

W.M.’s picture

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

ogursoy’s picture

Workss with php7 !!! Comitte Please.

jant’s picture

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

Crell’s picture

W.M.’s picture

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

Rodeo.be’s picture

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

Fabianx’s picture

Assigned: Unassigned » David_Rothstein

+1 to RTBC

Assigning to maintainer.

Perignon’s picture

Rooting for a commit!

vcouver’s picture

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!

David_Rothstein’s picture

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.

stefan.r’s picture

Perignon’s picture

Woot!

stefan.r’s picture

Assigned: David_Rothstein » Unassigned
stefan.r’s picture

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed to 7.x - thanks!

  • David_Rothstein committed 8472395 on 7.x
    Issue #2460833 by jackbravo, colinmccabe, checker, Alan D., twistor:...
W.M.’s picture

Thank you very much.

Status: Fixed » Closed (fixed)

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