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`)

Sibling issue: #3548292: Find required field instance matches for image (`json-schema-definitions://canvas.module/image`) and video prop shapes. Improving title here.

Per #3548761-19: Can't link (pick a DynamicPropSource) to populate an optional explicit input if that DynamicPropSource evaluates to `NULL` (typically: field is empty), I think this just became more important.

As of #8 and commit https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com..., the match appears in the UI.

But upon selecting it, this happens:

The log entry:

Twig\Error\RuntimeError occurred during rendering of component d9904f4d-3b6d-43c2-8312-8360f26299d7 in Content template Article content items — Full content view (node.article.full): An exception has been thrown during the rendering of a template ("[canvas_test_sdc:image-optional-with-example-and-additional-prop/image.src] NULL value found, but a string is required. This may be because the property is empty instead of having data present. If possible fix the source data, use the |default() twig filter, or update the schema to allow multiple types.. [canvas_test_sdc:image-optional-with-example-and-additional-prop/image.alt] NULL value found, but a string is required. This may be because the property is empty instead of having data present. If possible fix the source data, use the |default() twig filter, or update the schema to allow multiple types.. [canvas_test_sdc:image-optional-with-example-and-additional-prop/image.width] NULL value found, but an integer is required. [canvas_test_sdc:image-optional-with-example-and-additional-prop/image.height] NULL value found, but an integer is required.") in "canvas_test_sdc:image-optional-with-example-and-additional-prop" at line 1.

The log entry in #9 showed that Evaluator was evaluating FieldObjectPropsExpressions for an optional prop in a way that it'd just get {key1: null, key2: null …}. That's neither meaningful nor helpful.

But the Evaluator did do its job correctly. The challenge is that SDC simply doesn't know how to handle that, which is also correct and reasonable. We could replace an optional prop that has an array of NULL values to just NULL, but that will trigger

Twig\Error\RuntimeError occurred during rendering of component d9904f4d-3b6d-43c2-8312-8360f26299d7 in Content template Article content items — Full content view (-): An exception has been thrown during the rendering of a template ("[canvas_test_sdc:image-optional-with-example-and-additional-prop/image] NULL value found, but an object is required.") in "canvas_test_sdc:image-optional-with-example-and-additional-prop" at line 1.

So instead, we need to make ::hydrateComponent() smart enough to cross-reference the resolved value against the expected prop shape, and simply omit it: https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com...

No more render crash:

Picked this up again, ~2 weeks after I worked on this at DrupalCon. Still need to write tests.

But first: figure out why "Authored by → User picture" doesn't show up in the UI on Nodes, when the test coverage proves that at least for Users, "Use picture" does show up.

#13: root cause found, described in #2169813-25: Support deriving fields from entity definitions with multiple bundles (or zero bundles). Fixed in https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com....

This is definitely a generic problem, so retitling.

Test coverage complete. Next up: updating the matching/suggesting logic to omit the suggested DynamicPropSources I flagged as "unwanted" in https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com...

@lauriii: please confirm my hunches of what I expect you'll say:

1. 'Authored by → User → Picture → Width' — https://git.drupalcode.org/project/canvas/-/merge_requests/242#note_615993
2. 'Silly image 🤡 → User ID → User → Picture — https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs#not...
3. Authored by but it actually points to a user picture, I think this needs to become Authored by → Picture — https://git.drupalcode.org/project/canvas/-/merge_requests/242#note_615995

Looks like I got the needed feedback 👍

Per #3555413-27: Allow linking to referenced entities: add `url` property to `EntityReferenceItem::propertyDefinitions()`, one more closely related edge case bug was identified.

#18: per https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com..., yes — that proves it for media images.

I'll expand the test coverage to verify that that is true for videos too 👍

Per @lauriii at https://git.drupalcode.org/project/canvas/-/merge_requests/242#note_618126, #3551339: Suggest only relevant DynamicPropSources should land first, because this is surfacing many new matches that are considered irrelevant.

#3551339: Suggest only relevant DynamicPropSources is in!

