Make profile label able to be saved and so work with Auto Entity Label or other alterations that work on save

Looks like the profile label is never stored in the db, so the auto_entitylabel module won't work (tested to verify).

Digging into the profile module it appears the label does fire an event for overriding the label, so here is an example of code I'll end up using to alter the labels if some other feature is not created.

Register an event subscriber service:

mymodule.services.yml

services:
  profile_label_alter:
    class: '\Drupal\mymodule\EventSubscriber\ProfileLabelEventSubscriber'
    tags:
      - { name: 'event_subscriber' }

Event subscriber - set the new label and return the event.

mymodule/src/EventSubscriber/ProfileLabelEventSubscriber.php

<?php
namespace Drupal\mymodule\EventSubscriber;

use Drupal\profile\Event\ProfileEvents;
use Drupal\profile\Event\ProfileLabelEvent;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class ProfileLabelEventSubscriber implements EventSubscriberInterface {

  use StringTranslationTrait;

  /**
   * @return array The event names to listen for, and the methods that should
   *   be executed for the event.
   */
  public static function getSubscribedEvents()
  {
    return [
      ProfileEvents::PROFILE_LABEL => 'profileLabel'
    ];
  }

  /**
   * Subscribe to the user login event dispatched
   *
   * @param ProfileLabelEvent $event
   *
   * @return ProfileLabelEvent
   */
  public function profileLabel( ProfileLabelEvent $event )
  {
    $event->setLabel($this->t('@owner - @type', [
      '@owner' => $event->getProfile()->getOwner()->label(),
      '@type' => $event->getProfile()->bundle()
    ]));
    return $event;
  }

}

Not ideal, but hope this helps!

Not sure if this is the direction we want to go, but here is a first attempt at a patch that adds labels proper to Profile entities.

Changes:

• Adds a new entity_key and base field definition for label. Unsure how we want to make them available to administrators, I've left them as unmanageable for the moment.
• Renamed the existing label() method to defaultLabel().
• New label method still fires the ProfileLabelEvent for alterations.
• Update hook adds the new base field to the database table, then updates all existing profile rows with their label.

I suspect there are many problems with this atm, and I'm not suggesting we go with this patch as-is. Just wanted to get the conversation going around whether or not we want to go this route.

Feedback /suggestions welcome.

I have tested the patch and solved the issue of be able to set custom labels on profile entities in a project I'm currently working on. It seems that we will need to resolve the tests failures in order to have a definitive solution for this problem, right? Any direction to help on this?

Thanks.

I think we need maintainer feedback on the desired approach. If adding label as a new base field is okay, I'm happy to re-roll patch with improvements and tests.

