Problem/Motivation

SDC components have typed properties. They can either be scalar or implement a class.
However, the sdc module currently only allows class types on root properties, not on sub-properties of arrays.

This would be useful for templates like menu.html.twig.

Steps to reproduce

See this component for example:

$schema: https://git.drupalcode.org/project/drupal/-/raw/10.1.x/core/modules/sdc/src/metadata.schema.json
name: Main menu
description: Menu main
libraryOverrides:
  dependencies:
    - core/once
props:
  type: object
  properties:
    attributes:
      title: "Attributes"
      type: Drupal\Core\Template\Attribute
    foo:
      type: array
      items:
        type: object
        properties:
          attributes:
            title: "Attributes"
            type: Drupal\Core\Template\Attribute

The root attributes property works correctly, but the foo array attributes property triggers this InvalidComponentException:

[props.properties.foo.items.properties.attributes.type] Does not have a value in the enumeration ["array","boolean","integer","null","number","object","string"]/n[props.properties.foo.items.properties.attributes.type] String value found, but an array is required/n[props.properties.foo.items.properties.attributes.type] Failed to match at least one schema/n[props.properties.foo.items] Object value found, but an array is required/n[props.properties.foo.items] Failed to match at least one schema

Proposed resolution

ComponentValidator should call nullifyClassPropsSchema() on array properties too.

Issue fork drupal-3389588

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

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented
Issue summary: View changes
prudloff’s picture

prudloff commented

The MR adds support for the first level of array items, but it would probably be best to implement this recursively.

prudloff opened merge request !4871

prudloff’s picture

prudloff commented
Issue summary: View changes

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Related issues: +#3339201: Find a way to declare PHP object class as schema

I've been there before...

vidorado’s picture

vidorado commented

@geek-merlin was there and checked that it worked, but only for first level property types. For property items type, it doesn't work yet. I've applied the changes in MR4871 but it still doesn't work for me.

@prudloff, Could you review again your changes against the latest Drupal core (10.2.6 at the moment) to check if anything has changed in ComponentValidator::nullifyClassPropsSchema() that makes your changes don't work?

While I was fighting with it by myself, I changed the method ComponentValidator::getClassProps() to retrieve also the classes of item types. There is, maybe it could help.

  $types = is_string($type) ? [$type] : $type;

changed to

if (is_array($type) && in_array('array', $type) && ($itemsType = $prop_def['items']['type'] ?? NULL)) {
  $types = is_string($itemsType) ? [$itemsType] : $itemsType;
}
else {
  $types = is_string($type) ? [$type] : $type;
}

but that doesn't make it work either.

The code in this Validator is very hard for me to understand, it has a very strong array manipulation with reducers and the like...

vidorado’s picture

vidorado commented
Component: theme system » single directory components
vidorado’s picture

vidorado commented

For anyone also on this issue... I would like to share my experience after some playing with it :)

I realized that working with classes in SDC property definitions is not as practical as I first thought. The magic of SDC is in creating components "on the fly" in yor theme's twig files, and that is not possible if you have to provide a Class argument to a component.

If we would have a way of creating class objects from plain objects or arrays directly from twig (with a function, filter, whatever...) this could all be possible but, without that, I think is better to go along with simple types (string, boolean, integer...) and enums. We will have a little bit of repetition if we, for example, use the same enum in several places, but it is manageable.

If, like me, you also are lacking autocompletion when defining component properties, maybe a plugin for PHPStorm/VSCode could be developed, that was aware of SDC definitions.

prudloff’s picture

prudloff commented

I mostly agree with #10, component properties should be kept simple.
But I think attribute objects are a special use case.
It is possible to create them in Twig with the create_attribute() function and our front end developers are used to doing this.

prudloff’s picture

prudloff commented

I implemented this recursively but now some tests are failing and I'm not sure why. It looks like one of the components has an invalid definition but I don't see which one.
I can't reproduce if I run the tests locally.

vidorado’s picture

vidorado commented

It seems that props.properties wasn't being converted to stdClass in the $definition_object used to validate. It was being done in the original $schema var, but not in $definition_object.

All test failures were related to this, and now they pass :)

Feel free to set thiss issue to "Needs Review" if you consider that the work is now done.

prudloff’s picture

prudloff commented
Status: Active » Needs review

Thanks for the help @vidorado!

smustgrave’s picture

smustgrave commented
Issue tags: +Needs subsystem maintainer review
pdureau’s picture

pdureau commented
Status: Needs review » Needs work

I beleive we need to get rid of those PHP Namespace properties instead of extending their usage.

So, in my humble opinion, no need to add 100 lines of code for this, because:

  • We can consider them as a design mistake from the early days of SDC.
  • They are a "drupalism", non-compliant with JSON Schema standard.
  • SDC is doing weird stiff to skip JSON schema validation for them.
  • They create a coupling between UI components and CMS application.

So, it may be better to create an issue to raise a warning each time someone is using those.

prudloff’s picture

prudloff commented

Fair enough, I agree it adds complexity and the way we bypass JSON validation is not very clean.

For the specific use case of attributes, we would probably need to implement this workaround in our templates:
Outside component:

{# Convert attribute object to array because prop is now an array #}
{% set attributesArray = attributes.toArray() %}
{{ include('front:teaser', { attributes: attributesArray}) }}

Inside component:

{# Convert array back to object before rendering it #}
{% set attributesObject = create_attribute(attributes) %}
prudloff’s picture

prudloff commented
Status: Needs work » Closed (won't fix)

Looking at the code again, I agree it does not feel clean to do this.
I created an issue to deprecate class types: #3513441: Deprecate using a PHP class as SDC property type

prudloff closed merge request !4871