Problem/Motivation

If I set a single-directory component slot to an empty array, I will get this error:

Drupal\Core\Render\Component\Exception\InvalidComponentDataException: Unable to render component "MODULE:COMPONENT". A render array or a scalar is expected for the slot "SLOT" 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).

Setting empty values to NULL also does not work. If a slot is to be empty, it has to be not set.

Proposed resolution

Allow empty array in slots. An empty Drupal render array should render to the empty string. The fix will require changing Drupal\Core\Render\Element::isRenderArray().

Remaining tasks

Agree on what to do.

User interface changes

None.

Introduced terminology

None.

API changes

Empty arrays will be allowed as the value for SDC slots.

Data model changes

None.

Release notes snippet

Issue fork drupal-3526176

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:

Comments

liam morland created an issue. See original summary.

sd9121’s picture

Assigned: Unassigned » sd9121
sd9121’s picture

Assigned: sd9121 » Unassigned
idiaz.roncero’s picture

Agree that this is a bug. Especially if you consider that using an empty array using components directly on twig, you get no error at all: this only happens when using the RenderElement on your code.

There should be no difference on the accepted input values between the two ways to "invoke" a component.

idiaz.roncero’s picture

Status: Active » Needs review

Created a simple MR that allows empty arrays to bypass the negative check from Element::isRenderArray.

This is a solution, but it might be controversial and is no small change as it assumes that,contrary to what Drupal usually does, an empty array will be considered a valid render array in this particular context.

A different solution could be the opposite: making the twig embed and includes of components apply the same logic and disallow passing empty arrays as slot values (right now, they swallow it)

What we have now - different behavior depending on the way a component is called - is misleading. We need to chose one direction or the other.

(I am obviously not proposing here to let developers pass empty arrays voluntarily, but you can end up with an empty array as an unintended result of some business logic, both on twig and on PHP code, and we need to decide if this is something we want to support or not).

liam morland’s picture

Issue summary: View changes

Fix typo.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs subsystem maintainer review, +Needs tests
johnalbin’s picture

This is a solution, but it might be controversial and is no small change as it assumes that,contrary to what Drupal usually does, an empty array will be considered a valid render array in this particular context.

Yes, but, crucially, your MR isn't changing the logic of Element::isRenderArray so the definition of render array remains the same.

ComponentElement generates a Twig template on the fly and the MR modifies how/if the template is generated by checking if the contents of a slot are a render element and then creating a twig block containing the slot variable. IMO, I'd consider that a bug, because a twig block can contain any kind of content. Specifically, since ComponentElement is inserting a single variable (the slot's value) into a block, we should allow any variable value that Twig allows, not just render elements.

A different solution could be the opposite: making the twig embed and includes of components apply the same logic and disallow passing empty arrays as slot values (right now, they swallow it)

What we have now - different behavior depending on the way a component is called - is misleading. We need to chose one direction or the other.

Agreed. If we went the other way with the MR, then we would be limiting what is acceptable in Twig embed/includes; which would also be a breaking API change that would have to wait for 12.x. This MR expands what is acceptable for Single Directory Components and is not a breaking API change. And, as I said before, IMO, it's a bug fix. So this is the right way to go. 👍

The code looks good to my eyeballs, but as smustgrave pointed out, it needs tests.

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

oily’s picture

Test coverage.