Comments (13)
This is important behavior, if a profile is not linked, then many other parts of the profile bundle can fail. We have various checks just incase, but if a profile is not linked then we create one.
As the model wraps both repository and manager classes acting as a facade, this seems a good place to do this.
And we choose the UserModel, because again it is the best place to get the User and Profile, as we start with a member list/search etc, and that requires profile data for things such as website/msn/icq links etc.
from ccdnuserprofilebundle.
I understand why it is there. From the code I've read I understand that, once this bundle is installed, User and Profile are mutually inclusive. But I do not agree on the solution.
The current solution, putting this logic in the UserModel, means:
- Unexpected behaviour: retrieving a User without a Profile equals creating a profile; this is very much like making a GET request to create a resource, and,
- Unnecessary checks: over a long enough period of time, these checks will be redundant as all users will have Profiles. And,
- Breakage in the Profile code could mean breakage in the UserModel. Which is, for novices to the code like me, difficult to understand.
I propose we fix this by:
- Merging the User and Profile facades into one. As I mentioned, User and Profile are treated as one entity once this bundle is installed. The code needs to reflect this.
- Remove the
checkUserHasProfile
code. - Create a new task to generate Profiles for Users without Profiles.
- Mention the new task in the documentation as a post-installation step. If there are pre-existing Users, then they will not have Profiles. Any new users will have a Profile and are not a problem.
from ccdnuserprofilebundle.
Create a new task to generate Profiles for Users without Profiles.
Thats the part that worries me, because i once used doctrine prePersist to do this, and for some reason something did not work, i had a bunch of users at one point on my site get registered without a profile prePersisted. It created a lot of problems for my site. The site was young at the time but it was something difficult to test for, as the test would of needed to bridge functionality between ProfileBundle and some UserBundle and i was not using the same techniques i now use to pull vendors down within the bundle for testing (useful for travis-ci etc).
What method would you propose to create Profiles in a timely enough fashion that it would not interfere with other aspects of the bundle depending on a Profile being set?
In either event, it seems to me you will still need those redundancy checks.
from ccdnuserprofilebundle.
I'm proposing the following:
- New User entities always have a default Profile entity linked to it, and,
- Pre-existing User entities are migrated, to have a Profile created for them.
As to the details:
The task I'm referring to would be a Symfony task. Something to run like php app/console cc:user-profile:migrate-users
. That task would have logic similar to the following (assuming merge of user/profile facades):
$migrateUsers = $migrationModel->getUsersWithoutProfiles();
foreach ($migrateUsers as $user) {
$user->setProfile($userProfileModel->createDefaultProfile());
$userProfileModel->persistUser($user);
}
The task would only be used for migrating existing users to our new view: ensuring pre-existing users (users created before the bundle was installed) have a profile.
There might be no need to use prePersist, but I think I understand why you mention it:
as it stands, when a new user is registered, no profile is created for them. In my view, this should not be so, because creating a User entity means creating a default Profile entity. That way, any new user has a Profile.
As to needing redundancy checks regardless of any changes: I'm arguing that the UserProfile model can only retrieve User entities with a Profile and the bundle should, at no point whatsoever, allow a User entity to be created without Profile entity. This makes the checks redundant.
from ccdnuserprofilebundle.
@maartenJacobs ok then, that sounds good though the documentation would have to mention that the developer include the prePersist method in their bundles User entity schema, unless of course there is some generic doctrine listener that could be used to determine creation of a User entity (via Symfony's own UserInterface) and then create the Profile in the listener. Otherwise a mention in the documentation is fine.
from ccdnuserprofilebundle.
Ideally, there should be little changes to the documentation (other than the Symfony task) and the listener is not visible to the developer using the bundle (i.e. same behaviour as now, but it's up front).
Thanks for taking the time to discuss this. I'll take a stab at creating a PR for this (with a tonne of tests) next week.
from ccdnuserprofilebundle.
@maartenJacobs thanks for helping out, its appreciated.
Will you be attending SunshinePHP? If so i may see you there.
from ccdnuserprofilebundle.
You're welcome :)
Unfortunately not, but I am going to the UK conference
from ccdnuserprofilebundle.
Which UK conference? SymfonyLive London?
from ccdnuserprofilebundle.
The one from the PHP London UG (really, it should be called PHP London Conference): http://phpconference.co.uk/
There's no Symfony Live London 2014 announced yet, but I'd definitely go
from ccdnuserprofilebundle.
@reecefowell, I'm working on the PR, but as I'm digging deeper I found that I'm not sure how the Repository, Manager and Gateway classes are meant to work together. What are the responsibilities of each of these classes?
from ccdnuserprofilebundle.
@maartenJacobs The Gateway handles common ORM stuff, persisting/updating etc, and both the Repository and Manager gets the Gateway objects injected into them, then both Manager and Repositories and injected into the FrontModel.
|--FrontModel
|--Repository
|--Gateway
|--Manager
|--Gateway
Every entity has its own implementation of the above. For example:
|--TopicModel
|--TopicRepository
|--TopicGateway
|--TopicManager
|--TopicGateway
So the Manager focuses on write operations while the Repository focuses on the read operations, and the Gateways help them both do this but trying to abstract Doctrine specifics away from the Models (Repository/Manager [which is why we have the FrontModel as a Facade to bring it back together and simplify using it from outside]).
/ -- TopicManager -----\
TopicModel --< >-- TopicGateway
\ -- TopicRepository -- /
from ccdnuserprofilebundle.
@reecefowell Thanks for the explanation. It makes sense and I'll keep it in mind as I work on the PR :)
from ccdnuserprofilebundle.
Related Issues (4)
- Docs are out-of-date HOT 3
- KnpPaginatorBundle dependency HOT 3
- Users with numeric usernames HOT 9
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ccdnuserprofilebundle.