Move simplified Profile module into core

+1 from my side - in fact, that was the vision when joachim and me started working on profile2.

There shouldn't be many advanced features, as I've tried to keep a slim base. However, there are two different UI approaches: First, there is the user account categories like approach, what mimics the core profile module UI and is default. Second, there is the profile_pages module what implements profiles as separate pages with a separate entry in the user menu ("My profile" besides "My account").

Big +1 here, too. Profile2 has been extremely solid in my use on a few different sites.

Both UI approaches are valid.

But I guess the approach that mimics the core profile module should be the one going in Drupal 8 core, whereas the other approach, still valid from a UI point of view, could still be made available in contrib.

@fago / @joachim: It would be great to learn from you which approach of attacking this you'd prefer. E.g., fork it into a sandbox (possibly Platform) and move on in there, or work in the Profile2 module repository (8.x-1.x branch) directly?

As the goal is core-inclusion, my feeling is that it should live in a core sandbox. Platform would be fine with me. However, I should note that I won't be able to be the main driving force behind that, as I've already too many construction zones. Still, of course I'm happy to help out as much as I can.

From the entity api view, I'd really love to have a clean entity use-case in core without carrying any historic-cruft around.

> Remove the (contrib) entity property API support code

Shouldn't most have this have made into core's entity module?

Another alternative for core would be the simpler approach taken by http://drupal.org/project/profile_lite.

I just checked its code, and it essentially seems to be similar to profile2, but has the main difference of attaching fields to the user entity and the 'user' default bundle

Does that mean that profile2 will not be attached to user bundle in D8?
What would be better as seen from many user requirements
1. The user account and user profile should be separate entities but related
2. User registrations on 95% of sites is just email and password or user name and password
the concept of "profile" is misread along with "account" in core
profiling should only describe meta data of user like interests, hobbies, profession etc as such they cannot be "account" related data
3, if some user meta data required or a site want to determine the user profile before registering to categorize users(i.e. account) then the fields from profile may be exposed
4.also we should keep in our plan, to accommodate future profiling tools which may come up from user contributions like profile2 and profile_lite i.e a choice should be available to use which profiling tool they want to use, current approach is like forcing to use one lacking in flexibility in choice
This is my thought process

I think user entity versus a new profile entity could be debated ad nauseum. There are just tradeoffs for each approach. I would take whichever patch gets to RTBC first :)

As for wanting a simplified registration form, have we improved the way we handle entity forms such that an admin can remove Fields from a form? I'm thinking that we want Manage Display for Forms as well. is there an issue for that?

some issues with hard coded fields (locale, signature and pictures) in user.module are being fixed by the way.
this should solve problems with changing/ordering user fields, categorizing them and also moving them to other pages
#967566: Several important Core User settings need to implement hook_field_extra_fields()

Possible changes related to Field API and bundles that could impact Profile2 for Drupal 8 (and even Drupal 7): #1040790: _field_info_collate_fields() memory usage

We already have 'manage fields' which lets you choose a widget, but I think Moshe means allowing admins to set that per-form. Sounds like we'd need a form view mode or similar, don't think that concept exists at all yet.

Well it kind of exists, we used that for a client project in a module that i pushed in a sandbox that you can see here > http://drupalcode.org/sandbox/Artusamak/1796634.git we called that "form modes" i will post an example ASAP if you are interested. :)

I'm interested.

The FormControllerInterface knows the concept of form view modes as $operation, e.g. EntityFormControllerInterface::getOperation()

UserFormController uses that to create a 'register' form and an 'edit' form for users.

There's no UI for setting that or anything, though.

I'm ignorant of how Profile2 handles the user:profile relationship; does it support 1:n per bundle? We'd like to use a core profile entity type if possible instead of carrying the customer profile entity type forward to Commerce 2.x on D8, but we need to support the idea that a user might have 4 billing profiles and 2 shipping profiles (or any combination thereof).

If this isn't the case currently, is there any reason the core use case could not support it?

> which I think maps exactly to your Commerce needs.

I don't actually think it does.

Profile2 currently allows you to create multiple profile types, but user X can only have one of each type.

> a user might have 4 billing profiles and 2 shipping profiles (or any combination thereof).

So this isn't possible.

