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

[BC BREAK] Remove state from user entity #526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Danielss89
Copy link
Member

In the process of simplifying ZfcUser i've removed 'state' from the user entity.

@ojhaujjwal
Copy link
Contributor

I agree with the removal of this state thing. 👍

@stijnhau
Copy link

stijnhau commented Oct 5, 2014

Seems logic to me never used that option so 👍

@adamlundrigan
Copy link
Contributor

👍 IMO we should go further and remove everything that isn't necessary for authentication (eg: display_name, and, well, that's it I guess :P)

@texdc
Copy link

texdc commented Jan 15, 2015

👍 Many implementations would be better served by an Enablement

@Danielss89
Copy link
Member Author

@adamlundrigan i like the idea of display_name, because other modules know there's a way to print the users name if we have that method.

@adamlundrigan
Copy link
Contributor

@Danielss89 IMO in all but the simplest cases the developer will be extending ZfcUser with additional profile data for the user anyway so removing it from core would just add another field to implement. That said, name is pretty well a universal field for profiles so maybe you're right and we should leave it alone.

@stijnhau
Copy link

display_name shoudl stay in accoring to me.
Can be used to store the real name if we want to show that or somethiing else.
It shoudl be just the user state.
so merge is good for me 👍

@texdc
Copy link

texdc commented Jan 24, 2015

KISS: The only fields a user object needs is an account ID and a set of credentials. (Both usernames and passwords are credentials.) Everything else belongs in a profile object. This allows for multi-factor authentication and frees the developer to develop implementation-specific profiles. I'd support an abstract or basic profile class with first, last, and display name fields. Leave the favorite color fields to the custom project.

@adamlundrigan
Copy link
Contributor

@texdc that's my line of thinking as well, and what I've tried to accomplish (albeit awkwardly) in LdcUserProfile. In my own apps I prefer to keep the mechanical "user" (for authentication) separate from the domain model "user" (for use in the app).

We could chuck all the non-authentication-related fields into a separate ZfcUserProfile module that, as you say, provides an abstract class with the common fields already implemented and make it easy to add your own fields. I liked the approach taken by SpiffyUser, where various bits of the system were implemented through extensions that could be enabled/disabled.

Splitting it out into a separate module jives with my suggestion in #555 (Splitting ZfcUser); split each separate concern into it's own module, then zf-commons/zfc-user would become a meta-package that pulls the bits together into a cohesive package for those who want the whole shebang.

@claytondaley
Copy link
Contributor

display_name may be widely used, but locks someone into that implementation rather than the equally reasonable first_name and last_name.

@stijnhau
Copy link

Any progress on this?

@Danielss89
Copy link
Member Author

This is for 2.0 which is still in the future, so i'll keep them as PR's until i'm ready to do more about it.

@stijnhau
Copy link

stijnhau commented Jul 8, 2016

@Danielss89 Maybe you can merge this PR now?
Cause i don't see the point to keep this PR open and merge it in later when you cna do the small changes directly so there won't be that much open PR's

@Danielss89
Copy link
Member Author

@stijnhau Do you have time to rebase this?
I won't have time before week 29.

@stijnhau
Copy link

stijnhau commented Jul 8, 2016

@Danielss89
i have but no write acces to either repo

@Danielss89
Copy link
Member Author

@stijnhau You can just make a PR for the Eye4web/ZfcUser, and i will merge that. Then it will automaticly enter here.

@stijnhau
Copy link

stijnhau commented Jul 8, 2016

@Danielss89
Srry io have the time to do it but i can't even make a copy of that repository because of my git knowledge

@stijnhau
Copy link

stijnhau commented Jan 2, 2019

@Danielss89 Any progress on this?

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

Successfully merging this pull request may close these issues.

6 participants