Deprecate user_format_name() and the label_callback for 9.x (not 8.x)

Thanks @cilefen for filing this!

Per #2495301: Deprecate user_format_name() and the label_callback for 9.x (not 8.x) and #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name() I think we also need to deprecate the label_callback key for 9.x explicitly in order for the deprecation for user_format_name() to be correct/complete. I think it'd be alright to do this in one patch. It would involve:

• Updating the docblock of the protected property in EntityType.
• Fixing the documentation about it in EntityTypeInterface::getKeys().
• Adding @todo referencing #2450793: Properly deprecate support for entity type label callbacks wherever the label_callback is used, especially within the User entity.
• Possibly adding it to the annotation definition only to say "Legacy and don't use this"?

Anything I'm missing?

Ah, we also need to document what to do instead for the label callback. #2450793: Properly deprecate support for entity type label callbacks suggests overriding the label() method, but AFAIK this would only work for subclasses (i.e. the module that defines the entity), whereas I think one could alter the label_callback in hook_ENTITY_TYPE_build() or hook_ENTITY_TYPE_alter(). I pinged @Berdir for feedback.

Ah, there's more we would need to deprecate, from the summary of #2450793: Properly deprecate support for entity type label callbacks:

1. Remove Deprecate 'label_callback' as a valid (but undocumented) annotation element.
2. Remove Deprecate the EntityTypeInterface::getLabelCallback(), EntityType:Interface:setLabelCallback(), EntityTypeInterface::hasLabelCallback(), EntityType::getLabelCallback(), EntityType::setLabelCallback(), EntityType::hasLabelCallback() methods and the EntityType::$label_callback instance variable.
3. Remove test system/tests/modules/entity_test/src/Entity/EntityTestLabelCallback.php The test should remain until we actually remove it.
4. Rewrite Entity::label() to remove references to getLabelCallback() in #2450793: Properly deprecate support for entity type label callbacks
5. Remove deprecated user_format_name() in #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name()

Sounds about right.

4. Isn't going to happen before 9.x because that would essentially mean that is no longer supported.
5. As commented before, we can actually stop using it as a label callback internally in the user entity without removing the function completely. We're still doing all kinds of code cleanups to reduce technical dept and I can see an argument for doing that. But that's up to the core maintainers to decide in the end :)

Related issues has collision but but doc block should also point to right interface