(And I rather think it ought to be, for @rszrama's use case and many others.)

Other possible use cases for multiple instances of profile types include -

1. Educational background:
- primary school
- secondary/high school
- university/college

2. Employment history:
- ABC Ltd.
- XYZ Unltd.

Echoing @rszrama #21:

Does anybody know why profile2 doesn't support the idea of a user having more than one profile of a type? On a cursory look, it shouldn't be an issue as the module leverages the concept of entities and for content types a user can create more than one article, page or node type.

Does anybody know why profile2 doesn't support the idea of a user having more than one profile of a type?

The idea is that users always only have one profile, e.g. why should a user have multiple variants of a biography or forum profile? It's a natural 1:1 relation there.

ad #24:
I'd model that as CV profile or similar, having field-collection for the individually added items. Imo a school you visited is not a profile, it's part of a profile.

However, I agree that the commerce use case proofs that there are valid use cases where that 1:1 relationship to users does not hold. In that case I'd say the 1:1 relationship relates to orders.

So I think we should drop the 1:1 assumption out of profile2 core, but put it on the shoulders of consuming code to put that or another assumption (1:1 to an order) in.
Thus, the default UI could add the assumption and provide a simple way for developers to opt-out from the UI. Drupal commerce can then use that and handle UI + the relationship to users as desired.

I think it's mostly profile2_load_by_user() that applies the assumption, so that shouldn't be hard to change.

Alternatively, we could do a generic-entity reference and impose the 1:1 assumption on this reference. Experience shows that generic-entity references add complexity, thus I don't think that's the way to go.

Thanks @fago. Yes, the use cases in #24 can be achieved using field collection such as field_group as part of for example, a CV profile. There's no reason for not using a field grouping but I was thinking of a situation without a field collection module.

However, in legitimate cases a possible way would be giving the site admin the option of specifying how many instances of a given profile type a user can create. Maybe something like in field settings where you can specify Number of values: Unlimited, 1, 2, 3 etc?

I was actually investigating how to do this a while ago and felt that a change to profile2_load_by_user() would be a good approach. If it returns an array of profiles keyed by profile type, then the change wouldn't be too significant. Just a thought.

#25: That was the logic behind the old D5 Bio module. Same logic followed in Content Profile and now in Profile 2. I remember having problems on multilingual sites, since that was one obvious way to end up with multiple profiles of the same type. Entity translation would alleviate that specific need.

The permission system just sucks, all the permissions modules either core or contribs revolve around roles showing it in different angle without much of use . Permissions are assigned to a role. it is not possible to maintain privacy either for nodes or for profiles per user basis.if A role is given permission to view then all the users of the roles have view access .this is major draw back for a CMS .There should be flexible permission system where individual users can either be permitted or denied of an operation especially for the profiles.

@kaizerking if you have special needs you might help out in #1696660: Add an entity access API for single entity access and related issues.

@steinmb , I do not think it is my special need , I went through that Entity access APi discussion , which is almost going in the same old direction of role based system .Irrespective of any other access system being in place or not profile2 entity has its own special access/permission needs in terms of access restrictions.

Here we go, I read through the entire patch in detail, and have some comments. All of them could be fixed post-commit. Let me also say that for such a killer feature the amount of code debt introduced here (see the doc issues mentioned here, there are some todos in the code, and also this is the first fieldable config entity, which will need some follow-up consolidation) is ridiculously low.
Haven't tried this out yet, but having read the tests, this is almost not necessary.

+++ b/core/modules/profile2/lib/Drupal/profile2/Plugin/Core/Entity/Profile.php
@@ -0,0 +1,124 @@
+  /**
+   * Overrides Entity::label().
+   */
+  public function label($langcode = NULL) {
+    if (isset($this->label) && $this->label !== '') {
+      return $this->label;
+    }
+    else {
+      return entity_load('profile2_type', $this->type)->label($langcode);
+    }
+  }

I think this deserves a comment. I missed the '_type' part and was knee-deep trying to figure out why on earth there is no infinite recursion here...
Also, maybe you could explain the use-case that is being covered here. I'm not sure I get the point of this function.
Also, if only for reference, it should be noted that this would make 'profile' the only entity type which doesn't out of the box support altering in a 'label callback'. I don't think that is a bad thing, as we should be moving away from such callbacks anyway, IMO, but still.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.php
@@ -0,0 +1,128 @@
+    // Map to 'edit' access.
+    return $this->editAccess($profile, $langcode, $account);
...
+  public function editAccess(EntityInterface $profile, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return $this->access($profile, 'edit', $account);
+  }

Can you explain the indirection for introducing editAcess() here. And also why you're passing $langcode and then not using it?

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.php
@@ -0,0 +1,128 @@
+  protected function access(EntityInterface $profile, $operation, User $account = NULL) {
...
+  }

I'm usually not a fan of spilling other people's patches with @todo's but in this case I don't think we can get away without mentioning that this should be merged with node access into a generic entity access system.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileFormController.php
@@ -0,0 +1,54 @@
+ * Contains Drupal\profile2\ProfileFormController.

Just a note that the files introduced here are consistently missing a leading backslash in the namespace. I would suggest not to change that yet, though, as we might end up not requiring the namespace at all there. (It's safe to say, though, that if there's a namespace it should be fully-qualified, so...)

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileFormController.php
@@ -0,0 +1,54 @@
+
+  /**
+   * Overrides EntityFormController::actionsElement().
+   */
+  protected function actionsElement(array $form, array &$form_state) {
+    $element = parent::actionsElement($form, $form_state);
+
+    if (!$this->getEntity($form_state)->access('delete')) {
+      unset($element['delete']);
+    }
+
+    return $element;
+  }

This is nitpick-y but the override would be better placed in ::actions(), IMO vs. actionsElement().

+++ b/core/modules/profile2/lib/Drupal/profile2/Tests/ProfileEditTest.php
@@ -0,0 +1,175 @@
+    profile2_type_load('test')->delete();

This is super nitpick-y, and I personally don't really care that much, but before Crell sees this better change this to entity_load().

+++ b/core/modules/profile2/lib/Drupal/profile2/Tests/ProfileEditTest.php
@@ -0,0 +1,175 @@
+    // Try deleting multiple profiles by deleting all existing profiles.
+    $pids = array_keys(entity_load_multiple('profile2'));
+    $this->assertTrue($pids);
+    entity_delete_multiple('profile2', $pids);

Again very minor, but since we're already deleting them, why not try copy the $pids = ... line and then assertFalse() after the delete?

+++ b/core/modules/profile2/profile2.admin.inc
@@ -0,0 +1,59 @@
+/**
+ * Form submission handler for profile2_type_delete_form().
+ */
+function profile2_type_delete_form_submit($form, &$form_state) {
+  $form_state['profile2_type']->delete();
+  $form_state['redirect'] = 'admin/people/profiles';
+}

A drupal_set_message() would be standard here I think.

+++ b/core/modules/profile2/profile2.api.php
@@ -0,0 +1,51 @@
+/**
+ * @file
+ * This file contains no working PHP code; it exists to provide additional
+ * documentation for doxygen as well as to document hooks in the standard
+ * Drupal manner.
+ */

To be in-line with other file in core, we should simply do "Hooks for profile2.module" I think.

+++ b/core/modules/profile2/profile2.api.php
@@ -0,0 +1,51 @@
+
+use Drupal\profile2\Plugin\Core\Entity\Profile;
+use Drupal\user\Plugin\Core\Entity\User;

Don't blame me for this, but it is a standard that we don't "use" namespaces in api.php files, but instead inline the entire (fully-qualified) namespace.

+++ b/core/modules/profile2/profile2.install
@@ -0,0 +1,75 @@
+function profile2_schema() {
+  $schema['profile'] = array(
...
+}

Considering that a while earlier in the cycle we spent a significant amount of time adding a 'langcode' column to all entity's schemas I was surprised to not see one here. I think we should at least add the schema column and setting the default langcode on save or something, assuming that full entity translation support is non-trivial to implement.

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+  // Add bundle info but bypass entity_load() as we cannot use it here.

Sorry, but this definitely needs a @todo linking to #1822458: Move dynamic parts (view modes, bundles) out of entity_info()

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+        'path' => 'admin/people/profiles/manage/%profile2_type',
...
+        'access arguments' => array('administer profiles'),

Can you explain why the permission is 'administer profiles' and not 'administer profile types'?

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+    'load arguments' => array('%map', 'edit'),

This might totally be my sub-par knowledge of the menu system, but it's not clear to me why array(1, 'edit') is not sufficient here.

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+function profile2_menu_title_profile(Profile $profile) {
+  return $profile->label();
+}

It seems entity_page_label() could be used here.

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+function profile2_type_list_page() {
+  $controller = entity_list_controller('profile2_type');
+  return $controller->render();
+}

Any reason this is not in profile2.admin.inc?

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+        '#type' => 'fieldset',

This, I wouldn't have expected from you :-)

+++ b/core/modules/profile2/profile2.module
@@ -0,0 +1,596 @@
+function profile2_profile_edit(Profile $profile) {

Any reason this is not in profile2.pages.inc?

Awesome review, thanks!

I think this deserves a comment. I missed the '_type' part and was knee-deep trying to figure out why on earth there is no infinite recursion here...

Good point, will add that comment. There is at least one use-case for this that I can think of right now: 1:n profiles (multiple profiles of the same type per user) e.g. in case of Commerce where they probably want to have multiple invoice addresses. In that case it would make sense to allow their customers to define labels for their invoice addresses... (maybe?! :))

Regarding the 'label callback'... I think so too, it should be deprecated, just like uri callbacks, etc. I think we can keep it as it is without the label callback.

Can you explain the indirection for introducing editAcess() here. And also why you're passing $langcode and then not using it?

Because profile2 code uses 'edit' instead of 'update' / 'create' and so it maps to editAccess(). @see $entity->access() (it's basically $controller->{{$operation}Access}).

I'm usually not a fan of spilling other people's patches with @todo's but in this case I don't think we can get away without mentioning that this should be merged with node access into a generic entity access system.

Yep, @sun and I also talked about that and yes, we will do that eventually. Adding a @todo there sounds good to me.

Just a note that the files introduced here are consistently missing a leading backslash in the namespace. I would suggest not to change that yet, though, as we might end up not requiring the namespace at all there. (It's safe to say, though, that if there's a namespace it should be fully-qualified, so...)

Interesting, I thought in the @file declaration we were supposed to leave out that leading backslash.

This is super nitpick-y, and I personally don't really care that much, but before Crell sees this better change this to entity_load().

It's not nitpick-y, thats more than valid... I tried to remove ALL calls to those functions. Guess I missed one or two. Will fix that too.

This might totally be my sub-par knowledge of the menu system, but it's not clear to me why array(1, 'edit') is not sufficient here.

I think we wrote that at a time where we had a more complex route in there... Yes, you are right, array(1, 'edit') would be sufficient.

I agree with most of your other concerns. Will fix them (as usual) over at #1726822: Port Profile2 to D8.

Awesome, I wasn't aware of that issue. I'll post over there in case I have any more complaints. This smells like RTBC, though...

I hate to ping anyone for just tagging this issue, but there is no universe in which this issue doesn't have the "Killer End-User Features" tag.

Something I'm not sure I understand here: Is the idea that Drupal core would ship with two separate ways to add "user profile fields" via the admin interface? (The one we have now, where the fields are attached to users, and another one where you do it via a separate profile entity.) How does that jive with the discussion that happened about this in Drupal 7 in #608894: Resolve the conflict between the fieldable users UI and the profile module - in particular @webchick's thoughts in the issue summary: "It's a critical UX issue to ship Drupal with two different ways to add fields to users."

There are also some other interesting site building usability issues I've come across with profiles as a separate entity. For example, if you want to use Views to make a list of all users whose favorite color is green, that's relatively straightforward if "favorite color" is a field attached to users directly. With a separate profile entity, though, you have to start getting into relationships to make it work.

ad #41: We have to make a separation between ACCOUNT and PROFILE data at some point. Account data being the actual login information and profile data being anything from personal information, education, billing & invoice addresses, etc. ...
An account is always a 1:1 thing (user:account). Profiles can be 1:n (e.g. you might have multiple addresses).

There is also a performance issue related to adding a lot of field directly on the user object. Every time we need the user object we also have to load a lot of prob. unnecessary fields.

I think we would best simply remove the admin UI for attaching fields to users. The 80% use-case is the "Profile" use-case, i.e. adding a "Bio" field, a "Hobbies" field, etc. I think users should still be fieldable, because there are use-cases for fields on *accounts* (i.e. an admin-only field tracking if a user has trolled, a "Karma" field (arguable)), but the UI will be confusing. And it will be a <20 LOC contrib module to add that UI back in.

All that said, unless we reach an agreement here quickly, I think we should handle this in a follow-up. That is because architecturally "Profile" entities are what we want, I haven't seen any pushback on that. And figuring out where and how and if "User" and "Profile" deserve a Field UI, is just a matter of switching some keys in the entity annotation and will be trivial to code-up, once we know what we want UI-wise. Even completely making the "User" entity un-fieldable (though I would not suggest that) would be a trivial patch.

#45 make sense. I have often seen D7 users with profile2 installed struggle and getting confused trying to create user profiles.

I agree with #45 as well. I think this patch has to move the user_picture Field from user entity to profile entity.

I would hate to see us remove the ability to have fields on users. Sometimes there is information that is needed every time the user is loaded (like login information used by SSO) and it would be ridiculous to require joining in a profile every time. The UI for adding fields to users is already pretty hard to find, I can't believe many people just stumble over it. If we want to ship with something that defaults to a 'common' use case and we really think people are going to be able to find and then be confused by both of these field UIs, make the UI for user fields a separate module and ship with it disabled. The description for enabling the module can provide links to explanations of why you might use it over adding fields to profiles. That user field UI could easily stay in core without hurting anything if it is just disabled by default, and it's easily used if needed.

Full agreement with #89 and #90.

Many sites can just have a few simple fields on the user account, like phone number and address, and be happy. For one site I run, though, there are a few fields we keep on the user account itself, and the rest are in profiles. This makes it easy (and faster) to load users and get basic information most of the time, and lets us load a full profile only when we need to.

I presumed from the outset this was the goal of this issue; not to supercede fieldable users...

I also agree with the comments above. Removing the fields ui for any entity type never did anyone any good.

Alright, now let's get back on-topic and discuss the patch :). Most of what has been raised in the previous comments should be moved to follow-ups!

> Profiles can be 1:n (e.g. you might have multiple addresses).

Is that doable in this patch? It's not in D7 Profile's UI. +1 for the feature, BTW.

> And that's exactly what we're facing here: Profiles have a different business logic than User accounts. Both can be fieldable, and there are various and excellent reasons for attaching a field to one over the other.

Agreed.

Can we resolve the UX problem with some help text that explains the difference between the two and roughly when you'd want to use each one, to be shown on the field UI for both users and profiles?

1) I don't think a profile2 name makes a sense. The patch uses mix of profile/profile2 names within
2) what's about upgrade path/tests? is there an issue?
3) not sure that patch should introduce it's own attach to user. maybe better make it depend on ER issue?
4) is there a performance tests? it seems we could have a huge regression, having to load all dependent profiles on user

