performSaveHandlerUpdate #118
Conversation
dennybrandes
commented
Jun 30, 2019
•
dennybrandes
commented
- check with isString() before calling locateRegisteredSaveHandlers()
- fixed SessionConfig -> setPhpSaveHandler() - don't locate registered save handlers if instance of SessionHandlerInterface #117
- check is string before calling locateRegisteredSaveHandlers()
There was a problem hiding this comment.
@dennybrandes First I was quite confused by these changes, and then I've changed view in github pr - to splitted and ignore whitespaces and all became clear.
LGTM, just small two comments.
src/Config/SessionConfig.php
Outdated
| } | ||
|
|
||
| if ((! class_exists($phpSaveHandler) | ||
| || ! (in_array(SessionHandlerInterface::class, class_implements($phpSaveHandler))) |
There was a problem hiding this comment.
I see this condition was like that before, but I think it can be simplify by using is_a method.
There was a problem hiding this comment.
I tried, but unit tests are not working after change. Need to investigate more. I will do it in another MR.
There was a problem hiding this comment.
Have you tried is_a($phpSaveHandler, SessionHandlerInterface::class, true) ?
I've seen you've tried instanceof but this is not gonna work.
There was a problem hiding this comment.
I tried with is_a but that will break unit tests. Maybe we can fix it in another PR/issue.
There was a problem hiding this comment.
I've checked and it works for me:
if (! class_exists($phpSaveHandler)
|| ! is_a($phpSaveHandler, SessionHandlerInterface::class, true)
) {There was a problem hiding this comment.
You are right, Thanks. Its changed
…p' into feature/master/25-config-speed-up
|
@webimpress can you check again please? :-) |
src/Config/SessionConfig.php
Outdated
|
|
||
| if (! class_exists($phpSaveHandler) | ||
| || ! (in_array(SessionHandlerInterface::class, class_implements($phpSaveHandler))) | ||
|
|
There was a problem hiding this comment.
Remove empty line above, please
There was a problem hiding this comment.
I removed the empty line
| $this->phpErrorMessage | ||
| implode(', ', $knownHandlers), | ||
| SessionHandlerInterface::class, | ||
| SessionHandlerInterface::class |
There was a problem hiding this comment.
Above line can be removed, too many params.
|
@webimpress i added my change now to changelog, but maybe you need to change the release date there :-) |
…eed-up performSaveHandlerUpdate
|
Thanks, @dennybrandes ! |
