zendframework / zend-session

Manage and preserve session data, a logical complement of cookie data, across multiple page requests by the same client.
BSD 3-Clause "New" or "Revised" License
42 stars 64 forks source link

Add ValidatorServiceInterface for session validators #48

Open GeeH opened 8 years ago

GeeH commented 8 years ago

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html


Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/7384 User: @larsnystrom Created On: 2015-03-30T14:29:30Z Updated At: 2015-11-06T23:54:06Z Body This PR fixes #7381 without any BC breaks. It allows a session validator to be instantiated with dependencies and adds some default functionality to the SessionManagerFactory to get() session validators from the ServiceManager.

It introduces the new interface Zend\Session\Validator\ValidatorServiceInterface which extends Zend\Session\Validator\ValidatorInterface. It modifies the Zend\Session\Service\SessionManagerFactory to also inject validator services into the session manager. Lastly it modifies Zend\Session\SessionManager and Zend\Session\ValidatorChain to attach the validator services to the ValidatorChain and inject the reference value from the current session into the validator service using the setData() method defined in the ValidatorServiceInterface.

A validator which implements the ValidatorServiceInterface can be registered under the key services in the configuration array for session validators, like this:

array (
    'session_manager' => array(
        'validators' => array(
            'OldSessionValidatorWillWork',
            'services' => array(
                'NewValidatorServicesGoHere',
            ),
        ),
    ),
);

The example above illustrates that the old way of registering validators still works, and that new validator services goes under the services key of the validators array.

All validators under the services key must:

I'm open for discussion, but I think this is the best way to implement this feature without a BC break.

Caveats:

Things I haven't thought of yet: (hey, that's a paradox!)


Comment

User: @Martin-P Created On: 2015-03-30T14:44:32Z Updated At: 2015-03-30T14:44:32Z Body I like the way you fixed this without a BC break, but it also feels a bit like a workaround for the somewhat messy implementation of the SessionValidators. Personally I think Zend\Session can use a bit of a cleanup, so SessionValidators are created at one point in the process. Yes, that would mean a BC break, but looking at the current implementation I think that it can only be an improvement of the code.

See also my other comment (https://github.com/zendframework/zf2/issues/7381#issuecomment-87581173) regarding the SessionValidators:

Validators can be created at multiple points in the process: inside SessionManager and inside ValidatorChain. IMO the ValidatorChain should never create its own validators and should only be a dumb object containing the callback functions for validating the session. The validators itself should be injected at one point in the process and the session values can be injected there. Also that would make it easier to implement the feature with the ServiceManager.


Comment

User: @larsnystrom Created On: 2015-03-30T15:54:19Z Updated At: 2015-03-30T15:54:19Z Body I'm all for refactoring of Zend\Session, but since it would cause a BC break it will probably have to wait until ZF3 (or until somebody splits ZF into it's separate components). There are some serious issues with the current implementation, so I'm looking forward to that, but without word from higher up the chain of command here, it's just not possible.

I also agree with you that the ValidatorChain should never create it's own objects. However, that's another thing that can't be changed without a BC break. To be honest I think we should scrap the ValidatorChain altogether and just use an array in the SessionManager. I don't see the point in using a priority queue when we can't even specify priorities.

I also think it was a bad idea to force validation of the session in SessionManager::start(), especially when you can't specify which values to validate before running it.

One thing that could be done is adding an argument to ValidatorInterface::isValid(), to provide the session storage to the validator. That way you could validate pretty much anything you'd like, even certain combinations of values. It would also be a more intuitive way of doing the validation, following the same pattern used in Zend\Validator where isValid() always takes the value to be validated as the argument.

I guess this component was put together in a hurry. Splitting ZF into separate components would allow us to start fixing the real problems here. In the meantime we can stuff like I did here.


weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-session; a new issue has been opened at https://github.com/laminas/laminas-session/issues/20.