session_id() cannot be changed after session is started

Copying patch from #3012308: D7: Fully support PHP 7.3 and #3009351: "session_id(): Cannot change session id"

@Fabianx I don't see any file.module change which is relevant.

Fabianx, when you or Pol commit this, reminder to add php 7.3 automated test for core commits https://www.drupal.org/node/3060/qa

How can we get this committed? I can confirm that this works well and haven't seen any issues as of yet.

Ideally we could have one more person give this a thorough review and mark it RTBC?

I can't give thorough review, but I am successfully using this patch.

We have a client looking for support for this, so I can test this patch.

I don't see any comments in this ticket about how to identify what is being resolved, but I'll look at the code and see if I can figure out a way to confirm testing.

@wylbur I updated the issue summary so hopefully testing/reviewing is easier.

The parent issue is critical so upping the priority in hopes of getting this reviewed.

The patch works well, kindly commit it.

The patch works for me.

Fabianx I looked at file.module change from D8 #2 #3025335-2: session_id() cannot be changed after session is started

We should btw. also port the file.module change from D8

but there's nothing at all similar in the D7 version, I don't think there's anything to do in file.module.

RTBC!

I've been searching for a fix for this. We are seeing frequent log outs of authenticated users and numerous warning msgs in dblog ever since upgrading to PHP 7.2. Now waiting for 7.68 to drop... Thanks for your work!

Thanks all for the reviews it seems like this is ready to go in... :)

Please, lets get this in so we can start working on php 7.4.x support. php 7.4.x is scheduled to be released shortly.

I am seeing this working fine in our dev environment

RTBC+1

This is urgent.
Dries Buytaert the founder and eternal leader of Drupal is pushing for all of contrib and for core to support current versions of PHP.

see his post here: https://dri.es/the-end-of-php-5

This will encourage hosting providers to upgrade, clients to push hosting providers to upgrade versions of php, and will facilitate the adoption of Drupal 9 once it is released. The faster we get Drupal 7 ready for php 7.3.x and 7.4.x the easier it will be for everyone to run Drupal 9 which will require PHP 7.1.x as a recommended minimum (perhaps even 7.2.x).

This is a large ecosystem of hosting, clients, demand, critical mass, lets push it forward.

Patch in #3 worked great. Thank you.

Just tested it as well. Working fine.

I'd like to have a review from @Fabianx as well.

been running hard with php 7.3.x every day for the past 4 weeks in 4 different environments, one windows, 3 Linux/Apache, no issues with this patch.

still running hard with this patch, zero issues. If you have a suggestion for improving this patch, speak up now or never. Because once this goes in, it's never ever going to get changed, code written in stone FOREVER (until PHP 7.4.x that is)

@Pol , is there any reason to hold off on this other than waiting on another big name reviewer?

Hi,

I submitted the patch to my colleagues in the core team, I'm waiting for them to take actions.

This patch does work like a charm with upcoming PHP7.4 as well (at least with tests), see #3081386: [META] Fully support PHP 7.4 in Drupal 7

+1 RTBC

@FabianX, @pol, @Taran2L is using this patch with his php 7.4.x fixes and has fixed D7 core compatibility with php 7.4.x there's nothing left to do here except push this patch into the 7.x-dev branch

There's been enough reviews go in, jam this into the dev branch, so that all of contrib can test it,

Is there some sort of core policy that is preventing action here?

If you need someone to rattle the cage, I've requested core maintainer access, I'd be willing to move things forward and get us out of this 3 month stagnant rut.

#3086286: Offering to maintain Drupal core (@joseph.olstad)

I did a thorough review of this patch and what they did in D8.

From my RTBC comment #3025335-20: session_id() cannot be changed after session is started

FabianX I looked at file.module change from D8 #2 #3025335-2: session_id() cannot be changed after session is started

We should btw. also port the file.module change from D8

but there's nothing at all similar in the D7 version, I don't think there's anything to do in file.module.

I will add, existing test coverage is sufficient, there does not need to be tests written for this.

#3047844: [Regression] Tests fail on PHP 5.3

See my comment #37

Justification for Pending Drupal 7 commit: We need this in so that we can re-queue any patches against php 7.3.x before committing to core.

1) I'm not keen on this variable name:

+++ b/includes/session.inc
@@ -371,10 +371,20 @@ function drupal_session_regenerate() {
+    $original_session_saving = drupal_save_session();

How about something like $original_save_session_status which makes it clearer (to me at least) what we're storing to restore later on?

2) Is there an existing core test which fails under PHP 7.3 without this patch?

3) In the absence of a failing test which this fixes, what are the manual steps to reproduce the problem we're fixing here?

session regeneration fails when logging in if an anonymous session was already active.

...is in the IS, so that gives us some idea.

This is getting ridiculous. PHP 7 has been out a long time. The message is clear get used with the new D8+ system with it's quirks and vision or quit Drupal.

It is very sad your are postponing the patch for some kind of variable name issue again... We are using that patch for more than 6 months without problems.