1. +++ b/profile.install
   @@ -34,3 +34,28 @@ function profile_update_8001() {
   +function profile_update_8002() {
   

   Although I generally run updates from the CLI, since Drupal allows users to use update.php we should queue around 10 at a time using the batch/sandbox parameter

2. +++ b/src/Entity/Profile.php
   @@ -69,6 +70,19 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +    $label = parent::label();
   

   I already didn't like we ran the event on this invoke -- because what if multiple are loaded, or Views does something stupid.

   We should store on preSave, and always run. Because that way it is up to date whenever saved.

I have one request: Let's add an event subscriber to ProfileEvents::PROFILE_LABEL in profile_test. In the subscriber, just check if the field is equal to some value, and then modify.

Rerolled

@mglaman thanks for the notes and direction
@marthinal thanks for the re-roll

Updated patch with the following:

1. hook_update_N now uses the batch sandbox.
2. On preSave, set the label value to the defaultLabel (event-provided label) if there is no current label value.
3. Added event subscriber to profile_test. Checks if the label is literally "some value" and if not, sets that value. @mglaman Not 100% sure this is what you meant. Happy to re-roll if incorrect.

I've tested this in various ways: This is definitely saving the labels to the database, and I was successfully able to use the autoentity_label module to provide a profile's label automatically using tokens.

Working on making tests pass and I found an issue with the preSave() approach to setting the default label.

Since the default label wants the $profile->id() but the object doesn't have an id() before its first save, we can't expect correct default labels to be generated during preSave(). What happens is that the profile ends up with a label like: "Personal profile #" (without the id).

I've updated the patch to instead provide the default label during postSave() during first db insert ($update = false). Not sure this is the best approach, feedback welcome.

I've also updated Functional\ProfileTest::testCreateEditProfile() to expect the profile's provided label to be the title for the edit form, as opposed to the label pattern provided by the defaultLabel().

@daggerhart I think he meant for the "some value" to be in the test not in the implementation to ensure/assert it works. Not literal EDIT: Ignore me, you did it right I think;)

Thanks for your effort @daggerhart.

The patch is working great for new profiles. I'm wondering if it is a good idea, to fire it only on

if (!$update) {

This basically seems to lock the label forever, until we have a UI for setting or prepopulating the label. Or was it only meant for covering the defaultLabel fallback?

That leads me also to the question how to proceed with the initial issue summary + title. Maybe we should split it up between the groundwork that was done by @daggerhart here and the initial UI based request.

Reroll due to owner being added:

patching file profile.install
Hunk #1 succeeded at 124 with fuzz 2 (offset 70 lines).
patching file src/Entity/Profile.php
Hunk #1 FAILED at 48.
Hunk #2 succeeded at 73 (offset 3 lines).
Hunk #3 succeeded at 94 (offset 3 lines).
Hunk #4 succeeded at 274 (offset 3 lines).
Hunk #5 succeeded at 319 (offset 4 lines).
1 out of 5 hunks FAILED -- saving rejects to file src/Entity/Profile.php.rej
patching file tests/profile_test/profile_test.services.yml
patching file tests/profile_test/src/EventSubscriber/ProfileLabel.php
patching file tests/src/Functional/ProfileTest.php

Also needs the update function updated.

Change missing but interdiff was correct

@joelpittet - update function needs to update again... 8007 should work.

Let's see

Needs work - #26 failed to apply.

1. +++ b/profile.install
   @@ -124,3 +124,45 @@ function profile_update_8005() {
   +    $profile_ids = \Drupal::entityQuery('profile')->execute();
   

   Entity access checks will cause this to return zero profile IDs.

   This method is deprecated for grabbing the query from storage.

2. +++ b/profile.install
   @@ -124,3 +124,45 @@ function profile_update_8005() {
   +  $profile_ids = \Drupal::entityQuery('profile')
   

   This method is deprecated for grabbing the query from storage.

3. +++ b/profile.install
   @@ -124,3 +124,45 @@ function profile_update_8005() {
   +    $profile = \Drupal\profile\Entity\Profile::load($profile_id);
   

   Lets use the storage.

4. +++ b/profile.install
   @@ -124,3 +124,45 @@ function profile_update_8005() {
   +  drupal_set_message($sandbox['current'] . ' profile labels processed.');
   

   Deprecated function.

Here is a new patch, that should apply.

Review:

1. +++ b/src/Entity/Profile.php
   @@ -54,7 +54,8 @@ use Drupal\user\UserInterface;
   + *     "label" = "label"
   

   I wonder if we need to run entity key updates like the previous updates. I don't know for sure.

2. +++ b/src/Entity/Profile.php
   @@ -77,6 +78,13 @@ class Profile extends ContentEntityBase implements ProfileInterface {
       * {@inheritdoc}
       */
      public function label() {
   +    return parent::label();
   +  }
   

   No need to override this method anymore.

3. +++ b/src/Entity/Profile.php
   @@ -77,6 +78,13 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +  public function defaultLabel() {
        $profile_type = ProfileType::load($this->bundle());
        $label = $this->t('@type profile #@id', [
   

   Is this on any interface?

4. +++ b/src/Entity/Profile.php
   @@ -91,6 +99,18 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +  /**
   +   * Set the label for this Profile entity.
   +   *
   +   * @param $value string
   +   *
   +   * @return Profile
   +   */
   +  public function setLabel($value) {
   +    $this->set('label', $value);
   +    return $this;
   +  }
   

   This should be on the parent already, correct?

5. +++ b/src/Entity/Profile.php
   @@ -283,6 +303,14 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +    if (!$update) {
   +      // If no label was provided before insert, update with the default label.
   +      if (empty($this->label())) {
   

   These can be on the same line. Should we just check "isNew" versus not update?

Needs work based on review.

Thanks for the review @mglaman.

1. I don't know the answer here, who can we ask?
2. Removed
3. It's not but searching through a D8 the naming of getDefaultLabel() is more common, so changed it to that. Do you want to create an interface for this?
4. It's not on ContentEntityBase, just like setOwner() isn't
5. It's one less function call as-is, do we need to change this?

Test fail was bogus, setting to NR

@joelpittet thanks for the reroll to address my concerns!

I don't know the answer here, who can we ask?

Actually, the status report page. If there are no entity definition update warnings... then my concern was moot!

It's not but searching through a D8 the naming of getDefaultLabel() is more common, so changed it to that. Do you want to create an interface for this?

I feel like it should if the method is public. Does it appear to be internal, though? The only public usage is the update hook which updates existing profiles. And think we should probably skip this. There are Drupal Commerce sites with 10,000 or so profiles for anonymous users. Users with CRM or other instances can write their own update hook to change profile labels.

It's not on ContentEntityBase, just like setOwner() isn't

Yeah, I figured setOwner wouldn't be, as it is not the main interface method. It looks like methods like this to set the label key are always bespoke for the entity and should be declared on the interface as well.

It's one less function call as-is, do we need to change this?

:P no, lack of line context I did not realize the method signature

public function postSave(EntityStorageInterface $storage, $update = TRUE) {

Two more nits. I'm still concerned about the update process. Maybe we should just do an entity query for all where uid is not zero/anonymous.

Also... I'm putting to needs work because the issue title states this is about tokens and the user interface. Yet I don't see any of that here? And if the label is manually set, we no longer fire \Drupal\profile\Event\ProfileEvents::PROFILE_LABEL. Shouldn't this always be fired and the "default" response is to use whatever tokens were configured for the profile type? The main thing I am seeing out of this patch is a performance boost by not always dynamically generating the profile title.

But what about Commerce? Where the label is the address line. Now it would be static. There's a lot of questions here.

1. +++ b/src/Entity/Profile.php
   @@ -74,9 +75,9 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +  public function getDefaultLabel() {
   

   Let's make this protected. There is no visible reason for it to be visible... except in the update hook :/ So maybe we we can !$update && notEmptyLabel in preSave.

2. +++ b/src/Entity/Profile.php
   @@ -324,6 +346,19 @@ class Profile extends ContentEntityBase implements ProfileInterface {
   +      ->setDisplayConfigurable('form', FALSE)
   +      ->setDisplayConfigurable('view', FALSE);
   

   I think this is redundant as the default is always false.

@mglaman,

1. The tokens part just works once this base field exists, so ✅for that working.
2. Not sure I understand your uid concern, can you elaborate?
3. Making getDefaultLabel() private and moving it to the preSave() opens up some annoyances for updates. Brain's not working well at the moment to explain better than that.
4. I'm ok with removing the display config, trust you know this better than I.
   

     ->setDisplayConfigurable('form', FALSE)
     ->setDisplayConfigurable('view', FALSE)

Are you suggesting we fire the event on every label change? I think I'm down with that but want to make sure that is what you are suggesting?

This makes the method protected but lets setLabel() deal with the empty values.

The tokens part just works once this base field exists, so ✅for that working

Rad. I wasn't sure.

Not sure I understand your uid concern, can you elaborate?

My concern is here, in the update hook

+++ b/profile.install
@@ -160,3 +160,41 @@ function profile_update_8008() {
+  $profile_storage = \Drupal::entityTypeManager()->getStorage('profile');
...
+    $profile_ids = $profile_storage->getQuery()->accessCheck(FALSE)->execute();
...
+  $profile_ids = $profile_storage->getQuery()->accessCheck(FALSE)
+    ->range($sandbox['current'], $sandbox['current'] + $sandbox['_per_batch'])
+    ->execute();
+  $profiles = $profile_storage->loadMultiple($profile_ids);

Loading anon profiles

Making getDefaultLabel() private and moving it to the preSave() opens up some annoyances for updates. Brain's not working well at the moment to explain better than that.

Yeah, update becomes annoying. But feels weird to have it exist only for updates. I wonder if we could do a preSave to generate the label if its empty.

Are you suggesting we fire the event on every label change? I think I'm down with that but want to make sure that is what you are suggesting

I guess. Or something. Maybe on each save? So instead of every time we view a label the event is generated, we just generate on preSave. This way the label is always generated with the latest labels. Right now the label is super static, generated only when the profile is first created.

@mglaman I'm a little worried that if we have an empty check and the default returns empty that we end in a loop of ♾

What if on the edit form for the profile a checkbox was displayed, "Update Profile Label".
Or have a configuration option to determine the behavior of how the label is updated in:

Configuration > People > Profile Types > Profile > Edit

Thoughts?

Is there going to ever be a display to browser the revisions of a profile?

@emptyvoid That could be one way to solve it... I'd prefer to avoid config options if possible but if that's the only way this will work I'm interested.

Rerolled.

Rerolled for the update hook.

Hi joelpittet, I wanted to test this patch. But I am a bit misled on where to change the label. The title of this issue is "Make profile label work with tokens and expose them in the UI", but from the looks of the code I need an event subscriber to return the value of the label?

I am definitely scared by the update hook. Drupal's update process is unreliable and we can potentially have hundreds of thousands of profiles on a site. It feels too late to do such a thing now that the module is widely used in production.

Is there a downside to only populating labels on preSave()? And documenting how a site could perform the update manually?

+  /**
+   * Set the label for this Profile entity.
+   *
+   * @param string $value
+   *   The new label value.
+   *
+   * @return $this
+   */
+  public function setLabel($value) {
+    if (empty($value)) {
+      $value = $this->getDefaultLabel();
+    }

You can't add a public method to an entity if it's not on the interface.
Also, it looks really odd for a setter to care about setting a default value. We don't do that anywhere else in Drupal core or contrib.

There's a ProfileTest kernel test for testing profiles, but it wasn't expanded for the new functionality.

I'm up for the preSave() approach if there are no objections.

@bojanz Are you suggesting I add setLabel() to the interface profile interface?

Here's a WIP where I removed the update code label slow code(still has an update hook to create the field), and changed getDefaultLabel to protected.

Haven't done the test and preSave() solution, still pondering it

I haven't been following this issue closely enough so this is a general comment and question. Pathauto uses hooks to generate paths, presumably so it can use all entity data. Would these paths be available in preSave() or postSave() ? Or is that not relevant here?

Ref: #2972505-15: Pathauto 1.2 is not generating an alias after upgrading from 1.1

+1
Applied the patch but couldn't find the option to change the label. Should I find the new field here? /admin/config/people/profiles/manage/myprofiles/form-display

Rerolled removing the test change from #47 This is still a bit of a Work In Progress WIP

@cuman I'm not sure if it gives the label field to change in the UI, I've not tested that.

@imclean I don't think that is relevant here but I guess it could be because it allows tokens to work with the label...

Needs work to tackle @bojanz's comments in #45 Especially the test. Anybody looking for this please give it a try.

I am new to Drupal 8, Could you please help me on how this patch works? I mean how can i change profile label instead of default auto generated label. I was expecting a settings in admin profile configuration section but cant see a configuration to set that using tokens.

hi, just a quick note I tried patch #50, it made a change to the schema for label, but not in a clean way (ie left with outdated entity even after a drush updb), usually id run a drush entup, but thats now retired in 8.7+ so need to use devel_entity_updates.

Uninstalling and reinstalling the module, now the auto_entitylabel is working as expected !

Thank you @digitalcatalyst comment #53 was very helpful in terms of getting the patch to work. Thank you to all who worked on this patch!

The patch in #50 is working well for me. Thanks!

I am in a similar situation as @dev.paulson. I have upgraded the profile module to 8.x-1.0 and successfully applied the patch in #50 but I don't see any way of using tokens to edit the label. I tried devel_entity_updates but it said there was nothing to update. Please can someone offer me some advice? Thanks!

I've had another attempt at getting this working and managed to install patch #50 on Profile 1.1 and then successfully ran devel_entity_updates which this time did make some changes. I am not able to uninstall and reinstall the module though as I have a large number of profiles set up already. I still see no way of actually editing the profile labels using tokens or otherwise. Has anyone got this working to a point where you can edit profile labels? I am still using Profile version 8.x-1.0-alpha5 on my production site because of this issue and really need to upgrade! Thanks for any suggestions.

Reroll

I just installed and tested the patch in #58, and it worked like charm.

I also managed to modify the default label through an event subscriber, and I'm providing my code as an example here for others to make use of (hope this helps). This one just sets the profile label to the user's display label.

File 1: Create my_module/src/EventSubscriber/ProfileLabelSubscriber.php:

namespace Drupal\my_module\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\EventDispatcher\Event;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\profile\Event\ProfileEvents;


/**
 * Class ProfileLabelSubscriber.
 */
class ProfileLabelSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[ProfileEvents::PROFILE_LABEL] = ['setLabel', 50];

    return $events;
  }

  /**
   * This method is called when the setLabel is dispatched.
   *
   * @param \Symfony\Component\EventDispatcher\Event $event
   *   The dispatched event.
   */
  public function setLabel(Event $event) {
    $profile = $event->getProfile();
    $new_label = $profile->getOwner()->getDisplayName();
    $event->setLabel($new_label);
  }

}

File 2: Add entry to my_module.services.yml

  my_module.profile_label_subscriber:
    class: Drupal\my_module\EventSubscriber\ProfileLabelSubscriber
    arguments: ['@entity.manager']
    tags:
      - { name: event_subscriber }

One more point: I also agree with the comment in #45 that the setName method should not use the default label in case the provided argument is empty. In fact it's reasonable that in some cases somebody would want to use the setLabel method to remove any information stored in the database. I wouldn't want to strip anybody of the ability to do that. So I'm also for removing (what's currently) lines 115--117 in Profile.php:

    if (empty($value)) {
      $value = $this->getDefaultLabel();
    }

However, this is more of an API thing and I doubt it would affect whether this patch works or not, since the argument is provided in the only place where it's used (line 320).

        $this->setLabel($this->getDefaultLabel());

But apart from that it all looks great, so please get this committed soon!

None of this worked for me on 9.0.3, but this module did the trick:
https://www.drupal.org/project/auto_entitylabel

-i used tokens so it is perfect now.

@coaston
how did you config this? I was trying it without no success and found this issue:
https://www.drupal.org/project/auto_entitylabel/issues/2909742

Is this patch working with D9 or will it be integrated in the next stable?

I can confirm that auto_entitylabel does not work with unpatched 9.0.3. Applying the patch makes no UI changes to allow editing of a profile label. I am not sure why? Is there some desire to make that a separate issue/change?

Using auto_entitylabel, the "Re-save" functionality requires the patch from #3201476: ResaveBatch doesn't work for custom entity types to run without error. It only updates existing profiles if you choose the " Automatically generate the label and hide the label field" option, since there is no label field I suppose. For new profiles, the "auto generate if field left blank" also works.

nevermind, user error

The version of the patch in the merge request avoids trying to update existing labels. Instead, it allows the stored labels to stay blank, and keeps exactly the same behavior as before if that is the case. Now that we have a label base field where labels can be stored, however, this module can now be used with Auto Entity Label, which i think should be the main goal of this issue.

https://www.drupal.org/project/auto_entitylabel

There is not a need for this module to provide its own UI for altering the label pattern when Auto Entity Label exists.

With this merge request the only thing happening in the update hook is the necessary addition of the label field to the profile entity base, so there is no longer a risk of errors or timeouts when the update hook is run. Also, if people do not run the update hook right away, everything continues to work.