Entities without labels cause TypeError in EntityController title callbacks

Setting it to need review for test bot and review.

> return $this->t('Edit %label', ['%label' => $entity->label() ?? '']);

That'll produce a stray space - 'Edit ' instead of 'Edit'.

That should probably just be:

$entity->label() ?

because that way it also covers the case where label() returns an empty string.

I agree with @joachim, if empty we should return "Edit" and not "Edit " (extra space).
This needs tests.

Here's a re-rolled patch (11.1.x.)

Still needs tests, the remark in #11 seems to be fixed.

I converted the patch from #13 to an MR and added a Unit test. Then I realized that EntityController::deleteTitle() has the exact same problem. So I expanded the scope to include it.

It's a bit odd. ::deleteTitle() returns an empty string if the entity can't be loaded. Whereas ::editTitle() returns nothing (a NULL value). My new test does not cover these cases, partly because it would require different mocking and partly because I feel like making the two callbacks behave the same maybe ought to be handled in a follow-up issue. In which case the NULL entity test cases can be written at that time. Not my call though and that's WAY out of scope for this bug fix.

Hope this can be addressed in the related issue. See comment #3504477-20: Allow all Renderable values in SDC slots.

UPS: I suppose this is the wrong issue.

Good catch on the other callbacks!

And nice job making the test a unit test.

But I'm really not keen on the mixing of if/else and ternary that there is now.

We could maybe use the nullsafe operator like this (pseudocode):

$entity = getEntity()
$label = $entity?->label();

// Then we are back in the case where label is either a label or NULL.

But I'm really not keen on the mixing of if/else and ternary that there is now.

Yeah, it was kind of ugly. Unfortunately, the nullsafe operator doesn't help here because the functions don't return values based only on the label. They also return something in the case of NULL entities. Still, I didn't try to clean up the callbacks before, so I did it now. I'm not sure that I like this much either, but at least it should be easy to understand.

> Unfortunately, the nullsafe operator doesn't help here because the functions don't return values based only on the label. They also return something in the case of NULL entities.

Sorry, I don't follow.

We start with:

$entity = $this->doGetEntity($route_match, $_entity)

We have either an EntityInterface, or NULL, according to the docs for that method.

If it's NULL, then $entity?->label() is NULL.
If it's not NULL, then $entity?->label() returns the label or NULL according to its docs.

So in all cases, $entity?->label() is NULL or the label.

So we can do:

$label = $entity?->label();
if ($label) {
// This also covers for the wacky case where the entity label() returns an empty string.
}
else {
}

Currently, if the $entity is NULL then editTitle() returns NULL and deleteTitle() returns an empty string. This is separate from what happens if label() returns a value or NULL. If we want to maintain the current behavior of the functions, then we have to handle the case of a missing entity.

Ah right, thanks for explaining.

When editTitle() returns NULL it'll cause the same TypeError when it's output, so that's problematic too. We should always be returning a string, an empty one at worst.

It's going to be a really rare pathway though - you'd need to be on a route that has say node/42/edit as a parent path, and node 42 doesn't exist, so that the path breadcrumb builder tries to get the page title for node/42/edit.

I get a similar issue with deleted entities that had no labels, it is a kind of mix of the issue described here and #3554974: FormattableMarkup: NULL @ and % placeholder values should log a notice and use empty string. EntityDeleteFormTrait and DeleteMultipleForm use label() method while logging.

So I'm thinking, would it be better to return an ID from the default label() method (in ContentEntityBase or even in EntityBase class)? EntityDeleteFormTrait already uses IDs in messages and strings presented to the user (but not in the log message for some reason):

    return $this->t('Are you sure you want to delete the @entity-type %label?', [
      '@entity-type' => $this->getEntity()->getEntityType()->getSingularLabel(),
      '%label' => $this->getEntity()->label() ?? $this->getEntity()->id(),
    ]);

I'd only prepend an ID with the # character, but this is a personal preference, also an ID can be non-numeric.

Echoing the sentiment about the unit tests, pasting the test-only run here too for others https://git.drupalcode.org/issue/drupal-3319212/-/jobs/8306385

From what I can tell there are no open threads and the changes make sense to me. Don't mind marking for you.

@pingwin4eg I think that is worth exploring in a followup - should all entities have (non empty) labels by default?

In the meantime let's solve this immediate issue by committing this. Doesn't backport cleanly to 10.6, so this is going in 11.3 and up.