Provide workaround to validation error on SDC optional properties with null values

If this problem was discovered on a version of Drupal that is not 11.x, add that information in the issue summary and leave the version at 11.x. In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Also mentioned on the version section of the list of issue fields documentation.

Thanks for reporting this issue. While the patch adds null as a valid type for top-level non-required properties, those properties may themselves have child properties that are not yet handled.

Instead of adding null as a type, can we instead skip validation for properties that are both optional and null?

Works fine with Drupal 10

@lauriii explained this issue to me on Zoom. Basically: optional properties (anything not explicitly defined as required) should be allowed to default to null.

I don't know what the correct fix is, and I have no opinion on that. But you're going to need test coverage of whatever that fix is, so self-assigning to write a test.

I want to add that I run into this issue all the time, and also fix it by adding 'null' as a type.

   // Add null type to all non-required properties.

I didn't look in details yet but I have the feeling there may be something wrong with this proposal. There may be a root cause to address instead.

If the property is optional, why putting a NULL value instead of an empty value?

    $build = [
      '#type' => 'component',
      '#component' => 'module:component_name',
      '#props' => [
        'text' => NULL,
      ]
    ];

    $build = [
      '#type' => 'component',
      '#component' => 'module:component_name',
      '#props' => []
    ];

Let's try to not alter the prop definitions too much. We have already a similar hack to get rid off:

        // All props should also support "object" this allows deferring
        // rendering in Twig to the render pipeline.
        $type = $prop_schema['type'] ?? '';
        $schema['properties'][$name]['type'] = array_unique([
          ...(array) $type,
          'object',
        ]);

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

This isn't really all that relevant when using SDC in render array but when using SDCs within Twig. Twig doesn't have the concept of undefined unless you use Twig strict_variables turned on (which Drupal is not doing). So when you are passing in undefined properties, they become null. I don't think there's a practical way to keep the variables as undefined and really as a frontend developer, you shouldn't have to care.

An example of this would be this card component. The URL is optional and when it's not defined, it becomes null for heading which triggers the validation error.

This isn't really all that relevant when using SDC in render array but when using SDCs within Twig.

thanks fo the additional context. I will do some tests early next week.

Fresh Drupal 11.2.4 install, standard profile. Only changes:

• composer require justinrainbow/json-schema
• $settings['extension_discovery_scan_tests'] = TRUE; in settings.php

Calling sdc_theme_test:my-card component from this issue's MR, which has three optional props:

    slogan:
      type: string
      title: Slogan
    author_and_time:
      type: object
      properties:
        author:
          type: string
          title: Author
        time:
          type: string
          title: Time
      required:
        - time
    classes:
      type: array
      title: Optional extra CSS classes.
      items:
        type: string

I haven't noticed anything wrong for now: https://github.com/pdureau/oviedo/blob/master/templates/page.html.twig

I will do a second batch of tests later (to be sure JSON Schema validator is effectively triggered).

You would run into the problem if you provided slogan as an optional property of the page which passes it to the card from the page. This would not trigger validation errors when slogan is provided but would trigger the validation error when page is not receiving slogan.

slogan as an optional property of the page

If page has properties, I understand there are 2 components here: card and page.

So, I will do my next test with:

