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

Battle plan

  1. DONE: Minimal port to D8. (non-functional)
  2. DONE: Make the current code work with D8's entity system.
  3. DONE: Comment out or work around advanced (contrib) Entity API module functionality (e.g., metadata, etc)
  4. DONE: Investigate what code can be removed.
  5. DONE: Convert profile types into configuration.
  6. DONE: Implement EntityFormController for Profile and ProfileType.
  7. DONE: Implement EntityRenderController for Profile.
  8. DONE: Implement EntityListController for ProfileType.
  9. DONE: Rework user profile category tabs (@sun). Remove necessity for rebuilding the menu/router.
  10. DONE: Remove unnecessary helper functions.
  11. DONE: Implement entity access controller
  12. Fix and improve pre-existing Profile2 logic/behavior:
    1. DONE: Move profile2_load_by_user() as custom method into ProfileStorageController. Eliminates the drupal_static().
    2. DONE: Missing UUID for profiles.
    3. DONE: Ensure to leverage entity_delete_multiple()'s bulk deletion process wherever multiple profiles are deleted.
    4. DONE: Remove remaining entity API wrapper functions
    5. DONE: Migrate default "Main" profile from profile2_install() into config for now.
    6. Update Profile to EntityNG + Property/Field API. (not ProfileType)
    7. Figure out what to do with the custom profile access system.
  13. Vastly improve test coverage.
    • Profile (UI) delete confirmation.
    • Profile access.
  14. DONE: Clean up unnecessary and stray contrib Entity API code.
  15. 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.)
  16. Rename the whole thing from profile2 into profile. :)
  17. Move default "Main profile" into the standard installation profile.

http://drupalcode.org/project/profile2.git/shortlog/refs/heads/8.x-1.x

Needs architectural review

  1. 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.).
  2. 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.
  3. Should we use extra fields for injecting the profile form / displaying the profile on the user form / view?
CommentFileSizeAuthor
#172 profile2.patch8.09 KBsun
#171 profile2.patch6.42 KBsun
#170 profile2.patch2.04 KBsun
#168 profile2.patch1.29 KBsun
#165 profile2.patch962 bytessun
#163 profile2-8.x-1.x.patch12.21 KBsun
#161 profile2-8.x-1.x.patch11.92 KBsun
#159 profile2-8.x-1.x.patch9.95 KBsun
#157 profile2.head_.157.patch7.26 KBsun
#155 profile2.head_.155.patch4.87 KBsun
#153 profile2.head_.153.patch3.62 KBsun
#151 profile2.head_.151.patch1.72 KBsun
#146 profile2.access-test.146.patch10.68 KBsun
#144 profile2.form-alter.143.patch10.08 KBsun
#141 profile2.form-alter.141.patch4.87 KBsun
#139 profile2.cleanup.139.patch31.05 KBsun
#139 interdiff.txt665 bytessun
#137 profile2.cleanup.137.patch30.83 KBsun
#137 interdiff.txt1.34 KBsun
#135 profile2.cleanup.134.patch30.63 KBsun
#135 interdiff.txt6.97 KBsun
#131 profile2-fix-field-settings-form.txt874 byteststoeckler
#127 profile2.cleanup.127.patch27.25 KBsun
#126 list-controller.patch700 bytesfubhy
#125 template.125.patch6.21 KBsun
#124 profile2_load_by_user.patch7.96 KBfubhy
#122 profile2_load_by_user.patch8.56 KBfubhy
#120 profile2_load_by_user.patch7.94 KBfubhy
#119 profile2_load_by_user.patch8.82 KBfubhy
#118 install-tests.118.patch8.39 KBsun
#116 profile2.cleanup.116.patch21.89 KBsun
#115 cleanup.patch22.68 KBfubhy
#114 entity_delete_multiple.patch1.38 KBfubhy
#113 entity_delete_multiple.patch1.46 KBfubhy
#112 access-controller.patch9.87 KBfubhy
#107 access-controller.patch9.76 KBfubhy
#106 entity_delete_multiple.patch1.43 KBfubhy
#104 entity_delete_multiple.patch1.39 KBfubhy
#97 config-default.97.patch1.41 KBsun
#95 profile2.people.95.patch10.65 KBsun
#94 entity-info-plugin.do-not-test.patch10.18 KBsun
#93 1726822-93.patch13.83 KBfubhy
#90 1726822-90.patch10.39 KBfubhy
#88 profile2.uuid_.88.patch1.5 KBsun
#87 profile2.delete-confirm.do-not-test.patch3.94 KBsun
#85 1726822-85.patch3.33 KBfubhy
#84 1726822-84.patch3.59 KBfubhy
#79 profile2.profile-cleanup.79.patch5.91 KBsun
#79 interdiff.txt2.52 KBsun
#77 1726822-77.patch2.31 KBfubhy
#76 1726822-76.patch2.23 KBfubhy
#75 1726822-75.patch8.17 KBfubhy
#73 1726822-73.patch11.53 KBfubhy
#72 1726822-72.patch14.08 KBfubhy
#71 1726822-71.patch15.58 KBfubhy
#69 1726822-68.patch16.34 KBfubhy
#68 profile2.static-remove.do-not-test.patch6.3 KBsun
#68 interdiff.txt5.52 KBsun
#67 1726822-67.patch16.42 KBfubhy
#65 profile2.crud-test.patch4.71 KBsun
#64 1726822-63.patch5.4 KBfubhy
#62 1726822-62.patch8.27 KBfubhy
#59 1726822-59.patch6.53 KBfubhy
#57 1726822-57.patch6.45 KBfubhy
#53 profile2.type-storage.52.patch7.32 KBsun
#50 1726822-49.patch3.02 KBfubhy
#48 1726822-47.patch3.03 KBfubhy
#44 list-controller.43.patch4.02 KBn3or
#44 interdiff.txt3.18 KBn3or
#42 list-controller.42.patch2.65 KBn3or
#40 profile2.router.40.patch7.23 KBsun
#37 render-controller.37.patch2.7 KBn3or
#36 render-controller.36.patch2.64 KBn3or
#34 form-controllers-34.patch14.58 KBn3or
#34 interdiff.txt4.6 KBn3or
#32 form-controllers-32.patch14.99 KBn3or
#32 interdiff.txt3.02 KBn3or
#28 form-controllers-28.patch12.25 KBfubhy
#27 form-controllers.patch12.29 KBfubhy
#21 profile2_drupal7_usercategories.png18.29 KBn3or
#20 drupal8.profile2-port.20.patch7.45 KBn3or
#18 drupal8.profile2-port.18.patch7.01 KBn3or
#18 interdiff.txt2.52 KBn3or
#16 drupal8.profile2-port.16.patch6.86 KBn3or
#13 profile2.patch29.22 KBsun
#13 interdiff.txt18.52 KBsun
#12 drupal8.profile2-port.12.do-not-test.patch27.9 KBn3or
#12 interdiff.txt0 bytesn3or
#11 drupal8.profile2-port.11.do-not-test.patch28.7 KBn3or
#11 interdiff.txt31.32 KBn3or
#8 drupal8.profile2-port.8.patch21.59 KBn3or
#8 interdiff.txt5.64 KBn3or
#7 drupal8.profile2-port.7.patch16.88 KBn3or
#7 interdiff.txt20.48 KBn3or
#5 profile2.3.patch11.24 KBsun
#5 interdiff.txt3 KBsun
#4 profile2.3.patch14.17 KBsun
#3 drupal8.profile2-port.3.patch23.27 KBn3or
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
sun’s picture

