Ext session refactoring part 1 (session_set_save_handler() and mod user) #9781
Conversation
| public function open($path, $name): bool { | ||
| ++$this->i; | ||
| echo 'Open ', session_id(), "\n"; | ||
| echo 'Open:', session_id(), "\n"; |
There was a problem hiding this comment.
Not sure whether we really want to add these colons; this doesn't appear to clarify the tests, but might cause merge conflicts in the future. If we'd change to var_dump(session_id(), that might make more sense, although it'd increase the possibility of merge conflicts.
There was a problem hiding this comment.
This was to get rid of the whitespace sensitivity for the tests, as my IDE loves removing the trailing whitespaces...
| class Kill { | ||
| function __construct() { | ||
| global $HTTP_SESSION_VARS; | ||
| $_SESSION = []; |
| ?> | ||
| --CLEAN-- | ||
| <?php | ||
| $path = __DIR__ . '/session_module_name_variation4'; |
| <?php | ||
| $save_path = ini_get("session.save_path"); | ||
| if ($save_path) { | ||
| if (!file_exists($save_path)) { | ||
| die("skip Session save_path doesn't exist"); | ||
| } | ||
|
|
||
| if ($save_path && !@is_writable($save_path)) { | ||
| if (($p = strpos($save_path, ';')) !== false) { | ||
| $save_path = substr($save_path, ++$p); | ||
| } | ||
| if (!@is_writable($save_path)) { | ||
| die("skip session.save_path $save_path is not writable\n"); | ||
| } | ||
| } | ||
| } | ||
| ?> |
There was a problem hiding this comment.
Isn't this an exact duplicate of ext/session/tests/skipif.inc? If so, can't we use that file in the user_session_module tests?
There was a problem hiding this comment.
Yes, it is a duplicate, but I thought I could get rid of the skipif section all together and not rely on the file system but that doesn't seem likely so the easy fix was to just copy the file again so that I didn't need to amend all paths. But I can try again to get rid or point to the root SKIPIF file.
There was a problem hiding this comment.
Turns out it's completely useless, even for the normal session tests, as it's meant to check if the INI save path exists and is writeable, but this is never set in our tests.
|
|
||
| /* add shutdown function, removing the old one if it exists */ | ||
| if (!register_user_shutdown_function("session_shutdown", sizeof("session_shutdown") - 1, &shutdown_function_entry)) { | ||
| if (!register_user_shutdown_function("session_shutdown", strlen("session_shutdown"), &shutdown_function_entry)) { |
There was a problem hiding this comment.
There is no need to change strlen() to sizeof; any somewhat reasonable compiler is supposed to optimize strlen for literals.
There was a problem hiding this comment.
I went the other way, from sizeof to strlen
There was a problem hiding this comment.
D'oh! Makes sense, but maybe we should document that preference in our coding style guidelines.
Girgias
force-pushed
the
ext-session-refactoring-1
branch
from
bdac1dc to
737b322
Compare
| Validate ID | ||
| Read | ||
| array(1) { | ||
| ["OLD_ID"]=> | ||
| string(28) "a:1:{s:3:"foo";s:5:"hello";}" | ||
| } | ||
| Write | ||
| Close |
There was a problem hiding this comment.
This looks like a long standing bug to not clear the optional handlers. As this doesn't happen with the Object version.
Girgias
force-pushed
the
ext-session-refactoring-1
branch
3 times, most recently
from
cc18ef3 to
3397200
Compare
And small improvements to some
Use proper ZPP callables with FCI/FCC
Girgias
force-pushed
the
ext-session-refactoring-1
branch
from
3397200 to
74b11b3
Compare
2 participants
I started this 3 weeks ago and this is what led me through the whole FCI refactoring bit, but it's probably best to start having part of this already reviewed and merged.
The commits are self contained and should be reviewed independently (and also not squashed).
I've got more ideas for changes lined up, but those will require an RFC.