Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Redis as save handler #98

Closed
2 tasks done
ikovalyov opened this issue Nov 29, 2017 · 5 comments
Closed
2 tasks done

Redis as save handler #98

ikovalyov opened this issue Nov 29, 2017 · 5 comments

Comments

@ikovalyov
Copy link

ikovalyov commented Nov 29, 2017

Provide a narrative description of what you are trying to accomplish.

Code to reproduce the issue

                $config->setOption("save_handler", "redis");
                $url = "tcp://" . $redisDto->getHost() . ":" . $redisDto->getPort() . "?auth=" . $redisDto->getPassword() . "&database=1";
                $config->setOption("save_path", $url);

Expected results

It should save data in redis

Actual results

Warning: session_start(): open(tcp://redis:?auth=&database=1/sess_***, O_RDWR) failed: No such file or directory (2) in ***/vendor/zendframework/zend-session/src/SessionManager.php on line 140

Here is what I found in your code:

    public function setStorageOption($storageName, $storageValue)
    {
        switch ($storageName) {
            case 'remember_me_seconds':
                // do nothing; not an INI option
                return;
            case 'url_rewriter_tags':
                $key = 'url_rewriter.tags';
                break;
            case 'save_handler':
                // Save handlers must be treated differently due to changes
                // introduced in PHP 7.2. Do not alter running INI setting.
                return $this;
            default:
                $key = 'session.' . $storageName;
                break;
        }

The 2.8.0 version looks like

    public function setStorageOption($storageName, $storageValue)
    {
        switch ($storageName) {
            case 'remember_me_seconds':
                // do nothing; not an INI option
                return;
            case 'url_rewriter_tags':
                $key = 'url_rewriter.tags';
                break;
            default:
                $key = 'session.' . $storageName;
                break;
        }

If I pass redis as save_handler it's just ignored and php trying to use my redis config as default config parameters. It broken my live servers. Reverted to 2.8.0 for now.

Please fix ASAP.

Thanks.

@weierophinney
Copy link
Member

Fixed with #99 and released with 2.8.2.

@ikovalyov
Copy link
Author

ikovalyov commented Dec 1, 2017

Actually it wasn't fixed

v 2.8.2:

Warning: session_start(): open(tcp://redis:6379?auth=password&database=1/sess_139bc23a33fc5fdda65bc4b7b9268c69, O_RDWR) failed: No such file or directory (2) in /opt/webapp/***/vendor/zendframework/zend-session/src/SessionManager.php on line 140

Warning: session_start(): Failed to read session data: files (path: tcp://redis:6379?auth=password&database=1) in /opt/webapp/***/vendor/zendframework/zend-session/src/SessionManager.php on line 140

v 2.8.0 everything works correctly.
code looks like

                $config->setOption("save_handler", "redis");
                $url = "tcp://" . $redisDto->getHost() . ":" . $redisDto->getPort() . "?auth=" . $redisDto->getPassword() . "&database=1";
                $config->setOption("save_path", $url);

@ikovalyov
Copy link
Author

@weierophinney

@weierophinney weierophinney reopened this Dec 1, 2017
@weierophinney
Copy link
Member

@ikovalyov — Your diagnosis is incorrect.

The change in behavior of setStorageOption() is correct; if that change is not present, then the code would call ini_set('session.save_handler', $value), which causes issues within PHP 7.2 in a number of cases.

What we are doing internally instead is, within setPhpSaveHandler(), calling session_module_name($saveHandler) for built-in save handlers, and session_set_save_handler() for any custom save handlers. getStorageOption() has been updated to return the value of session_module_name() unless a custom save handler was used (in which case, the class name is returned).

I'm going to write a unit test based on your example above, and debug from there; I'll link this to a new pull request once I have a solution ready to review.

weierophinney added a commit to weierophinney/zend-session that referenced this issue Dec 1, 2017
The fix in zendframework#99, while correct, did not address setting the session.save_path
for non-files save handlers when the save handler is set via
`setOption('save_handler', $value)`. The reason for this was due to the
fact that `setOption()` delegates directly to `setStorageOption()`
instead of the relevant class method; the changes in zendframework#99 make that
method a no-op in the case of a save handler option.

What this patch does is two-fold:

- It overrides `setOption()` and has it call `setPhpSaveHandler()` if
  `save_handler` is provided as the `$option` argument. It then returns
  on completion. Otherwise, it delegates to the parent.
- It modifies `setPhpSaveHandler()` to extract the bulk of the logic to
  a new method, `performSaveHandlerUpdate()`. This new method now
  returns the save handler _name_ to store. `setPhpSaveHandler()` then
  sets that name as the `save_handler` value in the `$options` property
  before returning.

I have added a test to verify this behavior based on an example provided
in zendframework#98.
weierophinney added a commit to weierophinney/zend-session that referenced this issue Dec 1, 2017
The fix in zendframework#99, while correct, did not address setting the session.save_path
for non-files save handlers when the save handler is set via
`setOption('save_handler', $value)`. The reason for this was due to the
fact that `setOption()` delegates directly to `setStorageOption()`
instead of the relevant class method; the changes in zendframework#99 make that
method a no-op in the case of a save handler option.

What this patch does is two-fold:

- It overrides `setOption()` and has it call `setPhpSaveHandler()` if
  `save_handler` is provided as the `$option` argument. It then returns
  on completion. Otherwise, it delegates to the parent.
- It modifies `setPhpSaveHandler()` to extract the bulk of the logic to
  a new method, `performSaveHandlerUpdate()`. This new method now
  returns the save handler _name_ to store. `setPhpSaveHandler()` then
  sets that name as the `save_handler` value in the `$options` property
  before returning.

I have added a test to verify this behavior based on an example provided
in zendframework#98.
@ikovalyov
Copy link
Author

thanks

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

No branches or pull requests

2 participants