1. The last known bug is one that's existed for >1 year but was surfaced here. Extracted that into a separate issue: #3557612: `::matchEntityPropsForObject()` is too naïve: nonsensical `type: object` shape matches and useless labels.
2. The changes in this MR have grown more complex because another bug that was surfaced here lies in the SDC ComponentSource plugin, not in Shape matching. To keep the commit history understandable, I will split this into two MRs:
   1. One with only shape matching changes, and with test coverage — at this point the UI will be broken. See #10 and this MR comment.
   2. One with only component source changes, and with test coverage

Created https://git.drupalcode.org/project/canvas/-/merge_requests/319 for #22.2.2.

Note this closely relates to #3531905: Validation error on optional properties.: this "fixes" that problem in Canvas instead of in the SDC subsystem — but really, it's just Canvas now respecting the optionality of props.

I'll expand the test coverage to verify that that is true for videos too 👍

— in #19, in response to @lauriii's question in #18. This already works for required video fields (see https://git.drupalcode.org/project/canvas/-/merge_requests/242#note_623498), test coverage should be added to verify it works for optional fields.

Will do that. If it doesn't turn out to be working, I will extract that into a follow-up issue, because then it is most likely not related to the problem described in the issue title.

Turns out #25 does not work 🫠 Debugging led to a new core bug 😬

Quoting core's \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::propertyDefinitions()

      // We can add a constraint for the target entity type. The list of
      // referenceable bundles is a field setting, so the corresponding
      // constraint is added dynamically in ::getConstraints().
      ->addConstraint('EntityType', $settings['target_type']);

The commit that added that comment indeed added that. But then #2577963: Let entity_ref Selection handlers be in charge of the field validation removed it, in favor of validation happening in selection handlers, specifically in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::validateReferenceableEntities() — see https://git.drupalcode.org/project/drupal/-/commit/fbe0991b98a34be226b55...

This de facto means Typed Data incomplete; only when performing validation do we get to know which target bundles are actually allowed. That breaks scenarios like the one here. That means that all EntityReferenceItem objects' entity property are insufficiently constrained … probably due to #2169813: Support deriving fields from entity definitions with multiple bundles (or zero bundles)! 🫠😬 Because entity reference fields may target multiple bundles, but core's EntityDataDefinition is incapable of expressing that.

So this puts us in a catch-22:

• if the #2169813: Support deriving fields from entity definitions with multiple bundles (or zero bundles) core bug is fixed, we could also fix this bug
• without that, we risk breaking contrib/custom modules by swapping out the EntityDataDefinition exposed by the entity property on entity reference fields

Therefore the only choice remaining is to special-case EntityReferenceItem.

P.S.: in 2023, #3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" introduced \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::getReferenceableBundles(), which is related to all this: it is a work-around for #2577963: Let entity_ref Selection handlers be in charge of the field validation, but A) requires extra manual function calls and hardcoded logic rather than the correct metadata being present in Typed Data, but also B) prepares core for Dynamic Entity Reference support (i.e. >1 target entity type).

Failing test coverage for #18, which demonstrates in practice the problem outlined in #26: https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com...

Turns out I previously ran into this very challenge in July, specifically in #3524130: Define JSON Schema $refs for linking/embedding videos and linking documents's https://git.drupalcode.org/project/experience_builder/-/merge_requests/1..., where my write-up is describing the same problems as #22.
The analysis and solution there was simply incomplete: I fixed it then and there for multi-bundle entity reference fields, but failed to fix it for single-bundle entity reference fields 🫠

Fix: https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com...

👆 This entire set of work-arounds and the dozens of hours of debugging that led to them, could've been prevented if core had gotten this right a decade ago 😔 Because Drupal core's Typed Data descriptions are so inaccurate, Canvas has to introduce many work-arounds, and those work-arounds have to be careful not to tweak the actual Typed Data definitions in any way that could break core/contrib/custom code!
🤯 IOW: Canvas MUST keep Typed Data unchanged (to avoid BC breaks), and must convert/transform/upcast into the actually correct Typed Data (see: BetterEntityDataDefinition, ::getConstrainedTargetDefinition(), et cetera) to ensure correct matching. This will require a monumental effort to fix in core without breaking the ecosystem 🙈