PS:
I strongly disagree removing fields from users - D6 => D7 => in most of cases will have only one profile if any at all on user entity.

D7 still have troubles with relation 1:N in entity api see:
#1011370: Ctools relationship from user
#1874006: Expose entity references as CTools relationships

1) I don't think a profile2 name makes a sense. The patch uses mix of profile/profile2 names within

Of course not, it's the last step in our priority list over at #1726822: Port Profile2 to D8.

not sure that patch should introduce it's own attach to user. maybe better make it depend on ER issue?

We certainly don't want to load field values for profile attachments every time we entity_load a user object.

4) is there a performance tests? it seems we could have a huge regression, having to load all dependent profiles on user

We don't load profiles... It would be a performance improvement at best because of that exact reason. Currently everything you put on a user object is loaded when you entity_load a user object. With profiles being separate objects we only load them when actually required (e.g. when viewing a profile). But yeah... we should test that.

Can we resolve the UX problem with some help text that explains the difference between the two and roughly when you'd want to use each one, to be shown on the field UI for both users and profiles?

I guess we'd have to try, and would also need to look at how to phrase this in the module description, since the module being named "Profile" (and I can't think of a better alternative) carries with it the implication that you'd use this for all user profile needs, whereas the reality is more complicated.

We don't load profiles... It would be a performance improvement at best because of that exact reason. Currently everything you put on a user object is loaded when you entity_load a user object. With profiles being separate objects we only load them when actually required (e.g. when viewing a profile). But yeah... we should test that.

I think the performance concern is that in situations where you do need profile data, you'd now be loading two entities (with all the associated overhead of the loading process for each) rather than one. Also, views that use these would have extra JOINS.

However, this is a standalone optional module so as long as no core functionality is being forced to depend on this, I don't think the performance testing would be a requirement for this issue? Probably the exact effect on performance (pro or con) depends on the individual site and the details of how it's using this module anyway.

Generally this is looking awesome, good work everyone.

We will need views integration for this too; as profile2 previously relied on entity api to provide this for it. I'm happy to do this, do you want it in this patch or a follow up? I'm fine with either.

Here are a few small things:

+++ b/core/modules/profile2/lib/Drupal/profile2/Plugin/Core/Entity/Profile.phpundefined
@@ -0,0 +1,126 @@
+ * Contains Drupal\profile2\Plugin\Core\Entity\Profile.

+++ b/core/modules/profile2/lib/Drupal/profile2/Plugin/Core/Entity/ProfileType.phpundefined
@@ -0,0 +1,71 @@
+ * Contains Drupal\profile2\Plugin\Core\Entity\ProfileType.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,114 @@
+ * Contains Drupal\profile2\ProfileAccessController.

nitpick alert: I think we're adding a prefixed '\' in the file docblock nowadays.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,114 @@
+    // Map to 'edit' access.
...
+    // Map to 'edit' access.

Not sure we need these comments? I think it's obvious enough. If we really do, should they be on all the *Access methods?

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,114 @@
+    $pid = $profile->id() ?: $profile->bundle();
+
+    $uid = $account->id();

nitpick alert: $pid and $uid assignments should sit together then a space before the conditional. That's just me though :) I think it looks weird how it is.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,114 @@
+        $access = $return;

This assigns the variable but the TRUE check below just assigns TRUE directly. I think we should use one or the other.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,114 @@
+    $this->accessCache[$uid][$operation][$pid][$langcode] = ($access === TRUE);

Couldn't this just use $access?

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileFormController.phpundefined
@@ -0,0 +1,50 @@
+   * Overrides EntityFormController::actions().

