Session Save in DB Causes Error (After Fresh Installation)

[Sdfendor]

What I did

1. Cloned the project
2. Created a working DDEV environment
3. Openend & Followed the Installation Wizard and selected database as session save type (this is not the default but file system is)
4. Tried to go either to adminhtml or frontend
5. Get presented with:
   

Analysis

Error happens during session start. I followed a stack trace a bit further down and the relevant part of it is app/code/core/Mage/Core/Model/Session/Abstract/Varien.php:153

The value of session_start is indeed false so the exception is thrown. Why is that?
Let's have a look into the DB session handler in app/code/core/Mage/Core/Model/Resource/Session.php. Following SessionHandlerInterface, the open() method is called first, which in this case just returns true and then a read() happens for a given session ID:

At this freshly installed state (and if it is a new session ID or the session is expired) the fetchOne() in the last line returns false (look at the where clauses used).
The typing of the method explicitly takes the false into account but herein lies the problem.

The change has been introduced to OM in version 21 with the PR OpenMage/magento-lts#3499. I guess before this method already returned false which then implicitly was cast to an empty string. By that the exception was not thrown. I noticed right now that the previous behavior had a string cast in it.

Conclusion

The session save to DB currently doesn't work. As a consequence one idea for a solution could be changing the read() method's signature to just returning string and explicitly string cast the returned value (to make it clear that we are aware of the fact, that fetchOne() can return false and we don't want to do it implicitly by return type). I

I could prepare a PR if nothing speaks against this idea.

[justinbeaty]

You should be able to fix this by simply changing it to:

return (string)$this->_read->fetchOne($select, $bind);

I did this already in PR #36. See the comment here: https://www.php.net/manual/en/sessionhandlerinterface.read.php#128107.

However, I think the reason it happens in the first place is that you have an om_frontend cookie set already. If you cleared it, I don't think the problem would happen. Either way, it is a bug for sure.

Edit: I see you came to the same conclusion about casting to a string. That indeed seems to be the issue, but only because of the undocumented bug / feature in SessionHandlerInterface.

[justinbeaty]

Btw, if you'd like to test PR #36 I would be much obliged. 🙂

[Sdfendor]

Can you elaborate in which way this is undocumented behavior bug/feature? I double checked here: https://www.php.net/manual/en/sessionhandlerinterface.read.php

and it at least has not only the return type string|false but also states:
Reads the session data from the session storage, and returns the results. Called right after the session starts or when session_start() is called. Please note that before this method is called SessionHandlerInterface::open() is invoked.

session_start on the other hand only vaguely talks about "successfully started true/false" not what constitutes as a failure (not only open but also a false when reading).

Edit: it seems that one can't easily differentiate which part of the SessionHandlerInterface in the session_start leads to a resulting false, not with the current code that is.

[Sdfendor]

Btw, if you'd like to test PR #36 I would be much obliged. 🙂

I see what I can do.

[justinbeaty]

I say that it's a bug, because https://www.php.net/manual/en/sessionhandlerinterface.read.php says:

If nothing was read, it must return false.

Which would lead you to believe that if there's no session data for that session id, you should return false.

But the user contributed note says:

It appears that, if this function returns false, it causes "session_start(): Failed to read session data: user" to be emitted in the error log. This also seems to have a cascading effect that causes the write() method not to be called.

So, returning false appears to only be for indicating an error state. To indicate that there is no existing session, but that it is okay to create one, it appears that returning an empty string is the way to go.

Which aligns with what is happening here. Returning false causes session_start to fail instead of just populating an empty $_SESSION variable.

I think that the PHP intends you to use this interface in addition: https://www.php.net/manual/en/sessionupdatetimestamphandlerinterface.validateid.php

Then, it would first call to validateId(), where we would return false, and PHP would generate a new session id, assuming you have session.use_strict_mode enabled. Although likely you'd still have to return a string from read(), but not sure as I haven't tried it. Edit: or no, because if a new session was created, then no call to read should happen I think...

[Sdfendor]

So maybe let's first go with the fix idea we both had and add a todo? And later explore and idea and perhaps implement the other interface as well validating a session ID prior to read.
The return values text of read() is clearly misleading here, I agree.

[justinbeaty]

Yeah, I had planned to explore the new interface later, because I also wanted to improve the GC routines for both db and files handler. But don't let that prevent you from making a PR if you want to.

Plus, store owners are probably better off not accepting uninitialized session ids anyway...

[fballiano]

Hi guys and thanks for checking it out, I just wouldn't stop #36 waiting for this if it's not absolutely necessary :-)

[justinbeaty]

Hi guys and thanks for checking it out, I just wouldn't stop #36 waiting for this if it's not absolutely necessary :-)

It doesn't block anything. The string casting fix is already in #36. Later on we can investigate the validateId() method.

[fballiano]

@Sdfendor now that #36 was merged, could you retest please?

[Sdfendor]

I will have a look tomorrow but because of the discussion in this issue I'm quite optimistic that it works. I'm going to close the issue after the check tomorrow.

[Sdfendor]

I see only one cookie as planned and the main branch works without my own fix applied 👍🏼.