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

Remove default session manager as it introduces hidden dependencies #15

Open
weierophinney opened this issue Dec 31, 2019 · 4 comments
Open
Labels
BC Break Enhancement Question Further information is requested

Comments

@weierophinney
Copy link
Member

Currently the Zend\Session\AbstractContainer class has the setDefaultManager() static method which completely breaks the concept which forces developers to explicitly specify the dependencies for a class, model, controller etc. As a result, many ZF3 components implicitly use the default session manager.

If I configure 'not default' session manager, I have to pass it everywhere including controllers, services, validators (like FlashMessenger) etc., but I think this possiblity is not developed enough in ZF3, because all components use default session manager implicitly.

I suggest to remove the static default session manager and force users to configure and provide the session manager explicitly.


Originally posted by @olegkrivtsov at zendframework/zend-session#58

@weierophinney weierophinney added BC Break Enhancement Question Further information is requested labels Dec 31, 2019
@weierophinney
Copy link
Member Author

my two pennies worth on this one:
I do not personally see yet why there should not be a default session manager. A shared configured session sounds like a logical thing to me. However I totally agree that we could use a more elegant plumbing on this

For instance, when the session manager is configured using config options array, if the session manager has not been explicitely instanciated, the Zend\Authentication\AuthenticationService storage container will not use it thus ignoring the configured session. Here 's some pseudo code to make myself clear :

let's suppose we have this config :

'session_manager' => array(
        // enables the session manager as the default manager for container (default to true)
        'enable_default_container_manager' => true,
        'validators' => array(
            'Zend\Session\Validator\HttpUserAgent',
            'Zend\Session\Validator\RemoteAddr',
        ),
    ),
'session_config' => array(
        'php_save_handler'  => 'redis', 
        'cookie_secure' => true,
        'cookie_httponly' => true,
      ...
    ),

The following WON'T use the configuration:

$authService = new \Zend\Authentication\AuthenticationService();
authService->getStorage()->write( $anything );

BUT this will:

$uselessVar = $services->get('Zend\Session\SessionManager'); 
$authService = new \Zend\Authentication\AuthenticationService();
authService->getStorage()->write( $anything );

as during instanciation the sessionManager factory will call the static Container::setDefaultManager
@see Service/SessionManagerFactory.php line 137


Originally posted by @jcaillot at zendframework/zend-session#58 (comment)

@weierophinney
Copy link
Member Author

I agree there should be the "default" session manager service in the service manager (and it exists already - it is named Zend\Session\SessionManager. I suggest to remove the "static" default session manager, because its usage is confusing and conflicts with the concept of services.


Originally posted by @olegkrivtsov at zendframework/zend-session#58 (comment)

@weierophinney
Copy link
Member Author

TL;DR: I agree

In the vast majority of cases developers need only reconfigure rather than replace the default session manager. There are a number of provided ways to influence the configuration of the session manager:

The default factories for Zend\Session\SessionManager (found in the Zend\Session\Service namespace) will read a number of keys from the application configuration:

  • session_manager for controlling constructor parameters of the SessionManager instance and whether this instance is the global default (SessionManagerFactory)
  • session_config for controlling the PHP session parameters (ConfigFactory)
  • session_storage for controlling the storage backend used by SessionManager (StorageFactory)

Documentation for all of these options can be found here: http://zendframework.github.io/zend-session/config/

You could also attach a delegator factory to Zend\Session\SessionManager and then manipulate the SessionManager instance any way you need.

That said, and as @jcaillot shows, the "magic" injection of the default session scope isn't without it's problems. Personally I do prefer to inject the session manager or session container into the places I need it, and ZF doesn't preclude you from doing that -- the Container "default manager" stuff is a convenience layer but in no way mandatory.

In it's current state the only real benefit you get from the Container "default manager" setup is you can do new Container('name') instead of needing a factory to build the Container and inject the SessionManager. It's a "benefit" that's questionable at best for the reason you stated (hidden dependency).


Originally posted by @adamlundrigan at zendframework/zend-session#58 (comment)

@weierophinney
Copy link
Member Author

@jcaillot this is due to the fact that service manager factories aren't executed until the key is requested. AuthenticationService relies on a session Container instance, and that Container can't fall back to the default SessionManager until the default is injected into it. That default isn't injected until SessionManagerFactory is executed for the first time. In the absence of an injected default session manager, Container will instantiate one without config (here).

You've also identified the only feasible workaround at this point: if you have customized session manager configurations you need to pull the session manager somewhere early in the bootstrap process to ensure that your configured session manager becomes the system-wide default.


Originally posted by @adamlundrigan at zendframework/zend-session#58 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break Enhancement Question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant