Port Profile2 to D8 and prepare for Drupal core inclusion

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.

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

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)

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

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.

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

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

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

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

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

Forgot screenshot.. :)

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

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

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.

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

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

I'm back. Thank you very much fubhy!

I will try to get this patch green.

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

Why am I not allowed to delete my own profile as a user?

I just see that the original code didn't allow for this either, so this should probably be adjusted/fixed in a follow-up patch; laters.

This message should dynamically adjust to the current user; i.e., whether you are editing your own profile, or whether you are a site administrator editing another user profile.

For regular profiles, this shouldn't be needed.

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

::save() returns $status, let's differentiate between created and updated in the message.

In general, see #1798944: Convert config_test entity forms to EntityFormController for a prime/master example config entity conversion.

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

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

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

This @todo doesn't make too much sense to me. Let's remove it and discuss what can or needs to be improved on-issue instead.

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

Removed profile2_view.

Clarifying issue title.

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

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.

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

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

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.

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

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

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

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.

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

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.

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

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.

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

Re-roll

So sorry for the noise. :/

This patch is now actually the right one. Sorry!

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.

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

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.

Committed #79 and #76.

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

Remarks on #77:

s/edit/delete/g

Entity delete confirmation form constructors are expected to always add a form #type value using the entity property name of the entity ID as key; i.e.:

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

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

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

Oh, that was not the full patch

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

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

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

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

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

Converted default configuration into configuration.

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

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.

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

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

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 / UserInterface interface for use in \Drupal\Core namespaces and #1634280: drupal_anonymous_user() should return a User entity

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

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 / UserInterface interface for use in \Drupal\Core namespaces

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

Ultra minor tuning for the entity_delete_multiple patch.

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

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.

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.

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

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

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

Attached patch should address all issues that have been raised in #1668292-35: Move simplified Profile2 module into core (and some additional issues I encountered while stepping through the code and comparing it with other modules in core).

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.

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.

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

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.

Alright, now including temporary stop-gap fix.

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

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.

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?

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

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

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

Chasing HEAD.

Jesus.

Wow... #1852966: Rework entity display settings around EntityDisplay config entity introduced a DX that is a PITA.

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

@fubhy figured fit fout. :)

Updated for new Field UI permissions.

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

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

What can we do to move this forward?