Allow all Renderable values in SDC slots

Thank you for opening.

Lets add some test coverage for the proposed change.

Removing tests tag as the test-only run shows the coverage

1) Drupal\KernelTests\Components\ComponentRenderTest::testRender
Drupal\Core\Render\Component\Exception\InvalidComponentDataException: Unable to render component "sdc_test:my-banner". A render array or a scalar is expected for the slot "banner_body" when using the render element with the "#slots" property

Summary seems clear with clear proposal that lines up with the code change.

Code change seems pretty straightforward and see no objection.

LGTM

Can confirm that this fixes an issue I see with navigation:title the entity returns a TranslatableMarkup object.

Left one comment on the MR whether this should be expanded to MarkupInterface or even Stringable.

I think we should use MarkupInterface here.

Updated, also shortened the test method description, it was already over 80 characters and that made it even longer.

1) Drupal\KernelTests\Components\ComponentRenderTest::testRender
Drupal\Core\Render\Component\Exception\InvalidComponentDataException: Unable to render component "sdc_test:my-banner". A render array or a scalar is expected for the slot "banner_body" when using the render element with the "#slots" property
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Element/ComponentElement.php:118
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Element/ComponentElement.php:65
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:107
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Renderer.php:830
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Renderer.php:387
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Renderer.php:459
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Renderer.php:203
/builds/issue/drupal-3504477/core/tests/Drupal/KernelTests/Components/ComponentKernelTestBase.php:95
/builds/issue/drupal-3504477/core/lib/Drupal/Core/Render/Renderer.php:593
/builds/issue/drupal-3504477/core/tests/Drupal/KernelTests/Components/ComponentKernelTestBase.php:95
/builds/issue/drupal-3504477/core/tests/Drupal/KernelTests/Components/ComponentRenderTest.php:313
/builds/issue/drupal-3504477/core/tests/Drupal/KernelTests/Components/ComponentRenderTest.php:45
ERRORS!

Shows the test coverage

Believe this would solve a problem I see on my patterns theme were I have to pass #markup => content, in the twig file.

Change makes sense.

I didn't find any problems with this and all questions are answered.

Trying to improve the title

Hello,

Thanks for the work.

The current (11.x) logic is:

      if (\is_scalar($slot_value)) {
        $slot_value = [
          "#plain_text" => (string) $slot_value,
        ];
      }

Obviously, it was not enough.

So, this MR is adding

      if ($slot_value instanceof MarkupInterface) {
        $slot_value = [
          "#markup" => $slot_value,
        ];
      }

On UI Patterns 2, they have added something a bit different:

    if (($value instanceof TwigMarkup) ||  ($value instanceof MarkupInterface)) {
      return ['#children' => $value];
    }
    if ($value instanceof \Stringable) {
      return [
        '#plain_text' => (string) $value,
      ];
    }

I don't know which one is better, maybe we a mix of both would be great. I will check with them.

I keep the issue assigned to me until I have an answer ;)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

At least updating the title to how understand the current direction and scope.

And, let's add the Core-blessed RenderableInterface to the list.
In a time where static analysis thrives, it trumps naked render arrays.

Will look into #15. But need to grok some renderer details about #children first.

Research note:
- There is no such class as TwigMarkup, but ui_patterns does use Twig\Markup as TwigMarkup;

Dunno if and how it hits Drupal code (did not encounter it yet), but no kittens killed putting it on our list here.

@pdureau #15:

    if ($value === NULL) {
      // NULL is an rarely used but legal return of many callbacks, like EntityInterface::label().
      // Addresses #3505182 and a whole family of to-be-expected similar issues.
      return [];
    }

    if ($value instanceof RenderableInterface {
      // Return the wrapped renderable (array) of the renderable (object).
      return $value->toRenderable();
    }

    if (($value instanceof \Twig\Markup) ||  ($value instanceof MarkupInterface)) {
      // Mark value as safe for rendering. 
      // Prefer #markup over the undocumented #children.
      return ['#markup' => $value];
    }

    if ($value instanceof \Stringable) {
      // Ensure that results of stringable are properly escaped.
      return ['#plain_text' => (string) $value];
    }

I'm needing this for my navigation breaking when an entity returns markup, so rolled it in code and attaching a snapshot for composer.

Rebased. Did not investigate why this fails though.

Rebased. Did not investigate why this fails though.

Thanks a lot @geek-merlin.

The last pipeline run failed though. I will rebase and run the tests again. Then review your proposal.

Same mysterious pipeline fail... investigating

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Think the bot was wrong on this one.

Actually that test failure seems relevant to the issue.