• https://github.com/pdureau/oviedo/blob/master/templates/page.html.twig including oviedo:page component
• a new oviedo:page component (an exact copy of sdc_theme_test:my-card from this issue's MR) including sdc_theme_test:my-card component (many times): https://github.com/pdureau/oviedo/blob/master/components/page/page.twig

Let's push results of #15

The test failure is because we are not recursing any object that we could have (and it could be another object embedding another object, etc).

This is not a bug in SDC but in the component, prop = NULL is not the same than omitting a prop. which wouldn't trigger any error. But I agree this is terrible DX when using components inside other components because of twig, and should be solved.

Being unable to reproduce the issue and, to be honest, even to fully understand it , I still believe we are not addressing the root cause here but doing a workaround.

However, I guess I am OK for it anyway because:

• @lauriii assured me we are not fixing in Core something which must be fixed at Canvas level
• we are doing the alteration in ComponentValidator instead of ComponentMetadata so it seems we are not messing with the real component schema and we are not doing something like #3554720: Remove addition of a object type in all props

This no longer affects Mercury so removing the tag although I understand this is an overall DX improvement so may be a good idea anyway.

I was pointed here by @penyaskito from #3555413-32: Allow linking to referenced entities: add `url` property to `EntityReferenceItem::propertyDefinitions()`, after I ran into the very same problem over there (see #3541361-27: Find optional field instance matches for `type: object` props (images + videos), including for optional fields on bundleless entity types (e.g. `User`'s `user_picture`) for details).

I was going to fix this as part of #3541361: Find optional field instance matches for `type: object` props (images + videos), including for optional fields on bundleless entity types (e.g. `User`'s `user_picture`), since it's already doing something related.

I'm not convinced this change is necessary — it can be left up to the caller to just not pass NULL values. And in Canvas, we could quite easily achieve that:

 ...GeneratedFieldExplicitInputUxComponentSourceBase.php | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/Plugin/Canvas/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php b/src/Plugin/Canvas/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
index 82a7907d3..917642dda 100644
--- a/src/Plugin/Canvas/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
+++ b/src/Plugin/Canvas/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
@@ -368,6 +368,23 @@ abstract class GeneratedFieldExplicitInputUxComponentSourceBase extends Componen
   public function hydrateComponent(array $explicit_input, array $slot_definitions): array {
     $hydrated[self::EXPLICIT_INPUT_NAME] = $explicit_input['resolved'];
 
+    // Omit optional props whose value evaluated to NULL. Otherwise, an SDC
+    // validation error is triggered.
+    // @see \Drupal\Core\Theme\Component\ComponentValidator::validateProps()
+    $prop_field_definitions = $this->configuration['prop_field_definitions'];
+    foreach ($hydrated[self::EXPLICIT_INPUT_NAME] as $prop => $resolved_value) {
+      // The stored inputs SHOULD match the live schema, but mid-development or
+      // due to a botched release, that is impossible to guarantee.
+      // @see https://en.wikipedia.org/wiki/Robustness_principle
+      if (!array_key_exists($prop, $prop_field_definitions)) {
+        continue;
+      }
+      $is_required = $prop_field_definitions[$prop]['required'];
+      if (!$is_required && $resolved_value === NULL) {
+        unset($hydrated[self::EXPLICIT_INPUT_NAME]);
+      }
+    }
+
     if (!empty($slot_definitions)) {
       // Use the first example defined in SDC metadata, if it exists. Otherwise,
       // fall back to `"#plain_text => ''`, which is accepted by SDC's rendering

it can be left up to the caller to just not pass NULL values.

It can... but it's not always easy.

We frequently call components from templates where props may or may not be populated. If they're not populated, a null is passed, which makes us have to add"null" as a type (which is bad DX).

I feel that the the DX wins here far outweigh any negatives (which TBH, i don't see any negatives)

+1 to this.

I also think that leaving out to the caller (the developer) not to pass NULL values leads us to constructs that are ugly and cumbersome.

Some real, recent examples for a paragraph that uses Paragraph Behaviors to feed some settings that map almost 1:1 to SDC props.
We basically need to repeat an if/merge block for each prop that is not required in order to avoid NULL values from provoking a WSOD. They are a lot, so our twig code became an ugly mess:

{% set banner_props = {
      text_size: text_size|default('medium'),
      text_container_width: ext_container_width|default(100),
} %}

  {% if behavior.alignment %}
    {% set banner_props =
      banner_props|merge({
        text_container_alignment: behavior.alignment
      })
    %}
  {% endif %}

  {% if behavior.background_opacity %}
    {% set banner_props =
      banner_props|merge({
        overlay_opacity: behavior.background_opacity
      })
    %}
  {% endif %}

  etc, etc...

Yes, this could have been cleaner on PHP, but not so much.

If you add this to the old problem of false positives on render arrays that only carry cache metadata, something as simple as mapping content.field_whatever or myvar on twig and expecting it to be ignored when null / empty... is becoming excessively complex.

I strongly feel we need to reduce the noise for the sake of DX (in general), and allowing null on optional props would clearly help, even if it is not 100% correct (as we will be basically altering the schema contract under the hood, as I understood).

We just updated locally to 11.4 from 10.6 to test and many, many pages WSOD due to this error.

Even if it throws an error I don't think having the type wrong on a prop should escalate to a WSOD.

Core already adds 'object' to the type of every prop, in ComponentMetadata::parseSchemaInfo(). We can add 'null' the same way, for props that aren't required.

About the white screens: this check only runs inside assert(). If you get it on production, assertions are on there, which is a different problem. Set zend.assertions=-1 and it's gone. In dev it does break the whole page, because the exception is thrown while Twig is rendering. We could catch it in doValidateProps() and just log it instead, and keep it strict for core's own tests. Maybe as a follow-up issue.

Until then, the workaround per prop is type: [string, 'null'], or |default('') where you use it.

Core already adds 'object' to the type of every prop, in ComponentMetadata::parseSchemaInfo().

Not anymore: #3554720: Remove addition of a object type in all props

We can add 'null' the same way, for props that aren't required.

Let's try to not mess too much with JSON schema of prop definitions. Especially in ComponentMetadata.

As said in comment #21, if a fix is needed, it can be done in ComponentValidator instead.

@idiaz.roncero

Altering mapping (associative arrays) in Twig is not a casual operations. It is complicated with or without SDC.

What was wrong with a classic props management?

{{ include('my_theme:my_banner', {
      text_size: text_size|default('medium'),
      text_container_width: ext_container_width|default(100),
      text_container_alignment: behavior.alignment,
      overlay_opacity: behavior.background_opacity,
  }, with_context=false) }}

What was wrong with a classic props management?

They are optional, them missing should be expected, needing to add a default every call means it's not optional. It's just required with extra steps.

It also doesn't tell you where it was included from, and a WSOD is an extreme response to a mistyped prop.

@pdureau

This line

  text_container_alignment: behavior.alignment,

Will fail when behavior.alignment evaluates to null (because it has no value, because its value is null/undefined on purpose, whatever) unless you either add "null" as an explicit type.

It is common that optional values could and should be present but null on twig. Fields come to mind as a good example.

Yep, there are many ways to fix it before it reaches SDC; but I also feel that accepting null for optional values is a better DX than having to manually handle the inclusion or not of some variables to circumvent this.

We validate our props against JSON Schema. Per the JSON Schema documentation on required properties:

In JSON a property with value null is not equivalent to the property not being present.

See also case 6 in the JSON Schema tour of required properties, which shows that a null value for an optional property that doesn't accept null values is invalid.

Therefore, there isn't a bug here. Instead, the validation error given in the issue summary is appropriate and correct: if a value is present, it must be of the allowed types. The ideal fix is of the sort @wim leers suggested in #23: don't pass NULL values from the caller.

Before we close this as won't fix, though, it's worth asking: is there anything we could do to address the DX issue while maintaining a distinction between optional properties and null values?

I suppose we might provide a utility method that, given a data structure representing a component, tests the values against the component's schema and removes any properties for which the following are all true:

1. The property is optional.
2. A null value is provided.
3. null is not a type defined in the property's schema.

Developers could, optionally, invoke that method on data before passing the result to rendering.

Therefore, switching this from a bug report to a feature request and updating the summary accordingly.

DX issue

I think this is the primary thing and there are multiple aspects.
1. There is a WSOD if there is a data discrepancy.
2. Something should strip these parameters if they are null, having to handle null on every single optional parameter is a terrible DX.
3. I would expect some type juggling to happen, IIRC it's complaining that 1 is a string instead of boolean in some cases and it's boolean instead of an object in others.