Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] fix session_write_close on php7 #19040

Closed
wants to merge 1 commit into from
Closed

[5.4] fix session_write_close on php7 #19040

wants to merge 1 commit into from

Conversation

leo108
Copy link
Contributor

@leo108 leo108 commented May 3, 2017

fix issue #15717

@leo108 leo108 changed the title fix session_write_close on php7 [5.4] fix session_write_close on php7 May 3, 2017
@taylorotwell
Copy link
Member

Things like this have been PR'd and reverted several times before: #16251

It would be nice to get to the bottom of it when someone has time.

@bosschuyler
Copy link

bosschuyler commented Apr 7, 2018

I have looked into this @taylorotwell and can verify @leo108 has a fix for this.

The problem is that php7 and above expects a boolean return for the write() function:
true for if the write succeeded
false if there was an error (this will throw the internal error)

The CacheBasedSessionHandler is passed an instance of Illuminate\Cache\Repository which is used in the CacheBasedSessionHandler::write() method. The cache method used to store the data is Illuminate\Cache\Repository::put(). put() returns void which is actually NULL, but php7 evaluates that as a false and throws the internal exception which becomes a warning level, thinking that the write to the session store has failed.

After this PHP takes over and I believe it falls back to the ini configuration for saving a session, which fails again cause the file doesn't exist, was never opened and likely is looking in the wrong directory altogether.

in PHP 5.6 this wasn't an issue because it didn't have as strict requirements to adhere to the interface of the SessionHandlerInterface, now it's expecting it plus handles the response and the CacheBasedSessionHandler doesn't provide the proper response, leo's solution would at least suppress errors from being thrown incorrectly.

It would probably makes sense to throw a database error if the cache ever fails to save the session data to know there was an issue (maybe it already does and it's never failed to date for me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants