Problem/Motivation

When viewing the page of an entity which doesn't have a label, the 

navigation:title</component> throws the following:

<code>
Drupal\Core\Render\Component\Exception\InvalidComponentDataException: Unable to render component "navigation:title". A render array or a scalar is expected for the slot "content" when using the render element with the "#slots" property in Drupal\Core\Render\Element\ComponentElement->generateComponentTemplate() (line 118 of core/lib/Drupal/Core/Render/Element/ComponentElement.php).

This comes from PageContext which has:

    $build += [
      [
        '#type' => 'component',
        '#component' => 'navigation:title',
        '#props' => [
          'icon' => 'database',
          'html_tag' => 'span',
          'modifiers' => ['ellipsis', 'xs'],
          'extra_classes' => ['top-bar__title'],
        ],
        '#slots' => [
          'content' => $entity->label(),
        ],
      ],
    ];

This doesn't account for the fact that EntityInterface::label can legitimately return NULL:

  /**
   * Gets the label of the entity.
   *
   * @return string|\Drupal\Core\StringTranslation\TranslatableMarkup|null
   *   The label of the entity, or NULL if there is no label defined.
   */
  public function label();

Steps to reproduce

View an entity without a label entity key/custom label() implementation.

Proposed resolution

Not sure if a slot can validly be defined to allow NULLs, or whether PageContext should perform some checks before attempting to render the component.

Remaining tasks

User interface changes

N/A

Introduced terminology

N/A

API changes

Data model changes

N/A

Release notes snippet

CommentFileSizeAuthor
#12 drupal_3505182-12.patch1.66 KBtars16
#6 drupal_3505182-6.patch1.26 KBviappidu

Issue fork drupal-3505182

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andrewbelcher created an issue. See original summary.

quietone’s picture

quietone commented
Version: 11.1.x-dev » 11.x-dev
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Component: single-directory components » navigation.module

Just cross-referencing things, I did run into the same error, but for me, #3504477: Allow all Renderable values in SDC slots helps because it's a TranslatableMarkup object, not null.

I assume SDC has optional slots, but I think this is a navigation issue. I think an label should either fall back to some fallback string like "- No label -" or possibly skip the whole plugin as it can't really provide meaningful information?

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue tags: +Needs tests

Good find. Tagging as Needs test, it's not obvious how to reproduce it. I was able to reproduce it in a very hacky way by adding the following implementation of label() to \Drupal\node\Entity\Node, just to illustrate the issue:

/**
   * {@inheritdoc}
   */
  public function label() {
    return NULL;
  }

IMO, the plugin should be skipped if there isn't a useful title or badge. We skip the badge, avoiding this issue if there isn't one. We should do the same for the label.

m4olivei opened merge request !11390

viappidu’s picture

viappidu commented
StatusFileSize
new drupal_3505182-6.patch1.26 KB

Fork is a little bit behind...

This patch applies to 11.2.0 (tested)

xmacinfo’s picture

xmacinfo
he/him
Primary language French
Location Canada
commented

The Commerce Invoice module encounters this bug.

abhijith s made their first commit to this issue’s fork.

abhijith s opened merge request !12583

xmacinfo’s picture

xmacinfo
he/him
Primary language French
Location Canada
commented
Priority: Normal » Major

Moving to major since this issue essentially blocks displaying content when en entity without label is used.

ivan zugec’s picture

ivan zugec commented

Getting the same error when I view the logs using the AI logging sub-module with Drupal CMS.

Drupal: 11.2.2

tars16’s picture

tars16 commented
StatusFileSize
new drupal_3505182-12.patch1.66 KB

We are running into this issue when trying to translate content. The updated check for the label didn't solve our issue, however, casting $label to a string got us past our issue. Not sure if this introduces additional translation issues, but so far it has been working.

          '#slots' => [
            'content' => Element::isRenderArray($label) ? $label : (string) $label,
          ],

Tested with 11.2

plopesc made their first commit to this issue’s fork.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Added new iteration to latest MR including code from #12

tijsdeboeck’s picture

tijsdeboeck
Location Antwerp 🇧🇪 🇪🇺 🌎
commented

@plopsec, your MR works perfectly and solved the issue for me.

oily made their first commit to this issue’s fork.

oily’s picture

oily commented

Fixing PHPCS in MR !12583 so can run test-only test since the branch contains test coverage.

Need then to choose one branch, merge good code from the other branch and hide it.

oily’s picture

oily commented

I wonder if the code 

    $entity_label = $entity->label();
    if (!$entity_label) {
      return $build;
    }

at lines 89 to 93 of PageContext.php in MR !11390 is the best version to date?

Also

        'content' => Element::isRenderArray($entity_label) ? $entity_label : (string) $entity_label,

at lines 104 to 105?

fjgarlin’s picture

fjgarlin commented

Disregard comment. Fixing indexing issue.

oily’s picture

oily commented

Both branches now have test coverage.

This one: https://git.drupalcode.org/issue/drupal-3505182/-/tree/3505182-11x-with-... by @Abhijith S includes a full on test entity as in custom content entity. But I cannot trigger the 'test-only' test on it. It tells me I do not have the permissions. Any chance you could trigger it @Abhijith S? And paste the output in a comment?

