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

Add ValidatorServiceInterface for session validators #20

Open
weierophinney opened this issue Dec 31, 2019 · 0 comments
Open

Add ValidatorServiceInterface for session validators #20

weierophinney opened this issue Dec 31, 2019 · 0 comments

Comments

@weierophinney
Copy link
Member

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:

  • implement the ValidatorServiceInterface, and
  • be registered with the ServiceManager.

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

Caveats:

  • In the old way of doing things we only registered session validators when creating the session. On subsequent requests the session validators would be instantiated automatically. The new validator services work differently in that they must be injected in the SessionManager on each request. If a service validator isn't injected it will be improperly created by the ValidatorChain, which may cause unknowing developers some trouble.

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

  • How to insert the validators when creating the session. I'm not sure how to do this without removing them all, re-getting them from the service manager, injecting the reference value and then attaching them again. There are probably other ways, but it's still overly complicated.

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 (zendframework/zendframework#7381 (comment)) 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.



Originally posted by @GeeH at zendframework/zend-session#48

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

No branches or pull requests

1 participant