Properly deprecate support for entity type label callbacks

+1 on doing something about this.

I'm working on #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name() and it's really a pain to sort this out.

If label_callback is to go on existing, it needs to be able to define a true Callable in annotation. That is, an array of an object and the method to call on it suitable for call_user_func().

If it isn't to go on existing (and it shouldn't, for the reasons mentioned above), then the User entity should just override label(), and then getUsername() can just call label().

We also have this one: #2112679: getUsername() should return the username getDisplayName() for the formatted user name which I bring up because it should use label() instead of adding another method.

So basically this question of whither label_callback is an architectural blocker for those others.

I'll make a patch.

This patch completely removes the ability to set label_callback for an entity.

In the case of User, we implement label() and it just calls getUsername().

It passes unit tests and a few simpletests... Let's see what the testbot says.

I fully support removing label_callback. It's well-overdue and no one should be using it (at least if they've been paying attention). The methods on the new objects are what people should be using.

Bump up to Major because it's an API change. Added beta evaluation.

Can we also remove?

/**
 * Format a username.
 *
 * @param \Drupal\Core\Session\Interface $account
 *   The account object for the user whose name is to be formatted.
 *
 * @return
 *   An unsanitized string with the username to display. The code receiving
 *   this result must ensure that \Drupal\Component\Utility\SafeMarkup::checkPlain()
 *   is called on it before it is printed to the page.
 *
 * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
 *   Use \Drupal\Core\Session\Interface::getUsername().
 */
function user_format_name(AccountInterface $account) {
  return $account->getUsername();
}

#2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name() is posponed on this issue.

Here is the draft for change record : https://www.drupal.org/node/2481845
Applied the patch, everything looks fine.

Still here... anyone care to RTBC? :-)

Yeah sure.

+++ b/core/modules/user/src/Entity/User.php
diff --git a/core/modules/user/src/Tests/UserEntityCallbacksTest.php b/core/modules/user/src/Tests/UserEntityCallbacksTest.php
deleted file mode 100644

Why remove this test? It still passes as far as I can see and is still a valid code path to test.

I agree that it would be nice to get rid of this. However, not fully convinced that it is still possible in the current phase.

I'm not convinced that the beta evaluation is correct.

a) Blocking the removal of a deprecated function is not unfrozen AFAIK. It just means that @deprecated is a lie.
b) The change in the User entity is actually possible *without* removing the concept of label_callbacks. We could just not use it anymore.

label_callbacks (and uri_callback) exist for two reasons:

a) When they were added in 7.x, we didn't have classes, so it was not *possible* to add methods. So it made sense in that world.
b) Just like the entity key, it is something that can be changed through alter hooks. I'm not sure how useful that actually is, we still use uri_callback to do per-bundle URL's for forum terms, but we wanted to find a different solution there for a long time too.

Also agree that there is no need to remove the that test, just rename it to no longer mention Callback.

Leaving RTBC so that a core committer can decide how to continue with this.

That seems like an excellent point, @Berdir!

+1 to this. Was just working on new entity and I saw "label_callback" in the user entity anotation and I was confused for a sec. I agree this should be removed as it tends to cause confusion for new developers.

Not sure why that last test pass failed... The test record doesn't say.

So I added the test back and named it UserEntityNameTest.

Yes, label_callback is from a time before OOP. But we're in a situation where we have some confusion about how to name users... For instance we have the label() method which is annotated in the entity as t('User'), which would lead me to believe that we shouldn't use label() as a display name, or really even alter it. But then we have label_callback which in use changes the user name, not the label.

So the fewer ways to confuse the issue the better. We should have a clearly-defined API around user names moving forward. That's where #2112679: getUsername() should return the username getDisplayName() for the formatted user name is coming from, as well as the work to deprecate user_format_name(). If there are concerns about how developers can and should alter the name and label, we should address those in #2112679: getUsername() should return the username getDisplayName() for the formatted user name. If the preferred method for doing that is a label_callback, then I give up. :-)

label_callback is a throwback, has no documented behavior, and is confusing. It should go.

Oops, the dreaded double-upload.

Also, setting back to RTBC prematurely. If the test fails it will go back to needs work. Please change the status if desired.

RTBC as per #17.

Reroll.

Re-RTBCing as it's just a reroll...

Thanks everyone for your work on this issue! However, I don't think we can go forward with it during this phase of the release cycle. See also @Berdir's feedback in #12 which seems to have been overlooked.