We need to add the full \Drupal\profile2\ProfileFormController::method namespaces. I think this goes for all the entity methods.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileTypeFormController.phpundefined
@@ -0,0 +1,69 @@
+      '#description' => t('The human-readable name of this profile type.'),

of or for, I'm not sure. :)

+++ b/core/modules/profile2/lib/Drupal/profile2/Tests/ProfileCRUDTest.phpundefined
@@ -0,0 +1,159 @@
+class ProfileCRUDTest extends DrupalUnitTestBase {

Can't tell you how much I'm enjoying all these unit tests.

+++ b/core/modules/profile2/profile2.api.phpundefined
@@ -0,0 +1,50 @@
+  if ($profile->type == 'secret' && !user_access('custom permission')) {
...
+  if ($profile->type != 'main' && user_access('custom permission')) {

I always like to wrap the checks in another set of parenthesis here like ($profile->type == 'secret') but that's just personal preference I guess.

+++ b/core/modules/profile2/profile2.moduleundefined
@@ -0,0 +1,548 @@
+    $info['profile2']['bundles'][$config->get('id')] = array(
...
+        'real path' => 'admin/people/profiles/manage/' . $config->get('id'),

We could just assign an $id variable for these.

patch applied nicely. But i dont know where to look for profile now? any help please
EDIT:Sorry i got it at admin/people/profiles

it looks fine great work thanks to all of you,
i dint understand on how to change or access view modes though

Patch applied cleanly but I got this warning:

Warning: Missing argument 2 for profile2_menu_load() in profile2_menu_load() (line 143 of core/modules/profile2/profile2.module).

Created a profile type, cleared the cache but the warning hasn't gone away. Not sure why I'm getting the warning.

@sun: That's correct - I used an earlier version. I've now reapplied the #61 patch and it's as clean as a whistle. Works fine, no warnings.

Patch looks RTBC now, suppose views integration should be done as follow-up. Needs new one.
Also we need upgrade tests. And issue to convert D7 "profile" to this.

The only question here about BC - if you change a name to *profile* how to solve the "duality" in upgrade from both profile(2) modules? Some sites are D5->D6->D7 and could have both modules installed :( The issue #1261576: Profile module data is not upgraded

I mentioned views integration above, I have done alot of this. So will have a patch ready for a follow up on that. Just kind of tracking what goes on here.

Sun has addressed any points I raised in my review in #60, So I think I'm happy. Should we get a few more eyes on this?

Hmm.. This issue has been sitting here for quite some time now and got quite a few reviews already. I think we can safely RTBC this now. I wrote a few LOC for this patch too but given the earlier responses I believe it's ok if I RTBC it?!

Are we ready to go here? I'll wait for an RTBC before considering the final module rename from profile2 to profile. (That said, we could as well consider to do that rename directly within core.)

Hit it!

Nice, happy with this. Created a followup, which I will action soon to add view integration: #1894770: Add views integration for profile2 module

of course :)

Great work sun.

First off - great work everyone! I'm sry for not being involved more, but I finally had a first look over the code now. Here some first questions / concerns:

- D7 profile2 module had simple flags for making display at user accounts and the user categories integration optional. This is quite useful once you provide a different UI, what I expect will be required for quite some use-cases - e.g. for something along the lines of profile2_pages (separate entity page) or commerce customer profiles. So what are the reasons for removing this (API only) profile-type settings? Allowing modules to easily disable built-in UI is imho important for easy re-usability.

- Latest profile2 module for 7.x now implements extra-fields, I think this should have them also - such that you can move around the profile2 registration form and/or the profile2 display on the account view.

- As the requirement for the 1 profile (per type) to 1 user relationship is gone, what works for me. However this does not match with the editing UI, what is only able to handle 1 profile per user. But what happens if somehow more profiles end-up in the db, e.g. due to some other module. Maybe the 1-1 restriction should be controlled by a setting and en-forced/validated if there?

- Property names are still $uid, $pid, .. - I think we should align them with the pattern outlined by #1233394: [Policy, no patch] Agree on a property naming pattern.

- What about implementing the EntityNG API?

-

* @param Drupal\profile2\Plugin\Core\Entity\Profile $profile

This misses a leading slash - afaics that problem is all over the module.

- We should improve the profile2 form embedding to based upon the entity form instead of the field form - this needs more work and should be a follow-up.

Setting to needs-work for clarifying the UI control-flags and how we handle the not enforeced 1-1 relationship. I don't think those have to be fixed before commit, but at least we should have clear way forward for them.

+++ b/core/modules/profile2/lib/Drupal/profile2/ProfileAccessController.phpundefined
@@ -0,0 +1,117 @@
+  protected function access(EntityInterface $profile, $operation, $langcode, User $account = NULL) {

+++ b/core/modules/profile2/profile2.api.phpundefined
@@ -0,0 +1,50 @@
+function hook_profile2_access($op, Drupal\profile2\Plugin\Core\Entity\Profile $profile, Drupal\user\Plugin\Core\Entity\User $account) {

Is there any reason argument order changed for the hook and why are we not adding $langcode same ashook_node_access?

I just wanted to get this out, and I apologize that it doesn't really contribute to the patch reviews. I just don't really think that adding Profile2 gives us a major "win" for 80% of site users like Views, WYSIWYG, etc does. I'm not saying profile2 is a bad module at all and that it has no use case, NO. NOT AT ALL. I'm just saying that I don't agree with merging this module into core since it can live on in contrib and continue to innovate. Is there any reason that this module would *need* to be added to core? Are the majority of our users installing this functionality on the majority of their sites?

I think it would be great if we could push momentum into migrating the deprecated profile module data into user fields. Even if we had profile2 in core, we'd still need to migrate the old field data. I think we could get a better win by adding something like Field Groups into core so that fields on users can be logically grouped into the old profile categories.

Again, I don't want to derail anyone's work, I just wanted to get my opinion out. We have to ask ourselves hard questions as we get closer to deadlines.

@Dave Reid: The goal is not to add a new core module. But to update the core Profile module with a more robust and modern one.

If Profile 2 is not ready to replace the core Profile module, it will keep its name and stay in contrib.

Patch seems to work great in terms of adding profiles and fields to them.

• There is no Profile type yet. -- This text shown when there are no profiles, should this not be plural, or type(s) (not sure what the convention is).
• A user seems to be unable to edit their profile(s) on the user account edit screen (user/*/edit) when clicking the links -- receiving an access denied.
  

I missed that there are permissions per profile type (ie, Edit own profile) so disregard my 2nd comment above.

Profiles are a necessary for almost every site out there. This looks great, hope it goes through.

1) They inherently imply direct loading of all field data for every loaded user account whenever it is loaded, which is a memory + performance disaster.

Exactly. With profiles as separate entities you have the flexibility - if you want to include profile data in your account you can still do so via account fields or embed the profile form via modules like inline-entity-form.

Setting to needs-work for clarifying the UI control-flags and how we handle the not enforeced 1-1 relationship. I don't think those have to be fixed before commit, but at least we should have clear way forward for them.

So what about the following:
- open a follow-up for caring about validating the 1-1 relationship in case this applies to the profile type. This could be an optional validation constraint, or an additional entity reference for optional attaching the profile to another entity (configured per bundle). This reference could be used for making sure there is just 1 profile per attached entity + it can be the base for adding in profile2 display extra-field integration or user-register extra-form integration.
- make sure default UI can easily disabled by bundle in a follow-up, such that the module can serve as a good basis for stuff like commerce profiles or profile2-pages like UI

#79: platform.profile.79.patch queued for re-testing.

1) They inherently imply direct loading of all field data for every loaded user account whenever it is loaded, which is a memory + performance disaster.

As discussed above, "memory + performance disaster" is unlikely to always be true. It really depends on the site and in some cases it can be the other way around.

I also think Dave Reid makes a very good point that hasn't really been addressed yet. It is not a knock against Profile 2 at all, simply questioning whether it represents an 80% use case or anything near that... does it? In my experience fields attached to users are definitely the 80% (or more) use case.

That said, this patch is most likely further along than any other solution that could be used to migrate data from the previous Profile module. However, I think the usability concerns mentioned above (about shipping Drupal core with two separate ways to add user profiles) are still worrisome. The solution that was agreed to above was basically "we should solve that with help text" but it's not clear what the details would be or whether that's even a real solution :) Adding the "Needs usability review" tag (to this issue for now, rather than a hypothetical followup) since at some point it should be addressed.

However, I think the usability concerns mentioned above (about shipping Drupal core with two separate ways to add user profiles) are still worrisome

While people tend to use it that way, I do not think user account fields are a user profile nor does Drupal 7 present it that way. The user account is as the name says, the user accounts and as such represents account information such as user name, login password and site settings. Yes, it can serve as a simple profile replacement, but it's not as Drupal core would make user think of it that way.

In my experience fields attached to users are definitely the 80% (or more) use case.

Imo the 80% use-case is to have private profile fields, which user fields do not cover (without extensions).

Dupal 7 lacks a solution for storing user-related information efficiently. Regardless of profile UI (integrated or not) profile entities are a sane and scalable solution for that - while it can be the base for any kind of profile UI. I really think Drupal could need a proper solution for storing and keeping track of user data built-in. Well, in today's world knowing your users and engaging your user becomes more and more important, so I think having a built-in way for keeping track of user-data would be well-received.

ad #86, thanks that works for me, thus setting the status back to RTBC as it was before. (Not sure what's the usual status related to the requested usability review, so please change it back if suiting.)

I just tested this on simplytest.me and I got following error message - not sure if this is related to the hook_menu_local_tasks - but I don't think so .. needs a reroll anyway.

InvalidArgumentException: The entity (profile2) did not specify a render_controller_class. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 225 of /home/s257bb43377c4cf2/www/core/lib/Drupal/Core/Entity/EntityManager.php).

• Create a profile type with on textfield
• Fill in the profile on user/x/edit/profiletype
• Go to user/x

Indeed, the render controller appears to be missing. Fixed that and re-rolled the patch.

ops, forgot to merge in latest changes. Above patch contains unrelated hunks, this one fixes that.

#91 happened because of #1882240: Remove default assignment of render_controller_class in EntityManager, and the interdiff in #92 looks good to me.

Can someone post screens to review? I can try simplytest.me but I'd rather not spend all my time trying to find this functionality.

I'm also worried about having two different systems in core for this, and I don't think it can be solved with help text. Is there a meta issue where the bigger issue is discussed? It's hard to decide whether it makes sense to have two system without seeing what it will take to get rid of profile.module. I'd like to see what the next steps are, and how we can get replace the original profile module.

I did an intial walkthrough this UI, I got a bunch of errors on a clean install - perhaps thats because commits are being made every minute. At large I noticed a couple things:

1) It does not follow common patterns for "bundles", for example upon adding there is only a title and one setting, is that really all the functionality we offer?

2) Much of the labeling it uses is not in line with core defaults (e.g. even the module is still called Profile2, it should just be Profile). Its in a "other" module section.

3) The field UI seems broken?

4) There should be a default profile, it doesn't make much sense to start with no profile that should be part of standard.

Thats all I can do for now, seems like much of this is still bugged. And I share @Dries his concern there is a larger issue here that needs to be adressed, because we are kinda in a confusing place if this goes in.

I couldn't replicate any errors on install, though this is not exactly a 1:1 replacement for Profile module, either. The UX impact is pretty steep.

Profile today

To re-cap, here's what Profile module did:

From a configuration screen, you can add different fields of various types, and can group them together into field groups.

These field groups then end up displayed on the user's profile page, and displayed as sub-local tasks on their profile (Drupal, Personal information, Work, in the case of Drupal.org):

One of the more useful features of Profile module is a "freeform list" field, which lets you display a nice list of users who have the same value listed:

Here's what Profile 2 module does:

Profile tomorrow

Profile Types

First, you set up one or more profile types. These are basically corollary to the fieldsets in the old Profile module.

On those you can add fields, and get a standard Field UI screen for doing so (this starts out totally blank, which I agree with Bojhan looks odd, but OTOH I'm not sure what default assumptions we could make here are, unlike titles in the case of nodes):

Visual changes

Like Profile module, this shows up in the local tasks of the user profile, and as headings in the user profile view / registration form:

Extra steps

Unlike Profile module, there's extra set up involved to use this in a practical way. For example, you need to set the following highly granular permissions very carefully, in order for the module to do anything but spit out 403s to unprivileged users:

And also unlike Profile module, there's extra set up in order to use e.g. Term reference fields as free-form list fields. If you simply add the built-in "Tags" vocabulary, you get this very confusing behaviour:

(But ... but... I just clicked on something tagged with that!)

As far as I can tell, in order to replicate this behaviour, you have to set up your own separate Vocabulary, and your own separate User View (rather than Content view) and then re-write URLs somehow in the user profile page so they point to user_tags/1 instead of taxonomy/1... I'm actually not sure how to do that without custom theming.

And finally, getting a list of all users ala http://drupal.org/profile requires Views-fu. There are no default views that come with this module to show something simple.

UX Feedback

Three biggies for me:

1) After adding a profile type, I expected to be forwarded to the manage fields settings, like I am with content types (or at least given the option to go straight there, ala the "Save and manage fields" button). It took me awhile to figure out that there was a "manage fields" option under the drop-button there. Most normal people are not going to guess that this is an entity and know to look there, and then be confused when they click "edit" and don't see any field options there.

2) The permissions thing was very frustrating. It would be much better for these view/edit "own" permissions to default to visible to authenticated users, IMO. I know that's not very standard, but the alternative is users not being able to fill out their own profiles, which is the entire point in 99% of use cases.

3) The taxonomy behaviour was also utterly bizarre. Is there an actual workaround for people who need freeform list fields on users?

Other minor miscellany:

• I wouldn't put Profile Types on admin/people, personally. It feels like something you'd set up once or maybe twice and that's it, so I'd put it under admin/config/people instead.
• I'd like to see the "Show in registration form" checkbox value exposed on admin/people/profiles; that seems like useful data.
• Why is delete a tab on profile types? It's not on content types.
• From my user profile, it's odd to see a Delete link on the profile that says "Are you sure you want to delete your Work profile?" when I delete it. Most users won't think of these tabs as separate profiles, but rather just fields on their profile.
• If a profile type has no fields to fill out, don't show its local task under user/1/edit
• It doesn't look like user profile output is themed in any way? The blue links on Bartik that don't go anywhere look fairly bizarre.

Errors

On main user/1/edit page:

User error: "count" is an invalid render array key in element_children() (line 6098 of core/includes/common.inc).
Warning: Invalid argument supplied for foreach() in profile2_menu_local_tasks_alter() (line 190 of core/modules/profile2/profile2.module).
Notice: Undefined index: count in profile2_menu_local_tasks_alter() (line 220 of core/modules/profile2/profile2.module).
User error: "count" is an invalid render array key in element_children() (line 6098 of core/includes/common.inc).

When I click "Work" sub-tab under Edit, I get those same errors, and the sub-local task for "Work" appears twice:

When I create a user as an admin through admin/people, then log in as the user and attempt to edit my work profile, I get access denied. If it's truly access denied, I shouldn't see the local task tab in the first place, no?

When I added the Work profile to the registration form and attempted to register a new user, I got:

Fatal error: Call to a member function bundle() on a non-object in /Users/webchick/Sites/8.x/core/modules/taxonomy/taxonomy.module on line 1039

Registration fails.

I'm going to have to call this 'needs work'. :\

Oh, one other permission-related thing that was terribly confusing for awhile there... despite careful checking off of permissions I was still getting 403 when user 2 tried to view user 1's profile. That's because user.module defines a "View user profiles" permission in a *totally separate place* from Profile2's settings. I'm not sure if any normal people would ever figure that out. :(

Tagging as something that was RTBC in time for feature freeze.

On the upgrade path. If this ends up staying in contrib and an upgrade path gets written there, that'd be enough for me. Not sure what others think about that but my main concern would be that it has somewhere to go, not that it necessarily gets upgraded by core. Poll upgrade path is also handled by contrib.

I'm not convinced that this needs to be in core, and I agree with David Rothstein that the performance/memory disaster of not having it available all the time is far from clear cut. I've seen performance issues on sites specifically from having separate user accounts and profiles.

For example if you only want first and last name + avatar then fields on user accounts work fine for that and will be lighter weight than a separate profile entity. There's also functions like theme_username() that require an account object, so for things like that you often end up loading both the account and the profile to build the themed username, which makes things worse rather than better.

I've been able to replicate the basic profile module capability and upgrade path with a small contrib module, so making Drupal.org run on D8 isn't impossible. If the goal is to run Drupal.org entirely on core and nothing else, then that is something else. I just still think this isn't quite mature enough yet to land in feature freeze. The work done here goes right back into contrib and we could even possibly use Profile2 on drupal.org with D7 or D8.

I've been following this thread for a while now, and Id like to contribute to the core/non-core debate.
IMO Profile2 should not be in core, and this is why:

• Profile2 is a successor of the very popular Profile module, which solved a clear need: add fields to users. This however now resolved in D7: users are entities, entities are fieldable. If it wasn’t for the huge success of Profile, Profile2 wouldn’t be a success either.
• I can’t see why drupal.org should not rely upon contrib modules. Profile2 could be one, Profile_2_Profile2_migrate could be another. Adding modules to core does not diminish the work load.
• Account vs. Profile 1: for lots of sites, the difference is none; the extra load time is minimal. If you have lots of fields, try adding Entity_cache to core! Indeed, if you need to protect the fields from your user, we need a facility. Several contrib’s provide this facility ( ABT has a nice one.)
• Account vs. Profile 2: Profile2 has a strict 1:1 relationship to user. However, lots of (CRM-type) sites need profiles without users, or users with more profiles. There are several initiatives (CRM Core, Redhen, party) that are working on this issue. Adding Profile2 to core may hinder the emergence of a nice, more generic and more sophisticated solution, instead of only the d.o. use case.
• And last but not least: as a (modest) contrib maintainer and contrib consumer, I see a lot of feature request passing by, like: “Add support for Feeds”, “Add support for Migrate”, “Add support for Profile2”. The first 2 seem valid feature requests to me, but the third one seems a datastructure flaw. Consider merging the Profile2 data with the user-data upon loading, making it transparent for the site builder.

Consider merging the Profile2 data with the user-data upon loading, making it transparent for the site builder.

Profile2 already exposes a property-info relationship to account objects in d7, i.e. account:profile-$type. While in d7 this API is a contrib extension and not used everwhere, it helps already with e.g. tokens. However for d8, we could do the same with the new entity-field API. So we'd get a straight-forward API to use like $account->profile-$type->field->value and likewise full exposure to all contribs. I'd say that makes it rather easy to leverage a profile for a site builder.

Account vs. Profile 2: Profile2 has a strict 1:1 relationship to user. However, lots of (CRM-type) sites need profiles without users, or users with more profiles. There are several initiatives (CRM Core, Redhen, party) that are working on this issue. Adding Profile2 to core may hinder the emergence of a nice, more generic and more sophisticated solution, instead of only the d.o. use case.

Please read the issue, the module does not have this restriction any more.

Account vs. Profile 1: for lots of sites, the difference is none; the extra load time is minimal.

If it's a simple profile that might work out, but if you store tons of fields - even access protected stuff - for your users, the time and memory overhead could be quite big. All this data would have to be loaded needlessly when loading the global user object or when loading lots of accounts just for displaying the author information of e.g. comments. We need to optimize for this cases, just loading everything we have surely is not properly optimized.

I agree with fago on this,

Account vs. Profile 1: for lots of sites, the difference is none; the extra load time is minimal.

Account is basic thing for interaction or entry into sites especially when we are talking about dynamic sites.a simple email and password thing. after getting access what user should do/are allowed to do is individual requirements, one of them is profiling, profiling a very wider concept, there can be product profile, user profile,service profile. "profile" means which describes the nature and attributes and tangibility of the object being profiled,one of them may be user. so you cannot say the account = profile.
this has already bean discussed herehttp://drupal.org/node/1668292#comment-6397858.

I personally feel, after so much of work is done, it is not right time to go back to square one and discuss whether, account = profile or account != profile,after 7 months of initiation!!!

#101: platform.profile.101.patch queued for re-testing.

So, this initiative is dead? Do we continue in contrib?

@Bojhan: Webchick made an exception for Profile 2 in #comment-7082560. Let's see how it goes. :-)

Patch outdated. Where's sandbox?

Afaik it's all in profile2's 8.x branch.

last commit 3 months ago

Account vs. Profile 2: Profile2 has a strict 1:1 relationship to user. However, lots of (CRM-type) sites need profiles without users, or users with more profiles. There are several initiatives (CRM Core, Redhen, party) that are working on this issue. Adding Profile2 to core may hinder the emergence of a nice, more generic and more sophisticated solution, instead of only the d.o. use case.

Following on from our work here #355513: Create an anonymous user API We are now working on doing this in contrib instead. If we do this well it will make Party essentially obsolete and just work with anything that works with users. Party actually already extensively uses the Profile2 module and so if Profile2 was in core, other modules (simplenews maybe? Commerce Customer Profiles) may be more likely to use it and so it would help us quite a bit rather then hinder us.

Working on it here. https://drupal.org/sandbox/rlmumford/2037879

Account vs. Profile 2: Profile2 has a strict 1:1 relationship to user

Regarding
Users with many profiles #1919536: Restore profile flag to allow multiple profile instances per user
Profiles without users #1919540: Allow contrib to disable default integration of a profile type into user account UI

So it doesn't look like Profile2 has to have that. But it does currently? right? as it doesn't look like those two issues have gone further forward. Also that second issue is about attaching profiles to other entities but it doesn't allow profiles to exist on their own right?

Not convinced this is blocked on a usability review :-)

I agree with sun.

When you make a site that has heavy profile2 usage you quickly run into the issue of other contrib modules being compatible only with users and not profile2 entities.

Having at least the backbone of the profile system in core should help that issue hugely.

Yeah, I don't need convincing this is a good idea :-)

So, what's next? Looks like we need to find agreement on what's needed to fullfill

By introducing an architectural concept for user profiles, Drupal core does not only provide a (limited) set of functionality. Much more importantly, core lays out a base building block that allows an entire ecosystem of contributed and custom modules to advance on it.

Updated issue summary.

@All: As the existing code is rather out-of-date and this issue has been inactive for a while, I've started on making the existing code work with the current core (alpha11) version. At the moment, you can add/edit profile types.

You may follow the effort here - https://github.com/dakala/profile2. Thanks.

Thanks @dakala. Did you start from scratch for that? You should really start off of the git tree right here so we don't lose the history. Merging it later will make it absolutely obscure.

@fubhy: Quite happy to attach patches to this issue. Have contacted @sun, still waiting for his response. Maybe he'll have other ideas how to move this forward.

In that case, the first step would probably be a new patch for simply chasing D8 HEAD.

Here's the first patch that makes the 8.x-1.x branch compatible with the latest D8. Among other things:

1. Reorganised directory structure
2. Added profile2.routing.yml
3. Added profile2.local_tasks.yml
4. Added profile2.local_actions.yml
5. Updated ProfileType and Profile entities

A default main profile type is provided. You can now add/edit profile types

New patch making 8.x-1.x branch of profile2 compatible with latest D8 alpha-12. Apart from features listed in #123 above, when editing user settings, a profile form is displayed if the profile type has fields attached.

Screenshots for #124

Very nice work!
I did just look at the screenshots though.

Thanks dakala for working on this. Is your Git repository based on the 8.x branch of https://drupal.org/project/profile2 ? Your Git repo seems empty to me, but you could just start of from the 8.x branch of profile2 such that we can merge your changes in in case this issue goes nowhere.

@fago: That Git repository (https://github.com/dakala/profile2) is currently empty because the previous work I was doing wasn't based on the drupal.org project. Subsequent to https://drupal.org/node/1668292#comment-8794983 and https://drupal.org/node/1668292#comment-8795167 I started working from 8.x-1.x branch of the project and created the attached patches.

I think I'll go ahead and continue pushing my work off the 8.x-1.x branch to my github repository then.

@fago: I've just pushed everything to https://github.com/dakala/profile2 (8.x-1.x branch) until we know what we want to do with this issue which keeps getting longer.

Have taken a stab at 1:n profiles per bundle and the display on the user account page. Screenshots attached. Updated code in https://github.com/dakala/profile2 (8.x-1.x branch)

Shouldn't the admin UI for profile types and their fields either go alongside the admin UI for fields on users, or under Structure, rather than under People?

It's possible to re-arrange the admin UI. Picked up from the old 8.x-1.x branch and some comments in this issue. Let's have more ideas of moving this forward as it'd been untouched for quite some time and there's a need to keep up with core while we're discussing our implementation.

So which one do want to go with? Under Structure or fields on users? For me having it alongside fields on users may be confusing.

Under structure. I think a profile will often be percieved as a (special) content type, so grouping it close to content types, blocks etc. seems the way to go.

Profile types was under Structure in D7. Probably the way to go for D8?

Admin UI now under Structure (as it was in D7 version). Profiles can now be edited.

Access checks added to routes. Implemented Edit/Delete for profile items. Updated code in https://github.com/dakala/profile2 (8.x-1.x branch)

Ugh, even more in Structure. Frankly this could also be under People in Configuration. I don't agree with the "anything that is a type" belongs in Structure line of thinking.

Hmm, you may be right. Structure is getting crowded (Contact form categories :-\). You mean to group this with the other account related items instead right?

A legitimate point could be made for either, TBH. The issue of crowding might just swing it in favour of People. Anyways, I've always been more inclined to place this under People myself.

So, I'm changing my mind :) Better to add it to the People section in Configuration then.

> I don't agree with the "anything that is a type" belongs in Structure line of thinking.

I do :)

But I can live with it under Configuration -> People. That's where fields on users are set, so it has a certain logic too.

This is totally a bikeshed I can put off till D9, so we can just get this in ;)

Removing some tags, keeping sun assigned, though no sure if he is active at the moment on this. #1288658: Add bundle support to user entities push some traffic to this issue, where also the Media -people point out the lack of file bundle support.

Configured profile types now displayed in user registration form. https://github.com/dakala/profile2 (8.x-1.x branch) Will move the Profile Types admin settings back to under People if there aren;t any objections.

Could some people start looking at the code, please?

Moved Profile types to Administration > Configuration > People. How does it look now?

When do we expect to merge back the code to Drupal.org? :-)

@xmacinfo: I think that's the maintainers' call and I'm happy for it to take place sooner rather than later :-) That way, it will have more visibility and more people will join in the effort to push this forward at a much faster pace.

> Could some people start looking at the code, please?

Where's the most recent version of the code? Is it the latest patch, or is it github? If the latter, could this be explained in the summary, also explaining whether the github repo is just the profile module, or a complete D8 core.

@joachim: The latest code is on github - https://github.com/dakala/profile2. It's only the profile2 module, working off the 8.x-1.x branch on d.o.

Initial code review
1) module file:
- missed hook_help()
- a lot of commented-out code
- please remove profile2_load() and friends + profile2_type_get_types()
- array('%type' => $type->label() better to wrap into variable
- profile2_user_view() & profile2_form_user_register_form_alter should be replaced with proper ER widget&formatter

I'd suggest to make make module depend on entity_reference module and leverage it's widgets

2) profile2.install is not needed
3) view builder does not have default theme/template
4) access check should be converted to entity access controller/handler
5) list s/controller/builder

Thanks! Please add that to the summary too :)

- profile2.info.yml

The whole module should be called just 'profile'. Profile2 was only called that to avoid clashing with core profile which still existed in D7.

id: main
label: 'Personal information'

D6 grouped profile fields into categories and had no default, though the value it suggested in its help text was 'Personal information'. But I think we should probably keep this the way it was in D7 contrib.

The /contrib folder probably needs to be removed:

- profile2_i18n: translation should be built into the core module
- profile2_og_access: belongs in a contrib module, either OG or a glue module
- profile2_page: not sure if it belongs here or in contrib. Was this a feature in D6 core profile?

function profile2_type_load($id, $reset = FALSE) {

I see that node_load() still exists in D8, though it is marked as deprecated and to be removed in D9. Ditto node_load_multiple() and node_type_load(). Therefore we should kill the 3 corresponding functions in profile. There is no point adding functions that should logically be immediately marked as deprecated.

    $profiles = entity_load_multiple_by_properties('profile2', array(
      'uid' => $account->id(),
      'type' => $config->get('id'),
    ));

    if (isset($profiles)) {

entity_load_multiple_by_properties() is not properly documented (#2295383: EntityStorageInterface::loadByProperties() needs to say what it returns when nothing found), but it appears to return an empty array when nothing is found. So isset() will always evaluate as TRUE!

  $attachedProfileForm = FALSE;
  $weight = 90;
  foreach (\Drupal::configFactory()
             ->listAll('profile2.type.') as $config_name) {
    $config = \Drupal::config($config_name);
    $profileType = entity_load('profile2_type', $config->get('id'));

Procedural code should use snake_case variable names, not camelCase.

/**
 * {@inheritdoc]
 */
function profile2_form_user_register_form_validate(array &$form, array &$form_state) {

I thought @inheritdoc was for child classes and interface implementations?

function profile2_type_get_types() {

Another one that's immediately deprecated.

services:
  profile2.profile_access_checker:

This doesn't seem to be used anywhere?

--------------------------------------------------------------------------------
                              Profile2
--------------------------------------------------------------------------------

Core modules don't have their own README.

I've not gone through all of the /src folder as I'm running out of steam a bit... :(
I do see quite a few bits of commented code in there though, so more to either clean up or upgrade it seems!

        '#profile_type' => $config->get('label'),

That seems like a bad choice of variable name. I'd assume 'profile_type' means the machine name, not the label.

@andypost, @joachim: Thanks for the feedback. Effecting the suggested changes now.

@joachim: The profile2.profile_access_checker service controls access to profile2.account_add_profile, profile2.account_edit_profile and profile2.account_delete_profile routes, doesn't it?

> @joachim: The profile2.profile_access_checker service controls access to profile2.account_add_profile, profile2.account_edit_profile and profile2.account_delete_profile routes, doesn't it?

Ah, probably :) I was looking for the machine name of the service itself, and that doesn't show anywhere in the source. I'm not 100% familiar with how services are used! :)

/**
 * Checks access for displaying configuration translation page.
 */

This doesn't look right!

  /**
   * Implements AccessCheckInterface::applies().
   */
  public function applies(Route $route) {
    return FALSE;
  }

Doesn't apply to any routes at all? Probably needs a comment here to explain!

    $frags = explode('/', $route->getPath());
    if ((!count($frags) > 3) && (!in_array($frags[3], array(

I think we should be using methods of https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Routing!RouteMatc... here.

Also, denying access because the route is wrong looks like something that would go in applies() rather than access(), just going by the interface documentation?

    $fields['label'] = FieldDefinition::create('string')
      ->setLabel(t('Profile description'))
      ->setDescription(t('A brief description of your profile.'))
      ->setRequired(TRUE)
      ->setTranslatable(TRUE)
      ->setSettings(array(
        'default_value' => '',
        'max_length' => 255,
      ))
      ->setDisplayOptions('view', array(
        'label' => 'hidden',
        'type' => 'string',
        'weight' => -5,
      ))
      ->setDisplayConfigurable('view', TRUE)
      ->setDisplayOptions('form', array(
        'type' => 'string',
        'weight' => -5,
      ))
      ->setDisplayConfigurable('form', TRUE);

Profiles have never had labels before.

@joachim: Re implementation of AccessCheckInterface::applies(), the documentation says:

 /**
   * Declares whether the access check applies to a specific route or not.
   *
   * @param \Symfony\Component\Routing\Route $route
   *   The route to consider attaching to.
   *
   * @return array
   *   An array of route requirement keys this access checker applies to.
   */
  public function applies(Route $route);

The way I understand this is, we are checking for access to different routes e.g. /user/5/add/main, /user/2/add/work, /user/1/edit/education so our access check doesn't apply to a specific route. I may be wrong :)

I was going to experiment with that, but I can't get that far -- the site crashes on the user account edit page:

> Error message
Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("type") to generate a URL for route "/user/{user}/add/{type}". in Symfony\Component\Routing\Generator\UrlGenerator->doGenerate() (line 155 of core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php).

Changes made so far - https://github.com/dakala/profile2 (8.x-1.x branch):

1. Changed module name from profile2 to profile
2. Added hook_help (with stubs for more useful help)
3. Removed profile2_load() and other similar functions
4. Removed contrib subdirectory (will integrate some of the code into core profile)
5. Added default theme to view builder class
6. Removed most commented-out code, leaving a couple for porting
7. Refactored a number of variable names with proper case style
8. Removed "label" field from profiles

Can we make the profile entities revisionable?
It would allow Commerce 2.x to use this entity type instead of inventing a commerce_customer_profile again.
(Billing and shipping addresses change, but the order must reference the exact revision used at the time of checkout).

Yes, that would make a lot of sense. @bojanz: For adding/fixing the revision-targeting support in entity reference fields see #2209981: ConfigurableEntityReferenceItem should not mess with the field schema

@bojanz: Sure, that's occurred to me but not implemented yet.

@andypost:

2) profile2.install is not needed

I'm not sure I understand why we no longer need profile2.install (now renamed to profile.install). Could you elaborate please?

Because it defines a schema for "profile" entity that should be generated automatically https://www.drupal.org/node/2259243

@andypost: I see, thanks! I've been working against alpha-12 which still has hook_schema() implementations everywhere. I need to move to HEAD.

Re #159 @tstoeckler: That issue doesn't add/fix anything, it just moves some code around :)

https://github.com/dakala/profile2 (8.x-1.x branch) is now keeping up with D8 HEAD in case people get errors when trying to run the code against the current alpha release.

@bojanz, @tstoeckler: I've added support for revisions, I should say basic support, by looking at how node does it in core. Revisionable profile entities is definitely something we need to implement.

@andypost: Is #1894782: Leverage Entity Reference for new Profile entities? what you had in mind when you suggested making the new Profile module depend on entity_reference and leveraging it's widgets? If yes, wouldn't it be better to revive and discuss it there first?

IMO, we can continue with the work as is, while discussing the pros and cons of the entity_reference solution. There's been much discussion without actual work on this important module.

WRT a listbuilder for profile entities, what do you suggest the columns be? Profile entities don't have labels (title), so we're probably left with Profile type, Owner and Date created

> WRT a listbuilder for profile entities, what do you suggest the columns be? Profile entities don't have labels (title), so we're probably left with Profile type, Owner and Date created

That does raise the point that while profiles don't have a title, entities should probably return some sort of label, because other systems expect it. Even if here it's just "Foo type profile of user 1234".

I've found a few more problems in the code that I'll file on github so this issue can stay on the big picture.

@joachim: Generating a label for profiles will do for now. Out of interest, off the top of your head, can you think of any other core entities without a label/title?

> Generating a label for profiles will do for now

Yup, a generated label is exactly what I meant. Profile should override label() and return something sensible. Then the list controller can just show that.

I've changed the admin section for Profiles to accommodate both Profile and Profile type lists - first attempt.

Thinking we can do with a couple of bulk operations.

@joachim: Re: generated profile labels. I've had to reopen https://github.com/dakala/profile2/issues/8 while I continue working on displaying a profile. As explained in my comment on github, it seems as if EntityViewController expects "label" to be a field as in EntityViewController::view(), there's a check for the label's field definition.

Currently, the list builder works because we're calling our label() function which generates it when listing profiles.

One option is probably to allow entities to have "title" (label) fields just like nodes. What do you think?

> One option is probably to allow entities to have "title" (label) fields just like nodes. What do you think?

That goes against how Profiles work so far. I mean, as a hack we could hide it and auto-generate the title (eg "Main profile for user joachim"), but that's a waste of DB space for something that should be generated on the fly.

    // If the entity's label is rendered using a field formatter, set the
    // rendered title field formatter as the page title instead of the default
    // plain text title. This allows attributes set on the field to propagate
    // correctly (e.g. RDFa, in-place editing).
    if ($_entity instanceof ContentEntityInterface) {
      $label_field = $_entity->getEntityType()->getKey('label');

Are Profiles defined as using ContentEntityInterface?

I can confirm that the Profile class extends ContentEntityBase which implements ContentEntityInterface. I'm not sure now the thought process behind that approach as it's been around for a while now.

The suggested hack is how I'd go about this. I'm not sure we need to hide it on the form as fields such as "created" are usually set when the entity is new.

Yeah, Profiles have never worked with titles or labels so far so we'll have to come up with something that'll allow us to continue this way without the hack.

@joachim: Ignore me:-)

I was looking at the wrong place. I don't need to call EntityViewController::view() first, just like it's being done in NodeViewController::view(). All I need to do in my ProfileViewController::view() is retrieve my view_builder class and set the title with our label() function. It seems to be coming along fine now. Will push changes to github soon.

Bumping it forward…

Just coming across this now. Is the basic idea here to add a UI for allowing users to add fields to the User entity? If not, does anyone know if there's a separate issue for that already?

No, profile entities are different from the user entity. A user can have multiple profile entities, and these can have different bundles.

Moving to the ideas queue.

This issue came up in #yes-no-queue. catch pointed out this need produce manager signoff. I am adding the tag.

The issue summary is still pointing to the Drupal 7 version. The 8+ version lives at https://www.drupal.org/project/profile and has 30,000 users. For me 30,000 users shows that there are plenty of sites that still need this, but I also think they're being served well enough by the contrib module and I'm not sure it's something we need in core - especially in the project browser + recipes world where it will hopefully be more discoverable.

Going through the end of the drupal issue queue came across #1894770: Add views integration for profile2 module which is postponed on this.

I also agree with catch after reading up on recent history of the issue. It is better in contrib unless there is some major benefit of avoiding duplication in contrib (multiple profile modules competing and there is a need to standardise) or most Drupal sites using it.