Handling of the is_default property of profiles is currently inconsistent based on factors including how the profile is created (on registration, through the profile add/edit form, programmatically) and whether or not the profile type is designated as multiple (meaning a given user can have more than one profile of that type).
| How created | Profile type multiple | Profile type not multiple |
|---|---|---|
| On registration | not set as default | not set as default |
| Via profile add/edit form | set as default | set as default if user selects button |
| Programmatically | not set as default | not set as default |
What is expected? Put differently, what should a consistent behaviour look like?
The method ProfileStorage::loadDefaultByUser() loads (according to its documentation) "the default user profile" of a given type. The method considers a profile to be default if both its is_default property and its status property evaluate to TRUE.
So: if a user has at least one active profile of a given type, one of them should be the default.
Profile::postSave() to cover the case where the current default is deactivated.
Profile::preSave().
ProfileForm::actions():
::setDefault method and code that adds it as a submit handler.ProfileListBuilder::getOperations() to harmonize with ProfileForm::actions():
ProfileForm::deactivate() method.
'set_default' button will appear on profile add/edit form only if profile is not new.
Profile::preSave() conditionally sets default status of profiles.
None
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff-15-13.txt | 2.49 KB | mglaman |
| #13 | profile-default-2809081-12.patch | 7.98 KB | nedjo |
| #11 | profile-default-2809081-11-tests-only.patch | 4.28 KB | nedjo |
Comments
Comment #2
plopescHere I'm attaching the patch that performs the changes commented above.
However, not sure if would be more interesting to set always profiles as default at that point given that, even if it's multiple, that's the only profile of that type available when the user is being registered.
If you want I could redo the patch to match that behavior if it's a better fit.
Thanks
Comment #3
plopescComment #4
hypertext200This works like charm.
Comment #5
nedjoNice catch. Indeed this is something that needs fixing.
The current patch correctly replicates the logic in
ProfileForm::actions(). But - if the aim is to ensure that, if there is at least one profile of a given type, there is a default profile - that logic seems faulty.The logic is:
This doesn't cover the case that that this is a new profile and the first of its type for the user. Unless the user creating the profile chooses to make it the default, there will be no default profile of the given type.
When collecting profile data on registration, we can be sure it's the first profile of its type (unless it's possible to create multiple profiles of a given type on registration).
So it sounds like in this case we should be setting as default irrespective of whether the bundle supports multiple profiles.
But, ideally, we wouldn't be doing any of this in form submission handlers. That leads to duplication (we need the same or similar logic in multiple places). And it also means that profiles created through other workflows will be missed.
Ideally this would have an accompanying test.
Updating the issue summary accordingly.
Comment #6
nedjoWorking on a revised patch.
Comment #7
nedjoComment #11
nedjoFixing up tests.
Comment #13
nedjoUploaded those in the wrong order so this was set back to needs work. Re-uploading the same patch as in #11.
Comment #14
mglamanReviewing
Comment #15
mglamanLooks great. Going to commit with some of the unused variable tweaks attached in this interdiff.
Comment #17
mglamannedjo, thanks so much for the effort and patch!
Comment #18
dafederThis introduces a regression - new issue created #2878235: Set default button now showing in new profiles again