Title: Port to D8 » Port Profile2 to D8
Category: feature » task
Issue tags: +Platform Initiative
sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

n3or’s picture

Status: Active » Needs review
FileSize
23.27 KB

My 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.

sun’s picture

FileSize
14.17 KB

oh. 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.

sun’s picture

FileSize
3 KB
11.24 KB

Sorry, one more follow-up - committed the interdiff changes.

Status: Needs review » Needs work

The last submitted patch, profile2.3.patch, failed testing.

n3or’s picture

Status: Needs work » Needs review
FileSize
20.48 KB
16.88 KB

Got 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)

n3or’s picture

- 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:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'revision.timestamp' in 'field list': SELECT base.pid AS pid, base.type AS type, base.uid AS uid, base.label AS label, base.created AS created, base.changed AS changed, revision.timestamp AS revision_timestamp, revision.uid AS revision_uid
FROM
{profile} base
WHERE (base.pid IN (:db_condition_placeholder_0, :db_condition_placeholder_1, :db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7, :db_condition_placeholder_8, :db_condition_placeholder_9, :db_condition_placeholder_10, :db_condition_placeholder_11, :db_condition_placeholder_12, :db_condition_placeholder_13, :db_condition_placeholder_14, :db_condition_placeholder_15)) ; Array
(
[:db_condition_placeholder_0] => 34
[:db_condition_placeholder_1] => 35
[:db_condition_placeholder_2] => 36
[:db_condition_placeholder_3] => 37
[:db_condition_placeholder_4] => 38
[:db_condition_placeholder_5] => 39
[:db_condition_placeholder_6] => 40
[:db_condition_placeholder_7] => 41
[:db_condition_placeholder_8] => 42
[:db_condition_placeholder_9] => 43
[:db_condition_placeholder_10] => 44
[:db_condition_placeholder_11] => 45
[:db_condition_placeholder_12] => 46
[:db_condition_placeholder_13] => 47
[:db_condition_placeholder_14] => 48
[:db_condition_placeholder_15] => 49
)
in Drupal\entity\DatabaseStorageController->load() (line 171 of /Applications/MAMP/htdocs/drupal8_profile2/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php).

I don't get where the revision.timestamp and revision.uid fields were added.

sun’s picture

Important: #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. :)

sun’s picture

Status: Needs review » Needs work