I tried have added test coverage to the other branch: https://git.drupalcode.org/issue/drupal-3505182/-/tree/3505182-an-entity... using an article node, not a custom content entity, by adding a few lines to an existing text.

But the test is passing. The page is loaded with 200 so no error. And the error text reported in the IS is not appearing on the page inside the test. So again the assertion is passing when it should fail.

Can anyone see why my test is not working? Does an article node not count as an 'entity' in this issue? Does it have to be a 'proper' custom content entity? If so, then the branches could be merged? And see if the test coverage by @Abhijith S does its job?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work

The node title is the entity label. You have a label in your test. The title for nodes is required, you can't save a node without one, that's why a test entity type is needed for this.

There doesn't seem to be existing entity in entity_test module that works for this, so adding a new test type is the correct thing to do here.

oily’s picture

oily commented

Re: #21 Okay, makes sense. Nice that the entity code has been done and is ready to merge in.

oily changed the visibility of the branch 3505182-11x-with-testcase to hidden.

oily’s picture

oily commented

I have added the entity test code from one branch to the other and hidden the branch I copied the test entity code from. There is work to do on the tests.

oily’s picture

oily commented

Coverted the test entity annotation to an attribute. The pipeline shows broken tests but the output is useful, now. The test entity may need some tweaks but hopefully test coverage is close.

oily’s picture

oily commented

There is a failing assertion in relation to the test entity and the navigation top bar. The assertion fails due to a 403 reponse code instead of 200. Any ideas how to deal with it? Put to Needs review, yet?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

You need a user that is allowed to access that entity. There's an admin permission defined on that test entity.

oily’s picture

oily commented
Issue tags: -Needs tests
oily’s picture

oily commented

Re #27 Thank you @berdir. I think I have that right now. That was a lot of 'tweaking' of the test entity. There is an odd error message breaking the test now, the same test as was breaking due to 403. I am not sure if I have fixed the 403. This error may be concealing the 403 one?

oily changed the visibility of the branch 3505182-an-entity-without to hidden.

oily changed the visibility of the branch 3505182-an-entity-without to active.

oily’s picture

oily commented

This is the test-only output now:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.13
Configuration: /builds/issue/drupal-3505182/core/phpunit.xml.dist
E                                                                   1 / 1 (100%)
Time: 00:05.160, Memory: 8.00 MB
There was 1 error:
1) Drupal\Tests\navigation\Functional\NavigationTitleMissingLabelTest::testNavigationTitlePluginHandlesMissingLabel
TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /builds/issue/drupal-3505182/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238
/builds/issue/drupal-3505182/core/lib/Drupal/Component/Utility/Html.php:433
/builds/issue/drupal-3505182/core/lib/Drupal/Component/Render/FormattableMarkup.php:238
/builds/issue/drupal-3505182/core/lib/Drupal/Component/Render/FormattableMarkup.php:187
/builds/issue/drupal-3505182/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php:195
/builds/issue/drupal-3505182/core/lib/Drupal/Component/Utility/ToStringTrait.php:15
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Entity/EntityType.php:719
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Entity/ContentEntityBase.php:1444
/builds/issue/drupal-3505182/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php:78
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Entity/EntityFieldManager.php:235
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Entity/EntityFieldManager.php:197
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Entity/EntityFieldManager.php:459
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Extension/ModuleInstaller.php:375
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Extension/ModuleInstaller.php:229
/builds/issue/drupal-3505182/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/builds/issue/drupal-3505182/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:511
/builds/issue/drupal-3505182/core/tests/Drupal/Tests/BrowserTestBase.php:537
/builds/issue/drupal-3505182/core/tests/Drupal/Tests/BrowserTestBase.php:343
/builds/issue/drupal-3505182/core/modules/navigation/tests/src/Functional/NavigationTitleMissingLabelTest.php:36
ERRORS!
Tests: 1, Assertions: 0, Errors: 1, PHPUnit Deprecations: 2.

Since the test is breaking due to null value I am wondering if #3 where a fallback value is substituted for null might be best?

plopesc opened merge request !13931

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Created a new MR https://git.drupalcode.org/project/drupal/-/merge_requests/13931.

This MR use Unit Tests to double check the different possible combinations for the navigation:title and navigation:badge components.

smustgrave closed merge request !11390

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

I don't think we need to test render arrays here? \Drupal\Core\Entity\EntityInterface::label() is documented as "string|\Drupal\Core\StringTranslation\TranslatableMarkup|null", an array would break so many existing places.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Removed support for render arrays. They're ignored now.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Thanks.

I would have probably written this as a kernel test, the unit test involves a lot of mocking that in my experience can be tedious to maintain as it's tightly coupled against the implementation, iff we for example change stuff around how the content_moderation integration. But it's there, we can still decide to rewrite this in the future if that's less work than updating it.

  • catch committed a01dfd32 on 11.3.x 
    fix: #3505182 An entity without a label causes an uncaught exception for...

  • catch committed 2c691e8e on 11.x 
    fix: #3505182 An entity without a label causes an uncaught exception for...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !13931

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.