Unwanted creation of profiles about ccdnuserprofilebundle HOT 13 CLOSED

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.