Confirmed that optional video fields are now possible to match:

…
  "test_object_drupal_video": [
    {
      "id": "5e818fcf8e88c4b3",
      "source": {
        "sourceType": "dynamic",
        "expression": "ℹ︎␜entity:node:article␝field_ffffffffff␞␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜␜entity:file␝uri␞␟url,poster↝entity␜␜entity:media␝revision_user␞␟entity␜␜entity:user␝user_picture␞␟src_with_alternate_widths}"
      },
      "label": "ffffffffff"
    }
  ],
…

See the "ℹ️" comments on this very complex MR. 3 bugs are called out in headings. 2 of the 3 are core bugs 😅

The number of core bugs I’m running into is staggering … but on the other hand, it's understandable: Canvas is the first code to truly use the full potential of Typed Data. In a decade. So… makes sense.

The root cause of both core bugs is #2169813: Support deriving fields from entity definitions with multiple bundles (or zero bundles).

Given that nobody else knows enough about both core's Typed Data and Canvas' Shape matching … going ahead and merging this first MR. The test coverage proves it's working, the screenshots confirm it, and there's plenty of explanatory sprinkled to enable another person (one willing to learn enough about Typed Data) to understand what is happening, and what Canvas does to make things work as expected by end users.

Back to NW for https://git.drupalcode.org/project/canvas/-/merge_requests/319, to fix the component source bug this unveiled (see #9). Issue summary updated for clarity.

Done. Thoroughly manually tested.

Even if I missed something, the 2 MRs that this issue landed represent a huge leap forward.

🚢

See you in #3557612: `::matchEntityPropsForObject()` is too naïve: nonsensical `type: object` shape matches and useless labels tomorrow.

Is it perhaps a multiple-cardinality field?

I bet I know what's going on. I bet you're testing with an "media" reference field referencing multiple media types, and each of those being of a different media source: \Drupal\media\Plugin\media\Source\Image and \Drupal\acquia_dam\Plugin\media\acquia_dam\Image.

That today won't be found, because it requires the shape matching logic to:

1. traverse into all target bundles for a given reference field
2. observe different fields (typically: field_media_(image|video)_file + field_media_(image|video)_file_1) of the same field type
3. … or even more complex: of different field types (\Drupal\image\Plugin\Field\FieldType\ImageItem and \Drupal\acquia_dam\Plugin\Field\FieldType\AssetItem)
4. but still conclude that actually, the shape can be populated by the data in either field

Realized this while working on #3557612: `::matchEntityPropsForObject()` is too naïve: nonsensical `type: object` shape matches and useless labels, which is related. It also implicitly demonstrates the problem: the media_video_field it tests with targets both baby_videos and vacation_videos and for 'REQUIRED, type=object&$ref=json-schema-definitions://canvas.module/video 2 different matches are found, which thanks to the prevention of less-efficient-but-equivalent matches has one using baby_videos and the other using vacation_videos:

…
            'ℹ︎␜entity:node:foo␝media_video_field␞␟entity␜␜entity:media:vacation_videos␝field_media_video_file_1␞␟{src↝entity␜␜entity:file␝uri␞␟url}',
            'ℹ︎␜entity:node:foo␝media_video_field␞␟{src↝entity␜␜entity:media:baby_videos␝field_media_video_file␞␟entity␜␜entity:file␝uri␞␟url,poster↝entity␜␜entity:media␝thumbnail␞␟entity␜␜entity:file␝uri␞␟url}',

If you can confirm that that is the situation you're testing with, I'm happy to create an issue for it. Suggested title: Support matching field instances (`DynamicPropSource`s) that contain multi-bundle references

Thanks for confirming. Will make that happen.

As of #3563309-3: [PP-1] Add support for matching against multi-bundle reference fields (e.g. a media field referencing 2 media types of different MediaSource plugins), #3563309's scope is precisely to make #39 & #40 possible. 👍