\Drupal\Core\Entity\EntityInterface::label() can return a NULL

Here's all the label related change from #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves). I'm not entirely sure about the scope of this and how best to do it and whether or not we want tests. Once we support PHP 8.1 we get implicit test coverage via the PHP deprecations.

This patch was generated by doing git checkout 3220021-php81-compatibility (git diff 9.3.x 3220021-php81-compatibility -G label --name-only) and the removing the small about of unrelated change.

@longwave thanks for the review, sorry for taking a while to get back to this one.

Re #3.1 I agree that we should use the ID too... but I'm pondering if we should have a generic rule for entities that, if there is no label, then we return "content item 1" or whatever the label singular + entity ID would be. OTOH that would make more difficult to determine that a label is not set. Not sure.

Re #3.2 I think this is a good change because we're testing the unlabelled entity ID fallback :)

Re #3.3 I wonder if there possibly could be multiple layouts on the page all with Edit layout pointing to different section storages. I guess this is why I think we need something consistent to deal with an unlabelled entity.

@berdir pointed out the existence of #2939931: Add an implementation of EntityDisplayBase::label() which is definitely related.

@longwave re #3.2 this is a TestBase class - and in my mind it needs to deal with whatever the Content Translation UI outputs. This patch changes that so it needs to change so support both types of label.

Here's a patch with the @todo's removed.

I'm really not sure that the no label behaviour in DefaultSelection is correct but it is extensively tested in \Drupal\Tests\system\Kernel\Entity\EntityReferenceSelectionReferenceableTest so I don't think we should be changing that behaviour here. It opens up quite a few cans of worms about what to do with the match operator if the label can be an entity ID.

1. +++ b/core/modules/contact/contact.module
   @@ -123,7 +123,7 @@ function contact_mail($key, &$message, $params) {
   -    '@form' => !empty($params['contact_form']) ? $params['contact_form']->label() : NULL,
   +    '@form' => !empty($params['contact_form']) ? $params['contact_form']->label() : '',
   

   I think that label is required for Contact Forms. it is in the UI... in fact this bit here is about what to do when there is no contact form - ie. it's the user contact page. So this bit looks fine. NULL is just a not default value for a string replacement value.

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -25,7 +25,7 @@ class LayoutBuilderController {
   -    return $this->t('Edit layout for %label', ['%label' => $section_storage->label()]);
   +    return $this->t('Edit layout for %label', ['%label' => $section_storage->label() ?? $section_storage->getStorageType() . ' ' . $section_storage->getStorageId()]);
   

   Let's work out why this is happening.

3. +++ b/core/modules/user/user.module
   @@ -852,7 +852,7 @@ function user_user_role_insert(RoleInterface $role) {
   -      'label' => t('Add the @label role to the selected user(s)', ['@label' => $role->label()]),
   +      'label' => t('Add the @label role to the selected user(s)', ['@label' => $role->label() ?? $role->id()]),
          'configuration' => [
            'rid' => $role->id(),
          ],
   @@ -865,7 +865,7 @@ function user_user_role_insert(RoleInterface $role) {
   
   @@ -865,7 +865,7 @@ function user_user_role_insert(RoleInterface $role) {
        $action = Action::create([
          'id' => $remove_id,
          'type' => 'user',
   -      'label' => t('Remove the @label role from the selected user(s)', ['@label' => $role->label()]),
   +      'label' => t('Remove the @label role from the selected user(s)', ['@label' => $role->label() ?? $role->id()]),
   

   I don't like this change - pretty sure it is due to test roles with no labels. Let's fix that.

4. +++ b/core/modules/workspaces/src/WorkspaceRepository.php
   @@ -62,7 +62,7 @@ public function loadTree() {
   -        return strnatcasecmp($a->label(), $b->label());
   +        return strnatcasecmp($a->label() ?? $a->id(), $b->label() ?? $a->id());
   

   I don't like this change - label is required for workflows - pretty sure this is due to tests.

I've added some exceptions so we can break things to find out where the NULLs are coming from.

I still think that generic entity code has to support NULL labels as that is allowed by the interface but on entities where the label is required we should be making sure it is there.

Fixing the tests... leaving the exceptions in until we're green.

Fixing the layout one too...

@andypost that's a good idea. These exceptions are here just for testing and I was planning to remove them but I think once I've got this green I'll change them to asserts and add tets.

We're green - changed everything to assertions. I thought about adding tests but I'm not sure it is worth it. The advantage of adding assertions is that tests will fail even on PHP versions less than 8.1.

I disagree that a change record is needed here. There is no real change here. Code like user_user_role_insert() always required a label to work as expected and in regular runtime you can't create a role without a label via the UI.

Thanks for the review @Berdir

Re #20.1 I experimented with that in #7... we have extensive test coverage of this in \Drupal\Tests\system\Kernel\Entity\EntityReferenceSelectionReferenceableTest - I think if we want to change that we should do that in a follow-up.

Re #20.2 and .3 sure we can do that. It didn't seem to matter - you've always got the ID. Part of me is tempted to change the config behaviour and if there is no label use the ID instead. But that's a much larger issue and more change. Will use the ID.

[EDIT: fixed where we have extensive test coverage... copy pasta error - thanks @Berdir]

Less randomness....

Fixing the assertions and making the docs for label() correct with the reality.

@Berdir checkout #7 - that patch did...

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -352,8 +352,7 @@ public function getReferenceableEntities($match = NULL, $match_operator = 'CONTA
     $entities = $this->entityTypeManager->getStorage($target_type)->loadMultiple($result);
     foreach ($entities as $entity_id => $entity) {
       $bundle = $entity->bundle();
-      // @todo or should we use the ID here?
-      $options[$bundle][$entity_id] = Html::escape($this->entityRepository->getTranslationFromContext($entity)->label() ?? '');
+      $options[$bundle][$entity_id] = Html::escape($this->entityRepository->getTranslationFromContext($entity)->label() ?? $entity_id);
     }

The fails are in \Drupal\Tests\system\Kernel\Entity\EntityReferenceSelectionReferenceableTest - the link in #21 was a copy paste error that I'll fix...

Whoops.

Thanks for the review @longwave

RE #30.1 the problem with changing this to sort by ID when the label is missing is that this is a change in behaviour and then you are sorting the proverbial apples and pears. That's why I went for the no change to HEAD fix.

RE #30.2 well asserts should be disabled in production so the number of times is immaterial. The reason I didn't fallback to ID is the same as above - we'd then be sorting apples and pears.

I think it is best to rely on PHP 8's stable sorting for when the labels are equivalent. See https://wiki.php.net/rfc/stable_sorting - in practical terms this means the ID is used for sorting because the most common db (mysql) tends to return rows in the order of ID.