Not sure if I miss anything else.

You have removed all calls to code handling the session cookie. This is the part you are missing in validating that your fix has no side effects. I also have no idea what the "Re-register the session store after a session has been destroyed, to avoid PHP bug" comment refers to (and the commit introducing it has no contextual value) but I'd suggest this could imply there either was a PHP bug that was fixed or an active issue regarding a call to session_set_save_handler() that needed to be looked at here.

You have removed all calls to code handling the session cookie

There are two reasons I removed it:

1. The warning message from PHP tells us to not do that

session_set_cookie_params(): Cannot change session cookie parameters when session is active in

2. As I understand, it is set here https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/session/handler/joomla.php#L59 already

For the comment in the code Re-register the session store after a session has been destroyed, to avoid PHP bug, I am unsure too as before that, we already check and only run other code if session is active https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Session/Session.php#L794

@johanjanssens I don't suppose you can provide any insight here :)

I get what you're trying to do, but you also said "I'm not entirely sure about this" basically and just looking at the code and seeing what's removed all I can comment on is possible side effects that need to be looked for.

Yes, It is just an attempt and unsure PR as I mentioned. Hopefully, we can come up with a proper fix as it is not nice to see warning like that. It is not only logged in error logs but I also see it on the screen when I open the site after closing it sometime (guess that's the time Joomla logins by cookie)

@mbabker @wilsonge I think I know the reasons of the strange code now. If you view the code from Johan commit

That explains why he had to call $this->_handler->register(); again with that comment in the code. I guess later, when someone refactored the code, he forgot to remove these not necessary code.

So here is the summary:

1. We don't need to call session_set_cookie_params again because from PHP documentation:

Set cookie parameters defined in the php.ini file. The effect of this function only lasts for the duration of the script. Thus, you need to call session_set_cookie_params() for every request and before session_start() is called.

We already call that method here https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/session/handler/joomla.php#L59 when initialize application, so no need to call it again when login happens

2. The code $this->_store->register(); can be removed because:

• PHP tells us to do that
• We now know the reason why the code is there (original code call session_destroy()) in fork() method

I think this change is valid now. If people want to test it, what do they need to test to make sure this change doesn't cause any side affects:

1. Login, logout works

2. Session data before login is kept after login (I am not sure how to test it using Joomla core, I can only test it in my own extension)

Update description: Create a new session and copy variables from the old one

Could you please explain more about your last comment? it is not clear to me

See line 786 for description of this method. Is this copy variables from the old one still applicable with the deleted lines?

To me, the deleted lines were there by mistakes, see my earlier comment #19199 (comment) . So the description of the method should remain unchanged

I have tested this item ✅ successfully on 3fb31b9

It appears to be working. I see no side effects.

I have tested this item ✅ successfully on 3fb31b9

Tested on php 5.6.33, 7.0 and 7.2

Ready to Commit after two successful tests.

In my humble opinion, this fix should be merged asap and release J3.8.4
This is a very annoying php warning for those using PHP 7.2

I have tested this item ✅ successfully on 3fb31b9