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

Used UserStorage delegation instead of inheritance #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

foowie
Copy link

@foowie foowie commented Apr 27, 2014

Changed UserStorage inheritance to delegation for case when custom (basic) UserStorage is used.
To set custom parent UserStorage is necessary to set userStorage service in nette namespace.

nette:
    services:
        userStorage:
            class: FooNamespace\FooUserStorage

@Majkl578
Copy link
Owner

I like the idea, but I don't like the way you implemented it in DI. I'd rather prefer it to do automatically. If you need a custom storage, you replace default nette's in config by changing nette.userStorage. I think it could be still automated (but I am not sure whether that will be applied before loadConfiguration() is called). This should be just an internal decorator.

@foowie
Copy link
Author

foowie commented Apr 27, 2014

Fixed

@Majkl578
Copy link
Owner

There is more problems, but in Nette. Even though Nette relies only on IUserStorage (in User), it calls methods which are not defined in that interface, but only in UserStorage (methods getNamespace and setNamespace). Thus decorator is not a simple forwarding through interface, it is just hackish shit. How typical in Nette.

@Majkl578
Copy link
Owner

I kinda reworked your concept and fixed compatibility with User (as mentioned above). Could you please try and test branch feature/storage-decorated? (I'm currently not developing any application where I could test this thoroughly.)

@Majkl578
Copy link
Owner

Majkl578 commented Jul 3, 2014

Bump?

@foowie
Copy link
Author

foowie commented Jul 7, 2014

Seems working for me .)

@TomasVotruba
Copy link
Contributor

👍 Composition over inheritance

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

Successfully merging this pull request may close these issues.

None yet

3 participants