Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Work on the 8.x port is done on Github at https://github.com/fago/profile2. Please submit any improvements as PRs there and open issues for any discussions and issues in general here in the queue.
Original summary:
Goal
- #1668292: Move simplified Profile module into core
- Part of the Profile in core effort.
Battle plan
- DONE: Minimal port to D8. (non-functional)
- DONE: Make the current code work with D8's entity system.
- DONE: Comment out or work around advanced (contrib) Entity API module functionality (e.g., metadata, etc)
- DONE: Investigate what code can be removed.
- DONE: Convert profile types into configuration.
- DONE: Implement EntityFormController for Profile and ProfileType.
- DONE: Implement EntityRenderController for Profile.
- DONE: Implement EntityListController for ProfileType.
- DONE: Rework user profile category tabs (@sun). Remove necessity for rebuilding the menu/router.
- DONE: Remove unnecessary helper functions.
- DONE: Implement entity access controller
- Fix and improve pre-existing Profile2 logic/behavior:
- DONE: Move profile2_load_by_user() as custom method into ProfileStorageController. Eliminates the drupal_static().
- DONE: Missing UUID for profiles.
- DONE: Ensure to leverage entity_delete_multiple()'s bulk deletion process wherever multiple profiles are deleted.
- DONE: Remove remaining entity API wrapper functions
- DONE: Migrate default "Main" profile from profile2_install() into config for now.
- Update Profile to EntityNG + Property/Field API. (not ProfileType)
- Figure out what to do with the custom profile access system.
- Vastly improve test coverage.
- Profile (UI) delete confirmation.
- Profile access.
- DONE: Clean up unnecessary and stray contrib Entity API code.
- Apply coding standards, fix coding style, phpDoc, and comments.
- Consistently use $type for ProfileType entity variables. $id for ProfileType IDs, $bundle for Profile bundles.
(Replace all $type_name, $type_id, $profile_type, etc.)
- Rename the whole thing from profile2 into profile. :)
- Move default "Main profile" into the standard installation profile.
- DONE: Move profile2_load_by_user() as custom method into ProfileStorageController. Eliminates the drupal_static().
- DONE: Missing UUID for profiles.
- DONE: Ensure to leverage entity_delete_multiple()'s bulk deletion process wherever multiple profiles are deleted.
- DONE: Remove remaining entity API wrapper functions
- DONE: Migrate default "Main" profile from profile2_install() into config for now.
- Update Profile to EntityNG + Property/Field API. (not ProfileType)
- Figure out what to do with the custom profile access system.
- Profile (UI) delete confirmation.
- Profile access.
- Consistently use $type for ProfileType entity variables. $id for ProfileType IDs, $bundle for Profile bundles.
(Replace all $type_name, $type_id, $profile_type, etc.)
http://drupalcode.org/project/profile2.git/shortlog/refs/heads/8.x-1.x
Needs architectural review
- Is there any use-case for custom user profile labels? Yes there is, e.g. multiple profiles for the same profile type (invoice/shipment addresses, etc.).
- Consider removing the 'profile2_private' functionality. That looks rather like a custom field access solution instead of something we should provide as a built-in solution.
- Should we use extra fields for injecting the profile form / displaying the profile on the user form / view?
Comment | File | Size | Author |
---|---|---|---|
#172 | profile2.patch | 8.09 KB | sun |
#171 | profile2.patch | 6.42 KB | sun |
#170 | profile2.patch | 2.04 KB | sun |
#168 | profile2.patch | 1.29 KB | sun |
#165 | profile2.patch | 962 bytes | sun |
Comments
Comment #1
sunComment #2
sunComment #2.0
sunUpdated issue summary.
Comment #2.1
sunUpdated issue summary.
Comment #3
n3or CreditAttribution: n3or commentedMy first version. I made the following things working:
- profile types listing (admin/structure/profiles; for an unknown reason the title of this page is "Error")
- profile type add form (admin/structure/profiles/add)
- profile type edit form (admin/structure/profiles/manage/$type/edit)
- profile type delete form (admin/structure/manage/$type/delete)
For sure there are some faults, but it's a beginning.
Comment #4
sunoh. Sorry for missing those classes in profile2.module!
I've committed the basic/minimal change (moving them into the new location), and rebased your patch against latest HEAD.
Comment #5
sunSorry, one more follow-up - committed the interdiff changes.
Comment #7
n3or CreditAttribution: n3or commentedGot the following things done:
- Installation (including creation of default "main profile")
- profile type manage fields form (admin/structure/profiles/manage/$type/fields)
- profile type manage display form (admin/structure/profiles/manage/$type/display)
- page title in admin/structure/profiles (see #3)
Comment #8
n3or CreditAttribution: n3or commented- Admin registration form works
- User registration form works
- Replaced "user_category" by using fieldsets in profile edit
Got the following message when I try to edit a profile:
I don't get where the revision.timestamp and revision.uid fields were added.
Comment #9
sunImportant: #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) landed for Drupal core!
This probably has a huge impact here. Essentially, the profile type remains to be an entity type, but simply extending the ConfigurableBase class instead of the Entity class. :)
Comment #10
sunDiscussed this patch with @n3or over Skype -- discussion backlog:
- Make ProfileType extend ConfigurableBase instead of Entity
- Rename ProfileType::$type => $id
- Add ProfileType::$uuid
- Remove ProfileType::isLocked() without replacement.
- Remove profile type import/export forms/functions without replacement.
- Remove ProfileType::__construct() constructor override, fix calling code instead.
- Make Profile::__construct() comply with StorableInterface::__construct():
- Use/implement EntityFormController for Profile entity type: #1499596: Introduce a basic entity form controller
Drupal\config\ConfigStorageController
Missing ()
Can be removed.
Title should be 'List'.
Remove entirely.
Let's compare with taxonomy_menu() + taxonomy_entity_info(), which is the only comparable implementation in Drupal core.
Let's primarily orient at config_test.module in Drupal core.
However, config_test.module does not implement fields, so we also have to orient at taxonomy vocabulary overview page as well as node type list page. Node types seem to be more appropriate.
Note that #1184272: Remove deprecated $conditions support from entity controller just landed, so the signatures of the load* functions need to be changed accordingly.
Furthermore, in #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions there's a suggestion to not even implement entity type module specific API functions but use the entity_load() + friends functions directly. We might want to do that in a later step though.
Let's turn this into a ProfileType::$registration property.
Comment #11
n3or CreditAttribution: n3or commentedTried to implement all changes mentioned by sun. At the moment the EntityFormController is still missing.
There shouldn't any losings in functionality compared with #8.
Comment #12
n3or CreditAttribution: n3or commentedFixed incompatibility to current 8.x-1.x. Interdiff is meaningless.
Comment #13
sunSlightly revised. Any chance to test this before commit? ;)
Comment #14
n3or CreditAttribution: n3or commentedDid a few functional tests with positive result! Looks great. Thank you sun!
Comment #15
sunCommitted to 8.x-1.x. :)
Next steps:
1) Adjust the tests for the new code. Let's get the testbot working again, so we don't have to test patches manually.
2) Remove/replace profile2_* CRUD functions with entity* functions.
3) Remove profile2*_save() + _delete() with ->save() and ->delete()
Comment #16
n3or CreditAttribution: n3or commentedAlmost all tests will pass. One test will fail because "view/edit own main profile" permission is useless at the moment. We need to implement an alternative to user categories managed by profile2.
Comment #17
sunassertTrue vs. assertFalse -- massive change in expectations ;)
Let's always use the ->id() method to retrieve the (configurable) entity ID.
Comment #18
n3or CreditAttribution: n3or commentedSee comment of sun. Additionally added two empty lines killed in patch #16.
Comment #19
sunYAY! The testbot runs and passes against D8! :)
Comment #19.0
sunUpdated issue summary.
Comment #19.1
sunUpdated issue summary.
Comment #20
n3or CreditAttribution: n3or commentedRemoved the fieldsets in user/edit and implemented a "user category" replacement. But at the moment there is no linking from user/edit to user/edit/{$profile_type} (see D7 implementation in the attached screenshot; "main profile" and "permission test" were profile types). Any suggestions how to handle that in D8?
(Every test should pass now.)
Comment #21
n3or CreditAttribution: n3or commentedForgot screenshot.. :)
Comment #23
sunCommitted and pushed to 8.x-1.x, with a couple of additional adjustments. :)
Nice to see that this actually works already (tested manually)! :)
Comment #24
sunNext step would be to port for entity form controllers. :)
@n3or informed me that he most likely won't be able to work on this further in the next few days/weeks.
Any takers?
I'll look into the exposure of profile categories as local tasks on the user account edit page, since that seems to require some menu system voodoo. ;) (right now you need to manually access
user/%uid/edit/main
to get to the respective profile-type edit form)Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedNice work here.
I think that category subnavigation is a pre-ajax relic. I think the modern way to edit profiles is to view your own and click on stuff and edit inline. See facebook and Google+. I don't think there is much point in separating the profile out to separate pages.
Comment #26
fubhy CreditAttribution: fubhy commentedSun said in #24 that n3or is busy currently. I am taking over until he reports back for duty :).
Nearly done with "Clean up unnecessary code and stray helper functions." and "Implement EntityFormController for Profile and ProfileType.".
Also rebased on the current version of core (some classes were moved recently, etc.).
Comment #26.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #27
fubhy CreditAttribution: fubhy commentedTalked to @sun earlier today. We are going to push this forward with separate, smaller patches. First of all, this is the form controller patch. It converts both, the profile and the profile type form. However, the profile form simply getting built with fields, we still got most form related code in profile2.module because of the way we are currently conditionally attaching our profiles to the registration form through hook_form_user_register_form_alter().
I hope that we can find a better way of doing that without form alters. E.g. through a multi-step process or something ... The problem is, that by altering the form, the form submission / validation process becomes really fugly. That is why I didn't convert that part yet. If we decide that we want to keep it as is we can rebuild that to use the form methods through entity_form_controller() directly. I think that would be a follow-up patch as well.
Additionally, I added the tabs as shown in the screenshot above. On the user/%user/edit page we now have these tabs (one for each profile).
Some classes in core have been moved too... So I simply added that to this patch as well.
Will post the patch for the next step once this one is commited :).
Comment #28
fubhy CreditAttribution: fubhy commentedRemoving that ugly strtr(). Dunno why I even added that. :P
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedCould we get those final test fails out of the way? As i said in #25, I see subnavigation across profile types as pretty unwanted these days but is fine for the purposes of this patch.
Comment #31
n3or CreditAttribution: n3or commentedI'm back. Thank you very much fubhy!
I will try to get this patch green.
Comment #32
n3or CreditAttribution: n3or commentedConcentrated on fixing failing tests!
Locally now all tests passed. Adapted the test case and added the save handler within profile form controller.
Comment #33
sunThe next step here, after completing the form controller conversion should be to do the render controller conversion.
Why am I not allowed to delete my own profile as a user?
I just see that the original code didn't allow for this either, so this should probably be adjusted/fixed in a follow-up patch; laters.
This message should dynamically adjust to the current user; i.e., whether you are editing your own profile, or whether you are a site administrator editing another user profile.
For regular profiles, this shouldn't be needed.
No longer needed, HEAD defaults to 'label' now.
::save() returns $status, let's differentiate between created and updated in the message.
In general, see #1798944: Convert config_test entity forms to EntityFormController for a prime/master example config entity conversion.
Let's add a @todo right above these to remove them. (That will be my job.)
I don't see why the additional /profile/ part in the path is necessary. Let's revert this.
We need a page callback here to set an appropriate page title with drupal_set_title().
This @todo doesn't make too much sense to me. Let's remove it and discuss what can or needs to be improved on-issue instead.
Comment #34
n3or CreditAttribution: n3or commentedFixed everything mentioned in sun's comment. (Except the first two suggestions, they aren't related to this patch.)
Comment #34.0
n3or CreditAttribution: n3or commentedUpdated issue summary.
Comment #34.1
sunUpdated issue summary.
Comment #35
sunThanks! Committed and pushed with a couple of minor adjustments.
Next step: EntityRenderController.
(Updated issue summary accordingly; also started to collect issues from patch reviews there for later clean-up; but let's continue with the straight porting first.)
Comment #35.0
sunUpdated issue summary.
Comment #36
n3or CreditAttribution: n3or commentedFirst version of EntityRenderController implementation.
Comment #37
n3or CreditAttribution: n3or commentedRemoved profile2_view.
Comment #37.0
n3or CreditAttribution: n3or commentedUpdated issue summary.
Comment #38
sunYay, thanks! Committed and pushed.
Next step: EntityListController for ProfileType config entity.
Comment #39
sunClarifying issue title.
Comment #40
sunAttached patch removes the need for rebuilding the menu router, taking the same approach as in
#1067408: Themes do not have an installation status
But of course, having to workaround a dozen of quirks of the menu system and hook_menu_local_tasks_alter() in core to achieve it.
(That said, I'll probably take over those workarounds for that core patch...)
Tests pass for me locally (but test coverage is extremely limited anyway, so I manually tested this extensively on top). Will commit in a bit, unless anyone raises objections.
Comment #41
sunCommitted and pushed #40. Back to #38:
Next step: EntityListController for ProfileType config entity.
@n3or mentioned he wants to do that tomorrow.
If anyone wants to work on improving test coverage or some other dedicated task, please do so (and let us know before/when you do)! :)
Comment #41.0
sunUpdated issue summary.
Comment #41.1
sunUpdated issue summary.
Comment #42
n3or CreditAttribution: n3or commentedFirst version of EntityListController.
Comment #43
n3or CreditAttribution: n3or commentedComment #44
n3or CreditAttribution: n3or commented- Removed unused code
- Made field_ui operations conditional
Comment #45
sunThanks! Committed and pushed with minor adjustments.
The only noteworthy one: EntityListController uses capitalized operation labels now (i.e., "Edit" vs. "edit"), whereas the Field UI links were still lowercase.
This was the last precisely defined atomic step, so unless someone has another one that isn't mentioned/listed in the summary yet, we're basically in "random fix + clean-up + improve test coverage mode" now.
Comment #45.0
sunUpdated issue summary.
Comment #46
fubhy CreditAttribution: fubhy commentedshould use _field_ui_bundle_admin_path() instead of $entity->uri()
same here.
Furthermore we need to rebuild the entity info after adding a new profile type. Otherwise _field_ui_bundle_admin_path() fails.
Comment #47
sun_field_ui_bundle_admin_path() is marked as a private function, so it would be inappropriate to call it if you are not Field UI module.
None of the administrative entity listing pages in Drupal core do that either; e.g.:
node_overview_types()
Comment #48
fubhy CreditAttribution: fubhy commentedAlso, we have to provide a storage controller until we have a proper bundle api as proposed in #1782460: Allow to reference another entity type to define an entity type's bundles to inform the Field API of our bundles.
Tried to address.
Comment #49
fubhy CreditAttribution: fubhy commentedYou are right. I am not really sure WHY it is a private function though. It's simply a helper/wrapper for convenience.
Comment #50
fubhy CreditAttribution: fubhy commentedWhoops, copy&paste fail.
I am leaving the _field_ui_admin_path() stuff in. Re-store the previous code if you want. I am going to file an issue to remove that underscore prefix from that function. It doesn't make any sense imo.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedNice work! Profile editing has come along since profile.module. Specifically, look at the lovely edit in place experience that google+ and facebook offer. Thats what I would love to see from profile2.
Would be great if someone could install edit module with profile2 and make a list of todos in order to get a Google+ like experience.
Comment #52
webchickOne big todo is right now Edit module's hard-coded to only work with nodes. We have expanding that to all entities as a post-feature freeze task, but of course if someone wanted to get to it sooner, we'd love it. :)
Comment #53
sunThanks - I wondered why this seemed to work, so I've complemented that with a CRUD UI test for profile types. The new storage controller indeed fixes a range of bugs.
This is a very interesting part for Configurables in D8 in general. The Field (UI) integration ought to be possible much easier for fieldable entities whose bundles are managed by Configurables. I'll think about the required Config\Entity improvements.
Lastly, there's a final assertion in the new test, which fails. Something is broken in D8 HEAD, because I can perfectly reproduce that failure manually, but the entire field configuration data is stored and updated correctly. We probably need to file a (major) bug against core for that.
Will commit this in a minute. (Managing the patch flow in this issue now that we're beyond atomic steps isn't trivial, so I'll do my best to ensure a fast-path patch-to-commit flow. Keep 'em coming. :))
Comment #54
sunBtw, I hope we'll be able to eliminate profile2.api.php entirely, as I filed this against core:
#1824184: Remove and replace entity type specific API hook documentation with generic entity API hook documentation
Comment #55
fubhy CreditAttribution: fubhy commentedCommon... What do we do with profiles without fields? :D
Fields should be a dependency for profiles. no?
I didn't look at which assertion you mean but I already filed a bug report for a field ui bug in HEAD: #1824180: Undefined variable $admin_path in Field UI module
I think we should start to work on #1782460: Allow to reference another entity type to define an entity type's bundles.
I also opened an issue for that private field ui function: #1824244: Tests for: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())
Comment #56
sunYou're right. Quickly reverted and replaced that with a proper field module dependency in the .info file prior to commit.
Comment #57
fubhy CreditAttribution: fubhy commentedNext up: Further clean up for stray contrib Entity API code and cleanup of the Profile2 class (uri, url, path, etc.)
Comment #58
fubhy CreditAttribution: fubhy commentedLet's also switch to EntityNG and NG controllers as soon as possible.
Comment #59
fubhy CreditAttribution: fubhy commentedI don't think we are missing that hook, or are we? It's just invoked _before_ the normal delete hook is invoked to reassign attached data if required. In our case that is not needed. We just want to delete the profiles.
However, I think that we should use hook_user_predelete() instead of hook_user_delete() because we should be deleting attached data prior to deleting the 'host' object. This is also the way that taxonomy terms are being deleted on vocabulary deletion. And it makes sense too because if we use hook_user_delete() In the deletion hooks for the profiles we would not be able to load the account anymore because it would be gone already.
Comment #59.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #60
fubhy CreditAttribution: fubhy commentedSeems related to the latest patch. Adding that now.
Comment #61
sunThanks! Committed with minimal adjustments. (Coding standards say that there should be a blank line before the closing brace of a class.)
Comment #61.0
sunUpdated issue summary.
Comment #61.1
sunUpdated issue summary.
Comment #61.2
sunUpdated issue summary.
Comment #61.3
sunUpdated issue summary.
Comment #62
fubhy CreditAttribution: fubhy commentedInteresting. I always thought that it was the other way round. Thanks! Updated my IDE to reflect that in the coding style configuration.
Patch: Removed profile2_load_by_user(). After all it's simply a wrapper for entity_load_by_properties() and caching is handled in the storage controllers anyways.
Comment #62.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #63
sunYou might need to adjust your IDE a little more ;)
Anyway, that is looking great and even better. However, before we do this, we want to heavily improve the test coverage for Profile CRUD operations. I'm working on that.
Comment #64
fubhy CreditAttribution: fubhy commentedAlso removing the other entity_load wrappers.
Comment #65
sunAdded Profile CRUD test coverage. Will commit this in a second and send the latest patch for re-testing.
Comment #66
fubhy CreditAttribution: fubhy commentedI am currently working on finishing the form controllers and allowing deletions by users.
Comment #67
fubhy CreditAttribution: fubhy commentedOkay ignore that last patch, lol.
This one (re-?)adds the delete functionality for profiles and also cleans up the Profile entity class a bit. As well as some other things. It also removes the static caching for example in favor of a simple wrapper for entity_load_multiple_by_properties(). We still need that wrapper to output the results keyed by profile bundle. profile2_type_load() can't be removed either because of the menu callbacks and the exists callback for the machine name field.
Comment #68
sunReverted removal of profile2_load_by_user() in #62 and committed that.
I'm not sure whether we really want to remove it. It appears to be a helpful helper function. If we really want to remove it, that would have to be merged into #64.
On #64, I think we should keep profile2_type_load() as-is for now. #type machine_name should and will most likely be improved to allow to specify a callable for the 'exists' callback definition anyway in HEAD. (We should create a core issue for that.)
Comment #69
fubhy CreditAttribution: fubhy commentedWow, what's up with my IDE now? Weird :)
Comment #71
fubhy CreditAttribution: fubhy commentedRe-roll
Comment #72
fubhy CreditAttribution: fubhy commentedAccidentally overwrote your version of profile2_load_by_user(). Sorry, doing to many things at once.
Comment #73
fubhy CreditAttribution: fubhy commentedSo sorry for the noise. :/
This patch is now actually the right one. Sorry!
Comment #74
sunI think this makes sense for the default implementation.
However, do we really want to remove the $label property entirely (if so, we'd also have to remove the schema column)?
What if there is a legit use-case in contrib for user-customizable profile titles/labels?
You removed setUser() already, and I wonder whether we really need the [get]user() method... do we?
btw, type() is a really confusing name. Can we rename this custom method to getProfileType()?
Let's back out those redirect/delete changes for now and focus on the Profile class clean-ups first.
I'm not sure why you removed the phpDoc, but they should obviously stay.
This need to call into EntityInterface::uri().
Let's back out this removal for now, and potentially merge it into the similar changes that were in #64.
Comment #75
fubhy CreditAttribution: fubhy commentedI am not a big fan of implementing features for which we can't even outline a use-case yet. So I would suggest to keep it simple for now and also remove the label property, yes.
#64 was a fail :). I will create a separate patch for just that if you want.
Yeah I wasn't sure what to write there. None of the other core entity uri callbacks have anything in their docblocks.
Okay, separate patch too then.
Attached patch should be the plain cleanup.
Comment #76
fubhy CreditAttribution: fubhy commentedThis is the plain profile2_get_types() removal on top of the previous patch in #75.
Comment #77
fubhy CreditAttribution: fubhy commentedAnd this is the plain delete form patch on top of the patch in #76.
Comment #79
sunI think the removal of Profile::$label needs some more discussion. It would be great to learn from Profile2 maintainers and their experience on that topic.
Reverted off-topic changes from #74 and will commit this if it comes back green.
Comment #80
sun#76: 1726822-76.patch queued for re-testing.
Comment #80.0
sunUpdated issue summary.
Comment #81
sunCommitted #79 and #76.
Updated the issue summary with a list of architectural discussion topics. @fago / @joachim, any feedback on those?
Comment #82
fubhy CreditAttribution: fubhy commentedAny objections to #77 except that it doesn't apply anymore?
Just a minor nitpick btw.:
return reset($profiles) ?: FALSE;
(from profile2_load_by_user()) is redundant. reset() returns FALSE for empty arrays anyways.Comment #83
sunRemarks on #77:
s/edit/delete/g
Entity delete confirmation form constructors are expected to always add a form #type value using the entity property name of the entity ID as key; i.e.:
That is, as long as the delete confirmation form is not handled by the entity form controller (which is not supported by core yet).
I don't think the confirmation question nor the confirmation message will work out with real values. Needs manual testing and wordsmithing.
Let's group these lines and also add a comment to explain what is being done and why.
Comment #84
fubhy CreditAttribution: fubhy commentedI couldn't find that consistency in core and rather found something even more ugly (using #type => 'hidden', #value = $id), etc.
I really prefer the $form_state solution... So I left it as is for now.
Added @todos for the messages. We can fine-tune that later.
Added 'delete' permissions for now. We should kill profile2_access() once the basic Entity Access API has been committed.
#1696660: Add an entity access API for single entity access
Comment #85
fubhy CreditAttribution: fubhy commentedOh, that was not the full patch
Comment #86
fubhy CreditAttribution: fubhy commentedIn case that doesn't make sense... Would it maybe make sense to move the profile types configuration from /admin/structure/.. to /admin/people/...? User roles are there too.
Comment #86.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #87
sunCommitted #85, plus the attached follow-up patch for it.
Btw, there are no tests for the profile delete functionality.
Comment #87.0
sunUpdated issue summary.
Comment #88
sunAttached patch adds UUIDs to Profile.
Comment #88.0
sunUpdated issue summary.
Comment #89
sunAdded a single test assertion for the uuid property and committed #88.
Comment #89.0
sunUpdated issue summary.
Comment #89.1
sunUpdated issue summary.
Comment #90
fubhy CreditAttribution: fubhy commentedThis patch moves the Overview to /admin/people/profiles ... I believe it makes more sense to have it there.
Comment #92
fubhy CreditAttribution: fubhy commentedWill do the Entity plugin conversion now.
Comment #93
fubhy CreditAttribution: fubhy commentedComment #94
sunAttaching a diff with lesser minimum similarity index, just to confirm that #93 just moves files around and updates phpDoc.
Committed and pushed #93.
Comment #95
sunSame as #90, but re-rolled, and only minimal adjustments.
Comment #96
sunCommitted and pushed #95.
Comment #96.0
sunUpdated issue summary.
Comment #96.1
sunUpdated issue summary.
Comment #97
sunConverted default configuration into configuration.
Comment #98
sun#97: config-default.97.patch queued for re-testing.
Comment #99
nevergone CreditAttribution: nevergone commentedSorry, but one question: With what better the Profile2, than the user entity bundles and fields?
Comment #100
fubhy CreditAttribution: fubhy commentedYes, let's simply delete this part. It's a installation profile thing.
Comment #101
fubhy CreditAttribution: fubhy commentedBut yeah... Let's leave it there for now. We can easily move it over to the installation profile (standard) once we are ready for getting this in.
Comment #101.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #101.1
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #102
fubhy CreditAttribution: fubhy commented@sun are you still pro D8 inclusion? If so, considering that most of my other issues are blocked currently I would like to focus on this again. Would that be a good investment in time?
Comment #103
sunAbsolutely. That said, I'm actually not sure whether this is bound to Dec 1st feature freeze, since we need to upgrade former Profile module data somewhere for D8, and this effort delivers the solution for that (and has been listed by @Dries as one of the major feature efforts). But then again, you never know... So probably better to get this done — that said, I actually think we're pretty close :)
Comment #104
fubhy CreditAttribution: fubhy commentedAll right... Let's fix the remaining todo's then. I am also adding a few todos:
Attached patch improves multiple entity deletion.
Comment #104.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary. Users can delete their own profiles (flush/empty them).
Comment #106
fubhy CreditAttribution: fubhy commentedRight.. profile2_load_by_user() returns the profile type as array key and DatabaseStorageController works with the array keys for deleting multiple entities. A bit fragile if you ask me (should maybe work with array_map()) to retrieve the entity id's instead of just going array_keys()...
Comment #107
fubhy CreditAttribution: fubhy commentedAdding access controller. This patch will sadly fail because of drupal_anonymous_user still returning a stdClass object instead of a user entity...
@see:
#1825332: Introduce an AccountInterface to represent the current user and #1634280: drupal_anonymous_user() should return a User entity
Comment #110
sunGreat!
Can't we simply remove the User type-hints for now to get around the
drupal_anonymous_user()
problem?I think we need a
user_load($GLOBALS['user']->uid)
here - the global $user object is not a real user entity.I don't really understand why we're concatenating these — the cache is an array, so let's just make them multi-dimensional array keys?
Isn't this obsolete?
Shouldn't we move this into the access controller? A modular/extensible entity access system rather seems like a topic that core should figure out in a generic way, no?
Comment #111
fubhy CreditAttribution: fubhy commentedSadly no... Not yet at least. We need it for the menu system. In the next patch I will convert our routes to the new routing system and provide proper menu system access checks. Once we have that we can remove it.
I agree, but let's keep it until we have it!
Yes, good idea. It's sadly the same problem as the one with anonymous users.
Sadly no, they are in the interface :/.
We gotta fix that here: #1825332: Introduce an AccountInterface to represent the current user
Sure, why not :)
Comment #112
fubhy CreditAttribution: fubhy commentedThis should pass. Again, good idea with the user_load().
Comment #113
fubhy CreditAttribution: fubhy commentedUltra minor tuning for the entity_delete_multiple patch.
Comment #114
fubhy CreditAttribution: fubhy commentedOkay, or we just go with the array keys as pointed out by @sun :)
Comment #115
fubhy CreditAttribution: fubhy commentedRemoving stray functions, files and properties and cleaning up documentation (also in api.php).
Comment #116
sunReverted removal of ProfileType::$uuid and $weight.
Comment #117
sunAlrighty. AFAICS, the only remaining tasks are:
- Tests: The CRUD tests should be possible to use DrupalUnitTestBase. There are also @todos.
- profile2.tpl.php: Needs to be moved into ./templates + HTML5-ified.
- profile2.install: Still installs default "main" profile type. Based on earlier discussion, it looks like we simply want to delete that and leave the configuration to installation profiles.
Comment #118
sunAttached patch eliminates the default "main" profile and adjusts tests.
Comment #119
fubhy CreditAttribution: fubhy commentedDeprecating profile2_load_by_user() (we don't like entity_* wrappers).
Tests are getting a bit more ugly with that. Could use a base test class with a helper function.
Comment #120
fubhy CreditAttribution: fubhy commentedReverting the $type_id/$type_name/$type etc. changes. Follow-up cleanup material.
Comment #122
fubhy CreditAttribution: fubhy commentedComment #124
fubhy CreditAttribution: fubhy commentedSorry for the noise!
Comment #125
sunAttached patch makes the template work (was unused) and updates it for HTML5 and stuff.
Comment #126
fubhy CreditAttribution: fubhy commentedWe should extend ConfigEntityListController and not EntityListController for ProfileType
Comment #127
sunAttached patch should address all issues that have been raised in #1668292-35: Move simplified Profile module into core (and some additional issues I encountered while stepping through the code and comparing it with other modules in core).
Comment #129
tstoecklertl:dr; green patch => RTBC by monday evening latest
I already read through this patch once and I plan to do that again, and to try it out in all possible use-cases. I might (or might not) find some minor stuff in the code, but going by my first pass I highly doubt I will find anything substantial to complain about. I might only get to all that by Monday evening, but in case there was a green patch by that time for me to RTBC, it would be an honor. :-) Awesome work, you guys! Truely impressive.
Comment #130
tstoecklerSo I just gave this a spin. The profile type management works beautifully but I did catch some weird errors. What I did was check out 8.x-1.x of profile2 and then apply this patch on top of it.
At this point I'm assuming I did something wrong, either way, there's not much more to test due to this. :-/
Some feature-y things I noticed during the test that would be really cool.
Comment #131
tstoecklerHere's that patch I was talking about. It's not a full re-roll, only an interdiff-patch, so as not to conflict with anything else going on in the background.
Comment #132
fubhy CreditAttribution: fubhy commentedYeah we talked about that already and this is what we started for that: #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)
Comment #133
sunWorking on this.
The new load arguments revealed a bug in the menu system: #1863502: _menu_load_objects() does not pass already loaded arguments to subsequent argument loaders
Comment #134
tstoecklerRe #132: I knew there was an issue for this! I had searched for ConfigEntity which apparently no one has typed in that issue. :-)
Re #133: Right, that makes total sense. (What doesn't make sense to me is that my PHP didn't complain about the type-hinting violation while the testbot did...) I hope to be able to update the tests over there and write some new ones Wednesday-ish unless you/someone beat(s) me to it.
Comment #135
sunFixed a couple of things.
Obviously, this patch won't pass without aforementioned core bug being fixed. Unless that core patch will land quickly, we should consider to go back to using %map in the menu argument loader.
Comment #137
sunAlright, now including temporary stop-gap fix.
Comment #138
fubhy CreditAttribution: fubhy commentedThe ProfileAccessController currently relies on the profile id being available. For createAccess checks that's not really the case (because it doesn't have an id yet). Is it ok to simply cache on 'NULL' in that case? Do we want to cache based on the profile type instead and go with $profile->bundle() as the cache identifier?
Comment #139
sunAdjusted access controller to conditionally use $profile->id() for existing profiles and $profile->bundle() for non-existing/new profiles.
Comment #140
fubhy CreditAttribution: fubhy commentedNice! RTBC when this comes back green!
Comment #141
sunThanks - committed :)
One more final tweak — the $form structure of injected profile fields in the user registration form looks unnecessarily complex to me.
Attached patch performs a straightforward simplification.
Comment #143
fubhy CreditAttribution: fubhy commentedThe proposed changes to the form structure look nice. I can tell that you distaste constructed array keys. :)
Comment #144
sunFixed the test failures — and split that test method out into a new, dedicated ProfileAttachTest.
One of my goals was to eliminate the ProfileEditTest entirely before core inclusion... I'm not exactly sure what functionality is still asserted in there that isn't covered by new tests already.
Comment #144.0
sunUpdated issue summary.
Comment #145
fubhy CreditAttribution: fubhy commentedCool. Maybe it's time for another patch over at the core issue.
Comment #146
sunThanks - committed.
Attached patch eliminates ProfileEditTest. And fixes a yet unnoticed fatal error upon deleting a profile. ;)
The CRUD parts of ProfileEditTest seem to be fully covered by the other existing tests already, so I removed that test method entirely.
Comment #148
sun#146: profile2.access-test.146.patch queued for re-testing.
Comment #149
fubhy CreditAttribution: fubhy commentedI guess I am guilty as charged for that fatal :/. The patch looks good again. Let's roll another core patch.
Comment #150
sunAlrighty. :)
Comment #151
sunChasing HEAD.
Comment #153
sunComment #155
sunJesus.
Comment #157
sunWow... #1852966: Rework entity display settings around EntityDisplay config entity introduced a DX that is a PITA.
Comment #158
sunFinally. Committed.
Comment #159
sunFinal clean-ups:
Only attach profiles for the account view.
Fixed phpDoc namespace references.
Clarified comments about create/update access being folded into edit access.
Clarified code/comment relationship for $pid determination in ProfileAccessController::access().
Clarified $access === TRUE condition in ProfileAccessController::access().
Use local variables $id and $label in hook_entity_info().
Comment #161
sun@fubhy figured fit fout. :)
Comment #163
sunUpdated for new Field UI permissions.
Comment #164
sunComment #165
sunComment #166
sunComment #168
sunComment #169
sunComment #170
sunUpdated for latest changes in HEAD.
Comment #171
sunAccounting for most feedback items from @webchick in http://drupal.org/node/1668292#comment-7081990
Comment #172
sunNow including additional "Save and manage fields" submit button on the profile type creation form.
Comment #173
sunComment #174
steinmb CreditAttribution: steinmb commentedBumping the thread. Where do we stand here?
Comment #175
nevergone CreditAttribution: nevergone commentedWhat can we do to move this forward?
Comment #176
kscheirerWell, its been decided that profile2 won't be going into core, so that part needs to be fixed up. The big thing we're missing (D8 release blocker) is providing an upgrade path from D7 core profile to D8 contrib profile2.
@nevergone, steinmb: review this patch! it's pretty old already, and d8 has been moving rapidly. We need a working D8 profile2 with upgrade path in order to release D8.
Comment #176.0
kscheirerUpdated issue summary.
Comment #177
sergiu.popa CreditAttribution: sergiu.popa commentedIf I want to help, I need to apply all the patches in order to continue the work?
Comment #178
anavarre172: profile2.patch queued for re-testing.
Comment #179
ebeyrent CreditAttribution: ebeyrent commentedAny updates on this project?
Comment #180
joshtaylor CreditAttribution: joshtaylor commentedI've been working quite a bit to move tests over to work again for D8, but I'm running into issues with ProfileFIeldAccessTest.
There is a PR up on Github, if someone could have a look at that, and I would love to discuss this further on IRC.
Comment #182
fagodakala and gozoo worked more on this on github. I moved that code into a repo of mine on Github, so we can collaborate there. Also granted bojanz access there to help moving on.
->
Work on the 8.x port is done on Github at https://github.com/fago/profile2. Please submit any improvements as PRs there and open issues for any discussions and issues in general here in the queue.
Comment #183
fagoFixing scope to porting only
Comment #184
paucku CreditAttribution: paucku commentedWhy the package in profile.info.yml is Core. Isn't profile2 a contrib module?
Comment #185
dakala@paucku: I think it's the ultimate goal to have this incarnation of Profile2 replace the Profile module that was removed from core.
Comment #186
bojanz CreditAttribution: bojanz at Centarro commentedSince the profile2 module is now named profile (and aims for core inclusion), we've moved it to the https://www.drupal.org/project/profile namespace. Full history has been preserved.
We've also done initial work for getting it up to date with latest core, and I will be giving it more attention soon.
See you in the other issue queue.
EDIT: Should also mention that I'm going to keep the GitHub repo (fago/profile2) in sync with the d.o repo for now.