Component trees with dynamic prop sources that introspect field values don't add the relevant fields to their dependencies

Created on 9 April 2025, about 1 month ago

Overview

If you call \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::calculateDependencies() in a tree that has dynamic prop sources that look into entity fields (bundle fields, not base fields), the field.field.foo.bar.baz config entities for those fields don't get added to the dependencies. They probably should.

Proposed resolution

TBD

User interface changes

TBD, but probably none.

๐Ÿ› Bug report
Status

Active

Version

0.0

Component

Code

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @phenaproxima
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    This is inaccurate.

    ComponentTreeItem doesn't allow DynamicPropSources. That's been the case since ๐Ÿ“Œ Tighten validation: only allow StaticPropSource in XB fields + PageTemplate, DynamicPropSource in ContentTypeTemplate Active .

    Only ContentTemplate config entities' (being introduced in #3517741, which spawned this issue to keep scope tight there) component trees will be allowed to use DynamicPropSources.

  • First commit to issue fork.
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand danielveza Brisbane, AU

    Im pretty interested in following the ContentTemplate work, so I figured I'd make a start on this one based on the IS and the discussion from #2. I've pushed up a POC of the existing work I've done so far, which now adds config dependencies for fields to the content template entities. This has test coverage, but still additional work to be done around code cleanup and removing the legacy code from `ComponentTreeItemTest`

  • Pipeline finished with Failed
    26 days ago
    Total: 1816s
    #474591
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Woohoo, thanks so much! ๐Ÿ˜Š

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    I did some work on this to move the dependency resolution into ComponentInputs. I also made sure that it checks for base field overrides, which are another kind of config dependency (and we do need to test for this).

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    I have a question that either Wim or Lauri need to answer, brought up by this issue.

    The way the MR currently works, a content template that contains a dynamic prop expression targeting a field -- base field override or normal bundle field -- will cause the template to be dependent on that field.

    That makes sense from a data integrity standpoint, but are we not setting up a situation where, if a site builder deletes a field, all templates that mention that field are deleted too? That seems extremely bad.

    What should we do here? We probably need ContentTemplate to implement onDependencyRemoval() to handle this case in some way, or XB to be able to show a graceful fallback (assuming it doesn't already) if a prop expression is referring to a field that no longer exists. Either way, this needs product and tech lead input, so assigning this to Wim before we do any more work on it.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    We certainly should not delete the whole content template as a result of deleting a field. ๐Ÿ˜… I think we should just remove the use of the dynamic prop from the content template. In case the deleted field happens to be a required, we replace the dynamic prop expression with example value.

  • Pipeline finished with Failed
    24 days ago
    Total: 1593s
    #476903
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Cool. This sounds like it's more of a frontend issue, then, and not really a data integrity problem (and thus, I'd imagine, not a stable blocker).

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    This sounds like it's more of a frontend issue, then,

    How/why do you conclude that? ๐Ÿค” ๐Ÿ˜…

    It's not the front end that is deleting a FieldConfig; that's a pure server-side concern. It might also happen through a configuration sync.

    This is the ContentTemplate equivalent of #3484682-12: Handle update and delete of Block `Component`s, plus missing config dependencies โ†’ : we need to implement ContentTemplate::onDependencyRemoval().

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    This is at minimum soft-blocked on ๐Ÿ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active , but not yet sure whether I should hard-block it on that. Your help on that issue would surely be appreciated, @phenaproxima ๐Ÿ™

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    So: @lauriii and I +1'd @phenaproxima's proposal in #10:

    We probably need ContentTemplate to implement onDependencyRemoval()

    โ€ฆ and I missed that while writing #13 โ€” but you can see that there's other cases where we still need to do that, @phenaproxima โ€” it'd be nice for you to lead the way here :)

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    See #3457504-33: XB field type: calculate all dependencies, store them in queryable format, surface in new Component "Audit" operation โ†’ โ€” as of that moment, that MR implements a superset of ComponentInputs::getDependencies() that this MR was adding.

    That means that:

    1. This is now hard-blocked on ๐Ÿ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active โ€” because that other MR clearly is the more precise, complete, reliable solution.
    2. This issue can now be rescoped to handle the ::onDependencyRemoval() part ๐Ÿฅณ

    @phenaproxima: can you please help finish that issue? ๐Ÿ™

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Related, from ~10 months ago, where we predicted exactly this issue: ๐Ÿ“Œ Prevent fields from being deleted if they are used in Experience Builder field's dynamic prop values Active .

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active is in!

    The current MR is AFAICT obsolete; everything in it has already been implemented (in a more comprehensive manner) in #3457504.

    That means that the remaining scope here should be implementing (and testing) ::onDependencyRemoval().

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Took a crack at this - still needs tests and such, but does it look basically like what you're hoping for? I'm not sure what to do if a prop expression in a content template is referring to a removed field, but I'm hoping that just unsetting the input will allow it to fall back to the example value.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Actually, never mind - the issue summary definitely says what a reasonable test would look like.

  • Pipeline finished with Failed
    6 days ago
    Total: 725s
    #490628
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Re-titled and updated the issue summary to reflect the current realities.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    The content template should not be deleted.

    Indeed!

    This should make XB to fall back to the default (example) values for those inputs.

    Yes, that's what @lauriii proposed in #11 ๐Ÿ‘
    In my mind, we would just delete the component instance. But โ€ฆ I think what you propose here (and I guess makes more sense indeed! ๐Ÿ˜„

    So:

    1. I have a ContentTemplate (for article nodes) with a hero component instance whose marquee prop is populated by
      {
        "marquee": {
          "sourceType":"dynamic",
          "expression":"โ„น๏ธŽโœentity:node:articleโfield_funny_messageโžโŸvalue"
        }
      }
      

      i.e. it fetches the value field property of the field_funny_message field on the article node type.

      This was a DynamicPropSource.

    2. The site builder wants to delete field_funny_message; and it's not reasonable for a ContentTemplate to block that, but the ContentTemplate should also not be deleted.
    3. ContentTemplate::onDependencyRemoval(), checks iterates over all component instance inputs and finds the one that actually depends on this particular field (i.e. on the field.field.node.article.field_funny_message config). It replaces the DynamicPropSource with a StaticPropSource.

      Rough outline of algorithm:

      1. Check if this component requires explicit input ComponentSourceInterface::requiresExplicitInput(). If not: [] (the empty array)
      2. Sanity check: assert that Component::getSource() instanceof GeneratedFieldExplicitInputUxComponentSourceBase โ€” only these sources can be populated by fields.
      3. (Untested PoC.) Populate all props with examples (which means: all required ones, plus the optional ones that do have an example).
        $component = Component::load(โ€ฆ);
        $source = $component->getSource();
        assert($source instanceof GeneratedFieldExplicitInputUxComponentSourceBase);
        if (!$component->requiresExplicitInput()) {
          return [];
        }
        
        // @see `type: experience_builder.generated_field_explicit_input_ux`
        $props_with_examples = array_keys(array_filter(
          $source->getSettings()['prop_field_definitions'],
          fn (array $def) => $def['default_value'] !== NULL
        ));
        return array_map(
          fn (string $prop_name) => $source->getDefaultStaticPropSource()->toArray()
          $props_with_examples
        );
        

        ๐Ÿ‘† This should return something like

        {
          "marquee": {
            "sourceType":"static:field_item:string",
            "value": "Hello, world!",
            "expression":"โ„น๏ธŽstringโŸvalue"
          }
        }
        

        (You will need to refactor GeneratedFieldExplicitInputUxComponentSourceBase to make certain methods available to ContentTemplate. For alternative implementation inspiration, see GeneratedFieldExplicitInputUxComponentSourceBase::buildConfigurationForm()).

      4. ComponentSourceInterface::validateComponentInput() on the end result
      5. Prior to saving, verify the end result by using \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator::validate()

      IOW: kinda like \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval() :)

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Wrote an initial (functional) test, but I will still need to update the code based on Wim's outline in #23.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • Pipeline finished with Failed
    4 days ago
    Total: 863s
    #492451
  • Pipeline finished with Failed
    4 days ago
    Total: 811s
    #492523
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Probably ready for an initial review.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • Pipeline finished with Failed
    4 days ago
    Total: 822s
    #492539
  • Pipeline finished with Failed
    4 days ago
    Total: 739s
    #492632
  • Pipeline finished with Failed
    4 days ago
    Total: 834s
    #492647
  • Pipeline finished with Failed
    4 days ago
    Total: 757s
    #492674
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Failed
    3 days ago
    Total: 479s
    #493321
  • Pipeline finished with Failed
    3 days ago
    Total: 844s
    #493330
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    OK, I did some refactoring here to both bring things more in line with what Lee and Wim are asking for, and make it so that only the affected inputs are the ones that get replaced or cleared out (that bit still needs test coverage).

  • Pipeline finished with Failed
    3 days ago
    Total: 1157s
    #493424
  • Pipeline finished with Failed
    3 days ago
    Total: 751s
    #493429
  • Pipeline finished with Canceled
    3 days ago
    Total: 403s
    #493437
  • Pipeline finished with Failed
    3 days ago
    Total: 312s
    #493439
  • Pipeline finished with Canceled
    3 days ago
    Total: 346s
    #493441
  • Pipeline finished with Failed
    3 days ago
    Total: 910s
    #493444
  • Pipeline finished with Failed
    3 days ago
    Total: 754s
    #493456
Production build 0.71.5 2024