The beta evaluation for this issue needs work. The change is definitely not unfrozen. Refer to https://www.drupal.org/core/beta-changes#unfrozen for what is unfrozen:

• CSS
• markup
• translatable strings
• documentation
• automated tests

This issue is a task that introduces a BC break, and per https://www.drupal.org/core/beta-changes, we should postpone this BC break to 9.x, or potentially to 8.1.x if a BC layer is provided. I think we could do as @Berdir suggests and simply deprecate it and remove internal usage, so postponing it to 8.1.x for that.

We can only proceed with this for 8.0.x if someone can provide clear justification as to how it fits within the allowable changes for the beta.

I also don't think it's major -- how does it have significant impact? See the examples of major tasks: https://www.drupal.org/core/issue-priority#major-task So setting to normal. Introducing an API change does not make an issue major by itself, I don't think.

Also, as far as I can tell, @Mile23 RTBCed his own patch, and @Crell and @JeroenT just restored that status. @Mile23, you can't RTBC your own patch. :) Someone else does a code review and then sets it RTBC if they think it's ready.

If I'm working on it, this not-really-an-API-change is needed before #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name() can happen.

If someone else wants to step in, they're welcome.

See also @Berdir's feedback in #12 which seems to have been overlooked.

That's because he's wrong, and because there are other fundamental problems with the way the User entity gets its name. See #16. https://www.drupal.org/node/2450793#comment-9897065

Removing this code- and cognitive-complexity is a reasonable goal, as is making a clear API for core and contrib. The @deprecated is not a lie. The reason this is a separate issue from the @deprecated one is because it changes the API for the better in one way, and opens a discussion about how to change the API for the better in others.

If #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name() is impossible, then the function is indeed not really deprecated and what we should do is remove the deprecation until it can be deprecated properly, and then re-deprecate it for 9.0.

I agree that removing complex, problematic code is definitely a reasonable goal, and one we should do. However, that doesn't mean it's in scope to do during the beta phase. Please review: https://www.drupal.org/core/beta-changes I think we could start on this by deprecating it for 9.0 now and then removing usages in core in 8.1.x. That's why I postponed the issue, versus marking it wontfix or something.

Also, I don't think @Berdir is wrong; I think he's correct. He's also an Entity API maintainer.

I'll leave this at review for more feedback for a couple days, but I will re-postpone it after that otherwise. I removed the line about the change being unfrozen from the summary, because it's not unfrozen.

Can we file a separate issue to mark the label callback and user_format_name() as deprecated for 9.0?

Oh, I should also point out that it's not true that label_callback has no documented behavior. It actually is documented, in EntityTypeInterface::getKeys(). The documentation is confusing, and hard to discover, and not in the right place, but it is documented. So I'm increasingly firmly convinced that we cannot remove this in 8.x. There could easily be sites and modules out there already relying on this documented, tested callback, even if it's confusing, bad DX, and redundant at this point.

What we should do in 8.0.x in addition to deprecating it is add @todo everywhere it is used referencing the issue to remove it (this one), to further discourage contrib from using it.

Regarding #34: #2495301: Deprecate user_format_name() and the label_callback for 9.x (not 8.x)

Thanks @cilefen for getting that filed, and thanks @Mile23 for the work on this and the additional feedback.

Re-scoping and postponing to 8.1.x per #31, #34, and #35 in my capacity as release manager. The actual removal will need to happen in 9.x, but we will deprecate it now in #2495301: Deprecate user_format_name() and the label_callback for 9.x (not 8.x) and convert uses after the beta and RC phases.

Please consult me or another release manager if you think this should be re-evaluated (rather than changing the issue statuses). Thanks!

No one wants to keep it. But the schema issue was major, and this is normal.

This actually is documented in D7: https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func... but the docs must have been removed at some point even though the functionality hasn't been!

We should also add a CR here and @trigger_error() make sure it is no longer called. Note we either can't add it on getLabelCallback() itself or we have to get the property directly, as we still have to support the concept, so the @trigger_error() needs to happen when someone defines that property, not when the method is used. (or as mentioned, we work around it by calling get('label_callback')

Or we could undrprecate it because no other way yo customize label per bundle

@andypost: I think you're mixing that up with the (completely broken) url callback, I don't see any indication of a per-bundle support for label callbacks in the label() methods. That said, we have entity types with things like per-plugin callbacks, so they can always decide to support whatever they wan't, it's just not generic.

