Don’t pass Traversable as ArrayUtils::merge parameter #93
Conversation
If session data retrieved from $_SESSION (in SessionManager::start) was instance of Traversable, it would be incorrectly passed to ArrayUtils::merge, as that method accepts only plain arrays.
|
Needs tests |
|
Would gscscnd/zend-session@9c5a1e75237a9997375cc9e9b821fe2ca8fe5c55 be enough or more tests are needed? |
src/SessionManager.php
Outdated
| $oldSessionData = $oldSessionData->toArray(); | ||
| } | ||
|
|
||
| if (! empty($oldSessionData) && is_array($oldSessionData)) { |
There was a problem hiding this comment.
We can have here just if ($oldSessionData && is_array($oldSessionData)) {
| @@ -130,9 +130,13 @@ public function start($preserveStorage = false) | |||
|
|
|||
| session_start(); | |||
|
|
|||
| if ($oldSessionData instanceof \Traversable | |||
There was a problem hiding this comment.
Looks like this condition is removed. What then in case when $oldSessionData instanceof \Traversable (=== true) ?
There was a problem hiding this comment.
May I ask what should be done in such case? Is there a PHP (or ZF) function that converts Traversable to array or should I write something like:
if ($oldSessionData instanceof \Traversable) {
$oldSessionDataArray = [];
foreach ($oldSessionData as $oldSessionDataKey => $oldSessionDataValue) {
$oldSessionDataArray[$oldSessionDataKey] = $oldSessionDataValue;
}
$oldSessionData = $oldSessionDataArray;
}Update: iterator_to_array would probably do the job.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, I’ve updated the patch.
There was a problem hiding this comment.
One more thing to note: iterator_to_array is not recursive
@gscscnd Can you provide an example when $_SESSION is not an array?
There was a problem hiding this comment.
That is, before being passed to ArrayUtils::merge? I’ve encountered #10 when implementing “remember me” functionality — I had to retrieve SessionManager and I wasn’t able to do it until I’ve provided the following (minimal) configuration:
return [
'session_config' => [],
'session_storage' => ['type' => Zend\Session\SessionStorage::class],
];What happened is probably the same as described in #10. Yet another workaround: 8trust/zend-session@c5e523a
There was a problem hiding this comment.
@gscscnd thanks, now I see the issue, basically it's not possible to use other session storage than (session)array(storage), right?
There was a problem hiding this comment.
I haven’t tried all storage variants, but it seems SessionManager is not fully compatible with SessionStorage (and perhaps not only this), so either of them should be somehow fixed 😕
Update: SessionArrayStorage seems to work for me. But I don’t think it’s an acceptable solution — while the docs say that SessionStorage may be incompatible with 3rd party libraries, it should be compatible with zend-stdlib, shouldn’t it?
src/SessionManager.php
Outdated
| if ($oldSessionData instanceof \Traversable | ||
| || (! empty($oldSessionData) && is_array($oldSessionData)) | ||
| ) { | ||
| if ($oldSessionData instanceof Storage\StorageInterface) { |
There was a problem hiding this comment.
I think I don't understand something....
Shouldn't we just have here: if ($oldSessionData instanceof \Traversable) { instead?
There was a problem hiding this comment.
toArray is defined in StorageInterface and not in the other interfaces.
There was a problem hiding this comment.
Yes, see my above comments #93 (comment)
|
Sorry, I didn’t notice this PR is aiming to do the same as #57 |
$_SESSION is populated with StorageInterface (and then Traversable). After start(), $_SESSION is checked whether it is an array and if still contains initial data.
src/SessionManager.php
Outdated
| $oldSessionData = iterator_to_array($oldSessionData); | ||
| } | ||
|
|
||
| if (! empty($oldSessionData) && is_array($oldSessionData)) { | ||
| $_SESSION = ArrayUtils::merge($oldSessionData, $_SESSION, true); |
There was a problem hiding this comment.
How it's going to work? As I can see merge has type hint on param 1 and 2 (array).
https://github.com/zendframework/zend-stdlib/blob/43d209995f054fa62557652192c4943625b4145a/src/ArrayUtils.php#L269
Your fix is in case when $_SESSION is NOT an array but above you pass (not an array) as the 2nd param?
There was a problem hiding this comment.
If I understand the code correctly, the new $_SESSION is passed to merge() after session_start() was called. Have a look at the original code — it’s assumed (I guess so, because there’s no validation) that the new $_SESSION is something acceptable by merge. session_start() somehow modifies the $_SESSION superglobal.
I should move that session_start() call below my new code to make it easier to understand, shouldn’t I?
Update: moved.
Moved code that conditionally converts data fetched from $_SESSION to plain array a few lines up for better readability.
…bility Don’t pass Traversable as ArrayUtils::merge parameter Conflicts: src/SessionManager.php
|
Thanks, @gscscnd; I have merged your changes along with #57 to provide a comprehensive fix. |
|
@weierophinney, I’m not entirely sure if https://github.com/zendframework/zend-session/blob/master/src/SessionManager.php#L142-L144 is completely correct:
Update: #96 apparently aims to solve these problems. |
SessionManager::start()retrieves data from$_SESSIONand passes it toArrayUtils::merge(). The problem is that the retrieved data can be either:array— that’s not a problem 🙂;Traversable(specifically, it should be instance ofZend\Session\Storage\StorageInterface), but such type is not acceptable asArrayUtils::merge()parameter 😞.The point of this PR is to convert such
Traversableto a plainarray, by callingtoArray()(defined in the mentionedStorageInterface) on it. I’ve created also an alternative PR inzend-stdlib(zendframework/zend-stdlib#86) that’d letArrayUtils::merge()acceptiterableparameters.