Discussed 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():

  public function __construct(array $values = array(), $entity_type) {

- Use/implement EntityFormController for Profile entity type: #1499596: Introduce a basic entity form controller

+++ b/profile2.module
@@ -67,14 +65,13 @@ function profile2_entity_info() {
   $return['profile2_type'] = array(
...
+    'controller class' => 'Drupal\entity\DatabaseStorageController',

Drupal\config\ConfigStorageController

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+ * Implements hook_menu.

Missing ()

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+  $items = array();

Can be removed.

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+  $items['admin/structure/profiles/list'] = array(
+    'title' => 'Profile types',

Title should be 'List'.

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+  $items['admin/structure/profiles/import'] = array(
...
+  $items['admin/structure/profiles/manage/%profile2_type/clone'] = array(
...
+  $items['admin/structure/profiles/manage/%profile2_type/export'] = array(

Remove entirely.

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+  $items['admin/structure/profiles/manage/%profile2_type/fields'] = array(
+    'title' => 'Manage profile fields',

Let's compare with taxonomy_menu() + taxonomy_entity_info(), which is the only comparable implementation in Drupal core.

+++ b/profile2.module
@@ -82,17 +79,172 @@ function profile2_entity_info() {
+function profile2_type_list_page() {

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.

+++ b/profile2.module
@@ -165,7 +323,7 @@ function profile2_get_types($type_name = NULL) {
 function profile2_load($pid, $reset = FALSE) {
-  $profiles = profile2_load_multiple(array($pid), array(), $reset);
+  $profiles = profile2_load_multiple($pid, array(), $reset);
   return reset($profiles);

@@ -186,7 +344,13 @@ function profile2_load($pid, $reset = FALSE) {
 function profile2_load_multiple($pids = array(), $conditions = array(), $reset = FALSE) {
-  return entity_load('profile2', $pids, $conditions, $reset);
+  if (empty($conditions)) {
+    // it doesn't make sense to use pids and conditions,
+    // therefore ignore pids if conditions were set
+    return entity_load_multiple_by_properties('profile2', $conditions);
+  }
+
+  return entity_load_multiple('profile2', $pids, $reset);
 }

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.

+++ b/profile2.module
@@ -361,7 +517,9 @@ function profile2_form_user_profile_form_alter(&$form, &$form_state) {
-    if (!empty($profile_type->data['registration'])) {
+    $data = $profile_type->get('data');
+    $data = unserialize($data);
+    if (!empty($data['registration'])) {

Let's turn this into a ProfileType::$registration property.

n3or’s picture

Status: Needs work » Needs review
FileSize
31.32 KB
28.7 KB

Tried 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.

n3or’s picture

Fixed incompatibility to current 8.x-1.x. Interdiff is meaningless.

sun’s picture

FileSize
18.52 KB
29.22 KB

Slightly revised. Any chance to test this before commit? ;)

n3or’s picture

Did a few functional tests with positive result! Looks great. Thank you sun!

sun’s picture

Status: Needs review » Active

Committed 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()

n3or’s picture

Status: Active » Needs review
FileSize
6.86 KB

Almost 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.

sun’s picture

Status: Needs review » Needs work
+++ b/lib/Drupal/profile2/Tests/Profile2CRUDTestCase.php
@@ -103,11 +106,10 @@ class Profile2CRUDTestCase extends WebTestBase {
-    $this->assertTrue($new_user->status, t('New account is active after registration.'));
...
+    $this->assertText(t('In the meantime, a welcome message with further instructions has been sent to your e-mail address.'), t('User registered successfully.'));
+    $new_user = user_load_by_name($name);
+    $this->assertFalse((bool) $new_user->status, t('New account is active after registration.'));

assertTrue vs. assertFalse -- massive change in expectations ;)

+++ b/profile2.module
@@ -404,7 +404,7 @@ function profile2_type_delete(ProfileType $type) {
-  $pids = array_keys(profile2_load_multiple_by_properties(array('type' => $type->type)));
+  $pids = array_keys(profile2_load_multiple_by_properties(array('type' => $type->get('id'))));

Let's always use the ->id() method to retrieve the (configurable) entity ID.

n3or’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
7.01 KB

See comment of sun. Additionally added two empty lines killed in patch #16.

sun’s picture

Status: Needs review » Active

YAY! The testbot runs and passes against D8! :)

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

n3or’s picture

Status: Active » Needs review
FileSize
7.45 KB

Removed 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.)

n3or’s picture

Forgot screenshot.. :)

Status: Needs review » Needs work

The last submitted patch, drupal8.profile2-port.20.patch, failed testing.

sun’s picture

Status: Needs work » Active

Committed and pushed to 8.x-1.x, with a couple of additional adjustments. :)

Nice to see that this actually works already (tested manually)! :)

sun’s picture

Next 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)

moshe weitzman’s picture

Nice 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.

fubhy’s picture

Assigned: sun » fubhy

Sun 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.).

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

FileSize
12.29 KB

Talked 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 :).

fubhy’s picture

Status: Active » Needs review
FileSize
12.25 KB

Removing that ugly strtr(). Dunno why I even added that. :P

Status: Needs review » Needs work

The last submitted patch, form-controllers-28.patch, failed testing.

moshe weitzman’s picture

Could 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.

n3or’s picture

Assigned: fubhy » n3or

I'm back. Thank you very much fubhy!

I will try to get this patch green.

n3or’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
14.99 KB

Concentrated on fixing failing tests!

Locally now all tests passed. Adapted the test case and added the save handler within profile form controller.

sun’s picture

+++ b/lib/Drupal/profile2/Profile.php
@@ -165,7 +165,6 @@ class Profile extends Entity {
     return $content;

The next step here, after completing the form controller conversion should be to do the render controller conversion.

+++ b/lib/Drupal/profile2/ProfileFormController.php
@@ -0,0 +1,43 @@
+    if (!user_access('administer profiles')) {
+      unset($element['delete']);
+    }

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.

+++ b/lib/Drupal/profile2/ProfileFormController.php
@@ -0,0 +1,43 @@
+    drupal_set_message(t('Your profile has been saved.'));

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.

+++ b/lib/Drupal/profile2/ProfileFormController.php
@@ -0,0 +1,43 @@
+    // Rebuild the menu tree.
+    menu_router_rebuild();

For regular profiles, this shouldn't be needed.

+++ b/lib/Drupal/profile2/ProfileTypeFormController.php
@@ -0,0 +1,76 @@
+        'source' => array('label'),

No longer needed, HEAD defaults to 'label' now.

+++ b/lib/Drupal/profile2/ProfileTypeFormController.php
@@ -0,0 +1,76 @@
+    $profile_type->save();
...
+    drupal_set_message(t('%label configuration has been saved.', array('%label' => $profile_type->label())));

::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.

+++ b/lib/Drupal/profile2/ProfileTypeFormController.php
@@ -0,0 +1,76 @@
+    // Rebuild the menu tree.
+    menu_router_rebuild();
...
+    // Rebuild the menu tree.
+    menu_router_rebuild();

Let's add a @todo right above these to remove them. (That will be my job.)

+++ b/lib/Drupal/profile2/Tests/Profile2CRUDTestCase.php
@@ -121,7 +121,7 @@ class Profile2CRUDTestCase extends WebTestBase {
-    $this->drupalGet('user/' . $user1->uid . '/edit/main');
+    $this->drupalGet('user/' . $user1->uid . '/edit/profile/main');

+++ b/profile2.module
@@ -133,14 +137,25 @@ function profile2_menu() {
+    $items["user/%user/edit/profile/$id"] = array(

I don't see why the additional /profile/ part in the path is necessary. Let's revert this.

+++ b/profile2.module
@@ -106,18 +112,16 @@ function profile2_menu() {
   $items['admin/structure/profiles/manage/%profile2_type'] = array(
     'title' => 'Edit profile type',
...
+    'page callback' => 'entity_get_form',
+    'page arguments' => array(4),

We need a page callback here to set an appropriate page title with drupal_set_title().

+++ b/profile2.module
@@ -440,6 +455,8 @@ function profile2_user_view($account, $view_mode, $langcode) {
+ * @todo Let's try to find a better solution for this and everything related.

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.

n3or’s picture

FileSize
4.6 KB
14.58 KB

Fixed everything mentioned in sun's comment. (Except the first two suggestions, they aren't related to this patch.)

n3or’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Active

Thanks! 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.)

sun’s picture

Issue summary: View changes

Updated issue summary.

n3or’s picture

Status: Active » Needs review
FileSize
2.64 KB

First version of EntityRenderController implementation.

n3or’s picture

Removed profile2_view.

n3or’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Active

Yay, thanks! Committed and pushed.

Next step: EntityListController for ProfileType config entity.

sun’s picture

Title: Port Profile2 to D8 » Port Profile2 to D8 and prepare for Drupal core inclusion

Clarifying issue title.

sun’s picture

Status: Active » Needs review
FileSize
7.23 KB

Attached 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.

sun’s picture

Status: Needs review » Active

Committed 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)! :)

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

n3or’s picture

FileSize
2.65 KB

First version of EntityListController.

n3or’s picture

Status: Active » Needs review
n3or’s picture

FileSize
3.18 KB
4.02 KB

- Removed unused code
- Made field_ui operations conditional

sun’s picture

Status: Needs review » Active

Thanks! 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.

sun’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Status: Active » Needs work
+++ b/lib/Drupal/profile2/ProfileTypeListController.phpundefined
@@ -0,0 +1,42 @@
+        'href' => $uri['path'] . '/fields',

should use _field_ui_bundle_admin_path() instead of $entity->uri()

+++ b/lib/Drupal/profile2/ProfileTypeListController.phpundefined
@@ -0,0 +1,42 @@
+        'href' => $uri['path'] . '/display',

same here.

Furthermore we need to rebuild the entity info after adding a new profile type. Otherwise _field_ui_bundle_admin_path() fails.

sun’s picture

Status: Needs work » Active

_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()

fubhy’s picture

Status: Active » Needs review
FileSize
3.03 KB

Also, 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.

fubhy’s picture

_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.

You are right. I am not really sure WHY it is a private function though. It's simply a helper/wrapper for convenience.

fubhy’s picture

FileSize
3.02 KB

Whoops, 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.

moshe weitzman’s picture

Nice 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.

webchick’s picture

One 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. :)

sun’s picture

Thanks - 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. :))

sun’s picture

Btw, 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

fubhy’s picture

+++ b/lib/Drupal/profile2/ProfileTypeStorageController.phpundefined
@@ -0,0 +1,47 @@
+    if (module_exists('field')) {

Common... What do we do with profiles without fields? :D

Fields should be a dependency for profiles. no?

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.

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

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.

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())

sun’s picture

Status: Needs review » Active

You're right. Quickly reverted and replaced that with a proper field module dependency in the .info file prior to commit.

fubhy’s picture

Status: Active » Needs review
FileSize
6.45 KB
  • Adding a storage controller for profiles.
  • Removing profile2_profile2 (self implemented) hooks in favor or method overrides on the storage controllers
  • Also cleaning up entity info a bit.
  • Moving static cache clears over to the controllers as well changing that slightly

Next up: Further clean up for stray contrib Entity API code and cleanup of the Profile2 class (uri, url, path, etc.)

fubhy’s picture

Let's also switch to EntityNG and NG controllers as soon as possible.

fubhy’s picture

FileSize
6.53 KB

Missing hook_user_cancel()?

I 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.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Move profile2_load_by_user() as custom method into ProfileStorageController. Eliminates the drupal_static().

Seems related to the latest patch. Adding that now.

sun’s picture

Status: Needs review » Active

Thanks! Committed with minimal adjustments. (Coding standards say that there should be a blank line before the closing brace of a class.)

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Status: Active » Needs review
FileSize
8.27 KB

(Coding standards say that there should be a blank line before the closing brace of a class.)

Interesting. 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.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Assigned: n3or » Unassigned
+++ b/lib/Drupal/profile2/ProfileTypeFormController.php
@@ -66,4 +66,5 @@ class ProfileTypeFormController extends EntityFormController {
+  ¶

You 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.

fubhy’s picture

Assigned: Unassigned » n3or
FileSize
5.4 KB

Also removing the other entity_load wrappers.

sun’s picture

Assigned: n3or » Unassigned
FileSize
4.71 KB

Added Profile CRUD test coverage. Will commit this in a second and send the latest patch for re-testing.

fubhy’s picture

I am currently working on finishing the form controllers and allowing deletions by users.

fubhy’s picture

FileSize
16.42 KB

Okay 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.

sun’s picture

Reverted 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.)

fubhy’s picture

FileSize
16.34 KB

Wow, what's up with my IDE now? Weird :)

Status: Needs review » Needs work

The last submitted patch, 1726822-68.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
15.58 KB

Re-roll

fubhy’s picture

FileSize
14.08 KB

Accidentally overwrote your version of profile2_load_by_user(). Sorry, doing to many things at once.

fubhy’s picture

FileSize
11.53 KB

So sorry for the noise. :/

This patch is now actually the right one. Sorry!

sun’s picture

+++ b/lib/Drupal/profile2/Profile.php
@@ -29,13 +29,6 @@ class Profile extends Entity {
-  public $label;

@@ -56,92 +49,45 @@ class Profile extends Entity {
+  public function label($langcode = NULL) {
+    return $this->type()->label();
   }

I 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?

+++ b/lib/Drupal/profile2/Profile.php
@@ -56,92 +49,45 @@ class Profile extends Entity {
   public function user() {
...
-  public function setUser($account) {

You removed setUser() already, and I wonder whether we really need the [get]user() method... do we?

+++ b/lib/Drupal/profile2/Profile.php
@@ -56,92 +49,45 @@ class Profile extends Entity {
   public function type() {

btw, type() is a really confusing name. Can we rename this custom method to getProfileType()?

+++ b/lib/Drupal/profile2/ProfileFormController.php
@@ -20,10 +20,6 @@ class ProfileFormController extends EntityFormController {
-    if (!user_access('administer profiles')) {
-      unset($element['delete']);
-    }

@@ -40,6 +36,17 @@ class ProfileFormController extends EntityFormController {
+    $form_state['redirect'] = array($uri['path'], $uri['options']);
...
+  public function delete(array $form, array &$form_state) {

+++ b/profile2.pages.inc
@@ -0,0 +1,28 @@
+function profile2_delete_confirm_form($form, $form_state, Profile $profile) {

Let's back out those redirect/delete changes for now and focus on the Profile class clean-ups first.

+++ b/profile2.module
@@ -80,22 +80,15 @@ function profile2_entity_info() {
- * @param Drupal\profile2\Profile $profile
...
 function profile2_profile_uri(Profile $profile) {
...
- * @param Drupal\profile2\ProfileType $profile_type
...
 function profile2_profile_type_uri(ProfileType $profile_type) {

I'm not sure why you removed the phpDoc, but they should obviously stay.

+++ b/profile2.module
@@ -80,22 +80,15 @@ function profile2_entity_info() {
+  $uri = user_uri(entity_load('user', $profile->uid));

This need to call into EntityInterface::uri().

+++ b/profile2.module
@@ -278,7 +275,7 @@ function profile2_permission() {
-  foreach (profile2_get_types() as $type) {
+  foreach (entity_load_multiple('profile2_type') as $type) {

@@ -299,16 +296,6 @@ function profile2_permission() {
-function profile2_get_types() {

Let's back out this removal for now, and potentially merge it into the similar changes that were in #64.

fubhy’s picture

FileSize
8.17 KB

I 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?

I 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.

Let's back out this removal for now, and potentially merge it into the similar changes that were in #64.

#64 was a fail :). I will create a separate patch for just that if you want.

I'm not sure why you removed the phpDoc, but they should obviously stay.

Yeah I wasn't sure what to write there. None of the other core entity uri callbacks have anything in their docblocks.

Let's back out those redirect/delete changes for now and focus on the Profile class clean-ups first.

Okay, separate patch too then.

Attached patch should be the plain cleanup.

fubhy’s picture

FileSize
2.23 KB

This is the plain profile2_get_types() removal on top of the previous patch in #75.

fubhy’s picture

FileSize
2.31 KB

And this is the plain delete form patch on top of the patch in #76.

Status: Needs review » Needs work

The last submitted patch, 1726822-77.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
5.91 KB

I 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.

sun’s picture

#76: 1726822-76.patch queued for re-testing.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Active

Committed #79 and #76.

Updated the issue summary with a list of architectural discussion topics. @fago / @joachim, any feedback on those?

fubhy’s picture

Any 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.

sun’s picture

Remarks on #77:

+++ b/profile2.module
@@ -161,6 +161,16 @@ function profile2_menu() {
+    'load arguments' => array('%map', 'edit'),
...
+    'access callback' => 'profile2_profile_edit_access',

s/edit/delete/g

+++ b/profile2.pages.inc
@@ -0,0 +1,28 @@
+function profile2_delete_confirm_form($form, $form_state, Profile $profile) {
+  $form_state['profile'] = $profile;

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.:

$form['pid'] = array('#type'...'#value'...

That is, as long as the delete confirmation form is not handled by the entity form controller (which is not supported by core yet).

+++ b/profile2.pages.inc
@@ -0,0 +1,28 @@
+  $confirm_question = t('Are you sure you want to delete profile %label?', array('%label' => $profile->label()));
...
+  drupal_set_message(t('Deleted %label.', array('%label' => $form_state['profile']->label())));

I don't think the confirmation question nor the confirmation message will work out with real values. Needs manual testing and wordsmithing.

+++ b/profile2.pages.inc
@@ -0,0 +1,28 @@
+  $uri = entity_load('user', $form_state['profile']->uid)->uri();
...
+  $form_state['redirect'] = $uri['path'];

Let's group these lines and also add a comment to explain what is being done and why.

fubhy’s picture

Status: Active » Needs review
FileSize
3.59 KB

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.:

$form['pid'] = array('#type'...'#value'...
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 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

fubhy’s picture

FileSize
3.33 KB

Oh, that was not the full patch

fubhy’s picture

Would it make sense to replace Profile::$uid with ::$attachedEntityType + ::$attachedEntityID?

In 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.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Active
FileSize
3.94 KB

Committed #85, plus the attached follow-up patch for it.

Btw, there are no tests for the profile delete functionality.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Active » Needs review
FileSize
1.5 KB

Attached patch adds UUIDs to Profile.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs review » Active

Added a single test assertion for the uuid property and committed #88.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Status: Active » Needs review
FileSize
10.39 KB

This patch moves the Overview to /admin/people/profiles ... I believe it makes more sense to have it there.

Status: Needs review » Needs work

The last submitted patch, 1726822-90.patch, failed testing.

fubhy’s picture

Will do the Entity plugin conversion now.

fubhy’s picture

Status: Needs work » Needs review
FileSize
13.83 KB
sun’s picture

Status: Needs review » Active
FileSize
10.18 KB

Attaching a diff with lesser minimum similarity index, just to confirm that #93 just moves files around and updates phpDoc.

Committed and pushed #93.

sun’s picture

Status: Active » Needs review
FileSize
10.65 KB

Same as #90, but re-rolled, and only minimal adjustments.

sun’s picture

Status: Needs review » Active

Committed and pushed #95.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Active » Needs review
FileSize
1.41 KB

Converted default configuration into configuration.

sun’s picture

#97: config-default.97.patch queued for re-testing.

nevergone’s picture

Sorry, but one question: With what better the Profile2, than the user entity bundles and fields?

fubhy’s picture

+++ b/config/profile2.type.main.ymlundefined
@@ -0,0 +1,6 @@
+# @todo Are profile types the responsibility of installation profiles?

Yes, let's simply delete this part. It's a installation profile thing.

fubhy’s picture

Status: Needs review » Active

But 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.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

@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?

sun’s picture

Absolutely. 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 :)

fubhy’s picture

Status: Active » Needs review
FileSize
1.39 KB

All right... Let's fix the remaining todo's then. I am also adding a few todos:

  • Find better solution instead of hook_user_view() and hook_form_alter() implementations for injecting the profiles into the view/form display. (extra fields?)
  • We should split up single/multiple functions (e.g. profile2_load_by_user())
  • We should split up single/multiple functions (e.g. profile2_load_by_user())
  • Remove entity API wrapper functions
  • Implement entity access controller
  • 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.

Attached patch improves multiple entity deletion.

fubhy’s picture

Issue summary: View changes

Updated issue summary. Users can delete their own profiles (flush/empty them).

Status: Needs review » Needs work

The last submitted patch, entity_delete_multiple.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

Right.. 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()...

fubhy’s picture

FileSize
9.76 KB

Adding 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

Status: Needs review » Needs work
Issue tags: -Platform Initiative, -Profile in core

The last submitted patch, access-controller.patch, failed testing.

The last submitted patch, access-controller.patch, failed testing.

sun’s picture

Great!

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

Can't we simply remove the User type-hints for now to get around the drupal_anonymous_user() problem?

+++ b/lib/Drupal/profile2/ProfileAccessController.php
@@ -0,0 +1,127 @@
+    if (!isset($account)) {
+      $account = $GLOBALS['user'];
+    }

I think we need a user_load($GLOBALS['user']->uid) here - the global $user object is not a real user entity.

+++ b/lib/Drupal/profile2/ProfileAccessController.php
@@ -0,0 +1,127 @@
+    $cid = $account->uid . ':' . $operation . ':' . $profile->id();
+    if (isset($this->accessCache[$cid])) {
+      return $this->accessCache[$cid];
+    }

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?

+++ b/profile2.module
@@ -503,57 +489,29 @@ function profile2_form_submit_cleanup(&$form, &$form_state) {
+function profile2_access($op, Profile $profile, User $account = NULL) {
+  return $profile->access($op, $account);
 }

Isn't this obsolete?

+++ b/profile2.module
@@ -503,57 +489,29 @@ function profile2_form_submit_cleanup(&$form, &$form_state) {
 /**
  * Implements hook_profile2_access().
  */
...
+function profile2_profile2_access($op, Profile $profile, User $account) {
...
+  if (user_access("$op any $profile->type profile", $account)) {
+    return TRUE;
+  }
+
+  if (isset($profile->uid) && $profile->uid == $account->uid && user_access("$op own $profile->type profile", $account)) {
+    return TRUE;
   }

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?

fubhy’s picture

Isn't this obsolete?

Sadly 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.

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?

I agree, but let's keep it until we have it!

I think we need a user_load($GLOBALS['user']->uid) here - the global $user object is not a real user entity.

Yes, good idea. It's sadly the same problem as the one with anonymous users.

Can't we simply remove the User type-hints for now to get around the drupal_anonymous_user() problem?

Sadly no, they are in the interface :/.

We gotta fix that here: #1825332: Introduce an AccountInterface to represent the current user

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?

Sure, why not :)

fubhy’s picture

Status: Needs work » Needs review
FileSize
9.87 KB

This should pass. Again, good idea with the user_load().

fubhy’s picture

Ultra minor tuning for the entity_delete_multiple patch.

fubhy’s picture

Okay, or we just go with the array keys as pointed out by @sun :)

fubhy’s picture

FileSize
22.68 KB

Removing stray functions, files and properties and cleaning up documentation (also in api.php).

sun’s picture

FileSize
21.89 KB

Reverted removal of ProfileType::$uuid and $weight.

sun’s picture

Alrighty. 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.

sun’s picture

FileSize
8.39 KB

Attached patch eliminates the default "main" profile and adjusts tests.

fubhy’s picture

FileSize
8.82 KB

Deprecating 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.

fubhy’s picture

FileSize
7.94 KB

Reverting the $type_id/$type_name/$type etc. changes. Follow-up cleanup material.

Status: Needs review » Needs work

The last submitted patch, profile2_load_by_user.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
8.56 KB

Status: Needs review » Needs work

The last submitted patch, profile2_load_by_user.patch, failed testing.

fubhy’s picture

FileSize
7.96 KB

Sorry for the noise!

sun’s picture

Status: Needs work » Needs review
FileSize
6.21 KB

Attached patch makes the template work (was unused) and updates it for HTML5 and stuff.

fubhy’s picture

FileSize
700 bytes

We should extend ConfigEntityListController and not EntityListController for ProfileType

sun’s picture

FileSize
27.25 KB

Attached 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).

Status: Needs review » Needs work

The last submitted patch, profile2.cleanup.127.patch, failed testing.

tstoeckler’s picture

tl: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.

tstoeckler’s picture

So 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.

  1. The "Make field private" checkbox on the field instance settings form does not actually get saved. I've attached a little patch to fix that.
  2. The "user/%" page has "Error" as the page title. I did not debug this deeply, but user_page_title() does get called and it does return the username correctly. I have no idea what is going wrong there, and how it is related to this, but simply disabling profile2.module again made the username re-appear as page title.
  3. The "user/%/edit/%profile_type" page yields a fatal error in profile2_menu_load(). Something strange is going on there, where instead of the user object only the user ID is being passed into $account even though it is type-hinted correctly with "User", yet that is not the error, but the subsequent $account->id() call.

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.

  1. On the profile type add screen I think a "Save and manage fields" button would be incredibly useful.
  2. Weights for profile types. On the profile type list we could have tabledrag, and the weights would correspond to the weight of the local tasks. I'm pretty sure this is non-trivial to implement and would best be done in a follow-up but I wanted to mention it anyway to see if that is actually something you guys would consider useful.
tstoeckler’s picture

Here'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.

fubhy’s picture

Weights for profile types. On the profile type list we could have tabledrag, and the weights would correspond to the weight of the local tasks. I'm pretty sure this is non-trivial to implement and would best be done in a follow-up but I wanted to mention it anyway to see if that is actually something you guys would consider useful.

Yeah we talked about that already and this is what we started for that: #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

sun’s picture

Working 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

tstoeckler’s picture

Re #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.

sun’s picture

Status: Needs work » Needs review
FileSize
6.97 KB
30.63 KB

Fixed 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.

Status: Needs review » Needs work

The last submitted patch, profile2.cleanup.134.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
30.83 KB

Alright, now including temporary stop-gap fix.

fubhy’s picture

The 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?

sun’s picture

FileSize
665 bytes
31.05 KB

Adjusted access controller to conditionally use $profile->id() for existing profiles and $profile->bundle() for non-existing/new profiles.

fubhy’s picture

Nice! RTBC when this comes back green!

sun’s picture

Thanks - 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.

Status: Needs review » Needs work

The last submitted patch, profile2.form-alter.141.patch, failed testing.

fubhy’s picture

The proposed changes to the form structure look nice. I can tell that you distaste constructed array keys. :)

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?

sun’s picture

Status: Needs work » Needs review
FileSize
10.08 KB

Fixed 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.

sun’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Maybe it's time for another patch over at the core issue.

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.68 KB

Thanks - 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.

Status: Needs review » Needs work
Issue tags: -Platform Initiative, -Profile in core

The last submitted patch, profile2.access-test.146.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Platform Initiative, +Profile in core

#146: profile2.access-test.146.patch queued for re-testing.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

I guess I am guilty as charged for that fatal :/. The patch looks good again. Let's roll another core patch.

sun’s picture

Status: Reviewed & tested by the community » Active

Alrighty. :)

sun’s picture

Status: Active » Needs review
FileSize
1.72 KB

Chasing HEAD.

Status: Needs review » Needs work

The last submitted patch, profile2.head_.151.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

Status: Needs review » Needs work

The last submitted patch, profile2.head_.153.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.87 KB

Jesus.

Status: Needs review » Needs work

The last submitted patch, profile2.head_.155.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.26 KB
sun’s picture

Status: Needs review » Active

Finally. Committed.

sun’s picture

Status: Active » Needs review
FileSize
9.95 KB

Final 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().

Status: Needs review » Needs work

The last submitted patch, profile2-8.x-1.x.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.92 KB

@fubhy figured fit fout. :)

Status: Needs review » Needs work

The last submitted patch, profile2-8.x-1.x.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
12.21 KB

Updated for new Field UI permissions.

sun’s picture

Status: Needs review » Active
sun’s picture

Status: Active » Reviewed & tested by the community
FileSize
962 bytes
sun’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, profile2.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
sun’s picture

Status: Needs review » Active
sun’s picture

Status: Active » Needs review
FileSize
2.04 KB

Updated for latest changes in HEAD.

sun’s picture

FileSize
6.42 KB

Accounting for most feedback items from @webchick in http://drupal.org/node/1668292#comment-7081990

sun’s picture

FileSize
8.09 KB

Now including additional "Save and manage fields" submit button on the profile type creation form.

sun’s picture

Status: Needs review » Active
steinmb’s picture

Bumping the thread. Where do we stand here?

nevergone’s picture

What can we do to move this forward?

kscheirer’s picture

Title: Port Profile2 to D8 and prepare for Drupal core inclusion » Port Profile2 to D8 and provide upgrade path from D7 core profile
Status: Active » Needs review

Well, 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.

kscheirer’s picture

Issue summary: View changes

Updated issue summary.

sergiu.popa’s picture

If I want to help, I need to apply all the patches in order to continue the work?

anavarre’s picture

172: profile2.patch queued for re-testing.

ebeyrent’s picture

Any updates on this project?

joshtaylor’s picture

I'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.

  • 133b0ed committed on 8.x-1.x
    Issue #1726822 by dakala, GoZOo: More work on the 8.x port.
    
fago’s picture

Issue summary: View changes
Status: Needs review » Needs work

dakala 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.

fago’s picture

Title: Port Profile2 to D8 and provide upgrade path from D7 core profile » Port Profile2 to D8

Fixing scope to porting only

paucku’s picture

Why the package in profile.info.yml is Core. Isn't profile2 a contrib module?

name: Profile
type: module
description: 'Provides configurable user profiles.'
package: Core
core: 8.x
version: VERSION
configure: entity.profile_type.collection
dependencies:
- user
- field
- entity_reference

dakala’s picture

@paucku: I think it's the ultimate goal to have this incarnation of Profile2 replace the Profile module that was removed from core.

bojanz’s picture

Status: Needs work » Fixed

Since 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.