I also think we should just drop support for that uri callbacks too, it just doesn't play well with the everything else like url templates and routes. People can use aliases to have nice URL's, which would also work out of the box in pathauto if it wouldn't have to try and support the broken system around forum urls.

Here's what I proposed. Doing a @trigger_error() in getLabelCallback() only if one is actually defined and including which entity type it is. Changed User to just override label() instead. Renamed UserEntityLabelCallbackTest but to rename Callback otherwise left it as-is.

Kept the other usages for now to see the deprecation in action.

@Berdir yep, mixed things - why only has() deprecated?

Not sure what you mean?

Also, looks like this needs to be postponed on the not-yet-existing issue in jsonapi to remove its incorrect usage of getLabelCallback(). See #3042745: Remove group @legacy from jsonapi tests and fix deprecation messages

Also, fun facts, there are two other entity types with a label callback:

* A test entity type that is only used for a single test due to its long table name, the label callback that it defines doesn't exist, so apparently nothing calls label() on that.
* RestResource, which defines the label callback as a method (without class name, so that wouldn't work), but again, nothing seems to call label().

Both would just silently do nothing in HEAD as we are doing an is_callable() check on the value. But they would trigger the deprecation mesage.

Slightly different approach, fully deprecating all these methods and instead using get() and adding the @trigger_error() for that directly in the label() implementations.

For the jsonapi usage, I'm just removing the pseudo-generic implementation with a check for user entities and created #3057175: Implementation of user name in JSON:API can result in overwriting data because I think that implementation is actually incorrect anyway.

Thanks for pushing this forward, and for creating #3057175: Implementation of user name in JSON:API can result in overwriting data!

I reviewed this, and it's looking super close to ready. We still need to fix the remaining test failures of course. Here's an initial patch for that.

> I reviewed this, and it's looking super close to ready. We still need to fix the remaining test failures of course. Here's an initial patch for that.

Looks that wasn't just the initial patch, now we need someone else to RTBC it I suppose, since you approved everything else and specifically the change to jsonapi, that should be just about the test changes and the usual deprecation nitpicking, I noticed we still use will be removed from wording.

Functionally, I believe this approach is correct and fine, and ready to go. I'm setting to NW because the deprecation messages aren't up to standards, and the required @group legacy tests for each deprecation error are missing.

Specifically:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1246,7 +1246,8 @@ public function __clone() {
   +      @trigger_error('Entity type ' . $this->getEntityTypeId() . ' defines a label callback. Support for that is deprecated in drupal:8.0.0 and will be removed in drupal:9.0.0. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -166,7 +166,8 @@ public function bundle() {
   +      @trigger_error('Entity type ' . $this->getEntityTypeId() . ' defines a label callback. Support for that is deprecated in drupal:8.0.0 and will be removed in drupal:9.0.0. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   

   These are okay, They meet the relaxed format of %thing% is deprecated in %deprecation-version% (free text describing what will happen) %removal-version%. %extra-info%. See %cr-link%

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -665,6 +665,7 @@ public function setLinkTemplate($key, $path) {
   +    @trigger_error('EntityType::getLabelCallback() is deprecated. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -488,15 +488,13 @@ public function setLinkTemplate($key, $path);
   -   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   +   * @deprecated in drupal:8.0.0 and will be removed before Drupal 9.0.0.
       *   Use Drupal\Core\Entity\EntityInterface::label() for complex label
       *   generation as needed.
   

   These should match and be in the format %thing% is deprecated in %deprecation-version% and is removed from %removal-version%. %extra-info%. See %cr-link%

   The method also needs an @group legacy test to test for the deprecation error.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -672,6 +673,7 @@ public function getLabelCallback() {
   +    @trigger_error('EntityType::setLabelCallback() is deprecated. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -508,7 +506,7 @@ public function getLabelCallback();
   -   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   +   * @deprecated in drupal:8.0.0 and will be removed before Drupal 9.0.0.
       *   Use EntityInterface::label() for complex label generation as needed.
   

   same

4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -680,6 +682,7 @@ public function setLabelCallback($callback) {
   +    @trigger_error('EntityType::hasLabelCallback() is deprecated. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -522,7 +520,7 @@ public function setLabelCallback($callback);
   -   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   +   * @deprecated in drupal:8.0.. and will be removed before drupal:9.0.0.
       *   Use EntityInterface::label() for complex label generation as needed.
   

   Same

5. +++ b/core/modules/user/user.module
   @@ -436,12 +436,13 @@ function user_preprocess_block(&$variables) {
   - * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   - *   Use \Drupal\Core\Session\AccountInterface::getDisplayName().
   + * @deprecated in Drupal 8.0.0 and will be removed in Drupal 9.0.0.
   + *   Use \Drupal\Core\Session\AccountInterface::getDisplayName() instead.
     *
   - * @todo Remove usage in https://www.drupal.org/node/2311219.
   + * @see https://www.drupal.org/node/3050794
     */
    function user_format_name(AccountInterface $account) {
   +  @trigger_error('user_format_name() is deprecated in drupal:8.0.0 and will be removed in drupal:9.0.0. Use $account->label() or $account->getDisplayName() instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
   

   @deprecated in drupal:8.0.0 and is removed from drupal:9.0.0 etc.

   The annotation should match the error message

6. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -496,7 +496,8 @@ public function testLabel() {
   -      ->method('getLabelCallback')
   +      ->method('get')
   +      ->with('label_callback')
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
   @@ -181,11 +181,9 @@ public function testLabel() {
   -      ->method('getLabelCallback')
   +      ->method('get')
   +      ->with('label_callback')
          ->will($this->returnValue([$callback_container, __FUNCTION__]));
   -    $this->entityType->expects($this->at(1))
   -      ->method('getLabelCallback')
   -      ->will($this->returnValue(NULL));
   

   These @group legacy tests generate a dynamic deprecation message. The test class should use Drupal\Tests\Traits\ExpectDeprecationTrait and the test method should call

   $this->expectDeprecation('Entity type ' . $this->entityTypeId . ' defines a label callback. Support for that is deprecated in drupal:8.0.0 and will be removed in drupal:9.0.0. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794');
   

   to test for the deprecation.

Thanks for the review.

Updated the deprecation messages and added legacy tests.

Tests and messages look good. One small nit. I'll leave NR for now.

+++ b/core/lib/Drupal/Core/Entity/EntityBase.php
@@ -166,7 +166,8 @@ public function bundle() {
-    if (($label_callback = $entity_type->getLabelCallback()) && is_callable($label_callback)) {
+    if (($label_callback = $entity_type->get('label_callback')) && is_callable($label_callback)) {
+      @trigger_error('Entity type ' . $this->getEntityTypeId() . ' defines a label callback. Support for that is deprecated in drupal:8.0.0 and will be removed in drupal:9.0.0. Override the EntityInterface::label() method instead. See https://www.drupal.org/node/3050794', E_USER_DEPRECATED);
       $label = call_user_func($label_callback, $this);
     }

We add a BC layer here and trigger an error. Should we have a follow-up against 9.0.x and an @todo or other in-code documentation to remove this when the drupal 9 branch opens?

Hm, not sure what our policy is in regards to that. We have tons of deprecations without an explicit follow-up referenced in code I think, and everything that has a @trigger_error() should be easy to find :)

Personally, I don't think having a todo referenced in the patch would help much, I could just create an issue and reference it from here if you think that would be useful?

I think that would be useful. I don't think we have an official policy in regards to in-code documentation of BC-layers, but I think we should. I opened #3069389: [Policy, no patch] Determine and document best practices for providing backwards compatibility code paths expected to be removed in the next major version. a few days ago, but no input yet.

I just wanted to point it out, I think we are going to have a rough time finding and removing all the BC layers without consistant in-code documentation, but it's too late now. I'm hopeful we can have a policy to start using in Drupal 9 though.

Everything in the patch itself looks fine, all feedback is addressed. I'm happy to RTBC.

Looks great! RTBC++

I didn't know in #60 whether my quick fix would solve all failures, looks like it did 😀

Committed ffe6995 and pushed to 8.8.x. Thanks!

The change notice for this has

To change the label of an existing entity type, create a module and have it implement hook_entity_type_alter():

function mymodule_entity_type_alter(array &$entity_types) {
  $entity_types['user']->set('label', t('Person'));
}

Is that correct? Should it instead talk about overriding the entity class with a custom label implementation?

There are two change records, both draft. One is from 2015 and yes, that is not correct.

Mine, while not very elaborate, is better. Published that now.

And yes, doing that for an entity type that is not controlled by you would require to replace the entity class, but that is a rare edge case.. I hope?