+++ b/core/modules/user/user.module
@@ -398,7 +398,7 @@ function user_preprocess_block(&$variables) {
- * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+ * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
  *   Use \Drupal\Core\Session\Interface::getUsername().

9.0 is mostly all over core... is there a rule about?

Also "Use ...AccountInterface:: ..." - function should point to right interface

addressed #7

Done 1-3 , as we wait for feedback.

The only broken place

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -108,8 +108,7 @@ public function getOriginalClass();
    *     $entity->subject, then 'subject' should be specified here. If complex
-   *     logic is required to build the label, a 'label_callback' should be
-   *     defined instead (see the $label_callback block above for details).
+   *     logic is required to build the label.

This sentence incomplete, suppose better mention that it would be removed when alternative solution will be found and point to isue

point to isue

Do we need to file a new followup for the same? Have question so not changing the issue status.

Sorry bymistake I change the issue status to needs review :(

1. +++ b/core/includes/entity.inc
   @@ -33,6 +33,11 @@ function entity_render_cache_clear() {
     * @see \Drupal\Core\Entity\EntityManagerInterface::getAllBundleInfo()
   + *
   + * @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0. Use the
   + *   \Drupal::entityManager()->getBundleInfo($entity_type) to get the info of a
   + *   known entity type and use \Drupal::entityManager()->getAllBundleInfo() for
   + *   all bundles.
     */
    function entity_get_bundles($entity_type = NULL) {
   

   This function has already been marked as @deprecated, so when the patch applies, it's marked as @deprecated twcie.

   This is probably the case for other functions here, as well.

   Also, @see comes after @deprecated. https://www.drupal.org/coding-standards/docs#order

2. +++ b/core/modules/user/user.module
   @@ -401,8 +401,10 @@ function user_preprocess_block(&$variables) {
   - * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
   - *   Use \Drupal\Core\Session\Interface::getUsername().
   + * @todo Remove usage in https://www.drupal.org/node/2311219
   + *
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\Session\AccountInterface::getUsername().
   

   Again, @todo comes after @deprecated, and @see.

I suggest not modifying entity.inc and letting that happen here: #2474151: Mark procedural wrappers in entity.inc as deprecated

Addressed feedback given by Mile23 in #16 + removed changes in entity.inc file.

Reworded the comment for getKeys and fixed a small typo.

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -588,6 +590,8 @@ public function setLinkTemplate($key, $path) {
    * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
    */
   public function getLabelCallback() {

@@ -595,6 +599,8 @@ public function getLabelCallback() {
    * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
    */
   public function setLabelCallback($callback) {

@@ -603,6 +609,8 @@ public function setLabelCallback($callback) {
    * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
    */
   public function hasLabelCallback() {

They are deprecated at interface definition, so this inherited all over.
please remove that

Removed the deprecate comments in EntityType class.

I think no reason to have @todo in each deprecated function so RTBC

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -109,7 +109,9 @@ public function getOriginalClass();
    *     label. For example, if the entity's label is located in
    *     $entity->subject, then 'subject' should be specified here. If complex
    *     logic is required to build the label, a 'label_callback' should be
-   *     defined instead (see the $label_callback block above for details).
+   *     defined instead (see the $label_callback block above for details) that
+   *     has been deprecated. It will be removed when an alternative solution
+   *     has been implemented.

We still telling people to use label_callback. When we deprecate we have to have to be to tell people what to do instead. I think that is implement Entity::label() on their on object.

Hopefully this addresses #23. Thank you for the review.

For the testbot to kick in.

Looks each deprecated method needs point what to use instead

EntityTypeInterface::getLabelCallback() is documented as returning a callable.

Therefore, EntityType::label_callback should also be a callable (or NULL) rather than a string.

This issue should also remove the label_callback annotation from Drupal\user\Entity\User class, since all it does is *wrongly* call user_format_name() which gives us the name, not the label.

There should also be a way to denote deprecation for 8.1.x, but apparently that doesn't exist. All the subordinate postponed issues punt on this until 8.1.x, not 9.x. Because this is the system, we *must* support this under-documented, poorly-considered API beyond 8.1.

Still boggles my mind that this is the kind of thing that gets swept under the rug, even when all the work is already done, simply because it doesn't count as 'critical.'

@Mike23
Thanks for your hint.
I have updated patch as per your guidance.

Going back to #24 because #28 and #30 have unneeded deletions.

Some doc tweaks to explain reality better, added a bunch of @see, addressed changes from #27.

Without the 'code' change.

Needs a followup to figure out what User's label_callback should actually be.

Patch looks good by me and I think that the concerns of alexpott in #23 are addressed now.

Still applies. :-)

Until this is in, user_format_name() is still deprecated for removal before 8.0.0.

Committed 5a508f5 and pushed to 8.0.x. Thanks!

This came up at #3042745-19: Remove group @legacy from jsonapi tests and fix deprecation messages.1

This does not have a published change record. I think https://www.drupal.org/node/2481845 was intended to be the change record for this commit?

Reopening this to clarify the change record situation.

I commented in your issue, a CR wouldn't have helped you since that usage of this function was never correct anyway, so a CR would not have helped you, a CR can only target those that are correctly using it, meaning, entity types.

We can add a CR in #2450793: Properly deprecate support for entity type label callbacks, when we actually deprecate I'd say.

when we actually deprecate I'd say.

It already is, isn't it?

  /**
   * Indicates if a label callback exists.
   *
   * @return bool
   *
   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   *   Use EntityInterface::label() for complex label generation as needed.
   *
   * @see \Drupal\Core\Entity\EntityInterface::label()
   * @see \Drupal\Core\Entity\EntityTypeInterface::getLabelCallback()
   * @see \Drupal\Core\Entity\EntityTypeInterface::setLabelCallback()
   */
  public function hasLabelCallback();

Quoting @Berdir from #3042745-21: Remove group @legacy from jsonapi tests and fix deprecation messages:

The deprecated label callback means only one thing. That entity types that do have a dynamic label need to override the label method instead of defining a callback.

So that method does have a @deprecated annotation, but it actually does not mean "this method is deprecated", it means "this method is deprecated for certain reasons for calling it"?

> It already is, isn't it?

Yeah. For me, deprecations that aren't enforced and core is still relying on are fake/pseudo deprecations, per our current rules, we can only deprecate something if stop relying on it in core and enforce it with @trigger_error(). I only consider something really deprecated after that.

The concept of having a label callback is deprecated and unnecessary, you can just implement label now(). That means also the method to access that callback is. And nothing except the default label() implementations of EntityBase and ContentEntityBase should have ever called it in the first place, that is the only proper use case for it.

I'm ++ on broader discussion cos "label" field used not consistently (label(), getName()/getWhatever() and magic getter)

As suggested, lets continue this discussion in the issue that's still open: [#https://www.drupal.org/project/drupal/issues/2450793]

#46: thanks for that extra context, I didn't realize that distinction until now.