Keep in mind that PHP 7.2 will go to "security fixes only" phase in less than month and then Drupal 7 will not support any actively developed version of PHP (only old versions). That is very sad! Hosting providers would not wait for Drupal community to fix these issues. They will upgrade PHP to newer versions and broke Drupal 7 installations. If that is what you want, then OK.

Here's a new core test for the user module which should fail in PHP 7.3 without the patch, with the relevant error message:

session_id(): Cannot change session id when session is active

With the patch in place, this test passes.

I've also changed the variable name per #40.

I've added the new test to the existing UserLoginTestCase and that means adding a little bit of overhead as we have a test module to create the anon session, and this is now enabled during set up for the other tests in the same class. We could split the new test out into its own class to avoid this, but it seems like the right place for it to go, and I doubt enabling this simple module adds significant cost to the other tests.

We expect the test_only patch to fail (with PHP 7.3).

Ok, looks like there are several core test failures under PHP 7.3 as a result of this issue.

Tests pass with the patch in place, including the new testLoginWithAnonSession which did indeed fail for test_only.

Thank you for all the feedback that this patch has had D7 sites running successfully under PHP 7.3 for some time; that's very useful to hear.

Back to RTBC.

$original_session_saving is used elsewhere in core so that's how I chose it - but I'm totally flexible on that. If we want to bikeshed variable names: Elsewhere in this same function we see $account = $user rather than $original_user = $user - some might prefer we use $account for consistency...

Yes there are numerous failing tests, thus I didn't add one initially.

@mcdruid, please procede with this patch anytime, at least in the dev branch , so that there's less confusion with test failures. I'm working on php 7.3 now and there's some noise failures that would be resolved by this patch.

there's also a php 5.3 patch to go in.

Thanks!

@mfb ah, I see $original_session_saving in drupal_cron_run() now. Sorry, I should have checked. I'm still not keen on the name though tbh :)

Here's a patch as outlined by @Fabianx where we move the new logic to a new internal helper function, and spell out what's going on.

The overall logic is unchanged, and the new test passes for me on PHP 7.3; let's see if the testbot agrees this still fixes the issue.

Oops, looks like it is; added a call to session_id() from the new helper.

Good; that was the expected / desired result.

Back to RTBC.

+  // Preserve and restore user object, as starting a new session will reset it.

It should be Preserve and restore the user object, as starting a new session will reset it.

You're right; well spotted, thanks.

I think we can fix that before commit, so no need for more patches - unless there's anything more serious we need to change.

@kiamlaluno , in this case I would leave out 'the' , otherwise we go beyond 80 characters and have to add a new line.

Ce patch est bon tel quel!

@joseph.olstad The line length is not more important than grammar. A line more doesn't make any change.

'the' it is. Thy grammar hast commeth firstly.

Couple of comment tweaks on commit - one was a copypasta error in the new test's dummy module.

We can probably enable testing with PHP 7.3 for the 7.x branch now that this is in, which is great!

Thank you everyone who contributed!

\o/ my 2 cents is it'd be worth putting out a release nowish to resolve PHP compatibility issues (given that PHP 7.1 only has security support for another 23 days), and follow up with another bugfix release soon after for other issues being worked on.

Fabianx and mcdruid are on a roll, I'd say let them keep the momentum going for a while yet before cutting a release. We've got a large inventory (backlog) of great fixes and improvements ready or very close to ready for commit, lots of issues marked as 'pending commit'. Still a lot more to go into a release, I am liking the recent activity would be a shame to stop prematurely.

Meanwhile I've put php 7.3 through it's paces, and I am pretty certain that I found a bug in php 7.3.8.1 it'self that manifests it'self in the unserialize() function. I'm using php 7.3.8.1 , hope to upgrade to 7.3.9.0 soon and put it through its paces, see if it fixes the bug I found. The release notes changelog for php do not indicate explicitly a fix but maybe implicitly through another fix somewhere else deep in the innerds of php.

php 7.4.0 hasn't been released yet, we should continue pushing forward, I have so far spent a lot of time on the unserialize() bug I found in php 7.3.8.1 and have not yet found a resolution. Maybe php 7.4.0 will fix this if php 7.3.9.0 doesn't.

with that said, this bug I found shouldn't slow down our effort to approach php 7.4.0 compatibility, @Taran2L basically did a one man hero job on that so far fixing everything via patches that he's provided for us to review.
here's a description of the bug (99% sure it's a bug in php 7.3.8.1) #2274543-108: [7.x-1.x] Remove usage of deprecated create_function()

Yes this sounds like the known issue of PHP's broken serialization, which had silent errors in older versions, and which Drupal 8 has had to laboriously work around (via DependencySerializationTrait etc.)

Brief: @mfb , I followed your suggestion and created a child D7 issue from that
You might have an idea for this, have a look. #3093110: D7 - EntityType objects cannot be reliably serialized without DependencySerializationTrait