Support props defaults that have content dependencies: avoid File content entity dependencies

Created on 9 July 2024, 2 months ago
Updated 25 July 2024, about 1 month ago

Overview

Follow-up for âœĻ Allow specifying default props values when opting an SDC in for XB Fixed .

That issue added support for static prop sources, but with some (unenforced) limitations:

A static prop source uses a field type. For many field types, the above won't be an issue, because the data is entirely self-contained (e.g. the string, uri etc. field types).

This issue is for adding support for static prop sources whose default value causes it to depend on a content entity:

Not every entity reference ought to be allowed. Only entity references that target a File entity should be allowed, at least initially. Because files can reasonably be serialized into the exported config, but the same is not true for arbitrary entity references.
This is where @Felix' prior PoC MR comes in, to allow exporting/syncing even default props values that rely on e.g. images.

User interface changes

None.

📌 Task
Status

Closed: outdated

Component

Page builder

Created by

🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @Wim Leers
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • Status changed to Active 2 months ago
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
  • 🇎🇧United Kingdom catch

    (e.g. the string, link etc. field types).

    This isn't the case for link, it takes a URI, which includes the entity:// format, which has an implicit, if not explicit, dependency on the entity being linked.

    Not every entity reference ought to be allowed. Only entity references that target a File entity should be allowed, at least initially. Because files can reasonably be serialized into the exported config, but the same is not true for arbitrary entity references.

    For all other default values or config -> entity references in core (blocks is one example), we require that the entity exists on the site in order to be able to export the reference. This has some inherent limitations - like having to create content on production, sync it locally, then create the config, then sync the config, but those limitations enforce some good practices too, and it does allow any entity reference to work. Why uniquely diverge from that pattern here and start (I assume) base64 encoding images into config?

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚
    (e.g. the string, link etc. field types).

    This isn't the case for link, it takes a URI, which includes the entity:// format, which has an implicit, if not explicit, dependency on the entity being linked.

    Oh, that is a very good point! 😎

    After I could not find any test coverage for this, I tried to create a Link field (allowing both internal & external links), and then entered entity:user/1 as the default field value â€Ķ it refused to accept this?! ðŸ˜ģ
    So I continued without a default field value, and instead tried using it. Turns out this automatically uses an entity reference autocomplete for the same entity type + bundle (in my case: node:page). Huh?!

    I then found test coverage over at \Drupal\Tests\link\Functional\LinkFieldTest::doTestURLValidation(). Following the trail of breadcrumbs, I ended up at \Drupal\link\Plugin\Field\FieldWidget\LinkWidget::getUriAsDisplayableString(), which contains:

        elseif ($scheme === 'entity') {
          [$entity_type, $entity_id] = explode('/', substr($uri, 7), 2);
          // Show the 'entity:' URI as the entity autocomplete would.
          // @todo Support entity types other than 'node'. Will be fixed in
          //   https://www.drupal.org/node/2423093.
          if ($entity_type == 'node' && $entity = \Drupal::entityTypeManager()->getStorage($entity_type)->load($entity_id)) {
            $displayable_string = EntityAutocomplete::getEntityLabels([$entity]);
          }
        }
    

    â€Ķ which was introduced by #2804391: Resaving menu links that points to a non-node entity changes the type to node and breaks the link → in 2017. It narrowed the entity: linking functionality that 📌 Implement autocomplete UI for the link widget Fixed introduced in 2015, to only support node. So â€Ķ until 📌 Allow multiple target entity types in the 'entity_autocomplete' Form API element Needs work lands, this is pretty much a non-issue, because the link field type only allows linking to entity:node/*, and hence only depends on the node module! ðŸĪŠ

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    For all other default values or config -> entity references in core (blocks is one example), we require that the entity exists on the site in order to be able to export the reference. This has some inherent limitations - like having to create content on production, sync it locally, then create the config, then sync the config

    Correct, this is how it works by default, today.

    Why uniquely diverge from that pattern here and start (I assume) base64 encoding images into config while disallowing other types?

    Because @lauriii has clarified requirement 63. Shared components and

    1.2 Design system (foundations)</a> as being able to share components from one site to another, including default values for props, using <em>nothing else besides standard configuration management</em>.
    
    (That's a big reason for 
                
                  
                  
                  ðŸŒą
                  [META] Configuration management: define needed config entity types
                    Active
                  
                 being so very important, and why there's such emphasis on config dependencies.)
    
    That's why uniquely diverging for component config entities' default props was considered A) reasonable, B) required.
    
    We labeled this the Big Default Content Question: instead of referencing content entities and hence depending on them, and hence needing recreate them/ensure they exist on another site, we base64-encode files/images (the 95% use case) to avoid that problem altogether. Additional entity fields/properties that are used such as alt, titles, labels could also be exported along with that.
    
    See <a href="https://git.drupalcode.org/project/experience_builder/-/commit/8ecc46f9141b0e563605b33ebdae7baf1d7730fc"><code>tests/src/Kernel/ConfigContentExportTest.php

    in @f.mazeikis' PoC for how this works quite elegantly.

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    Oh, and @tedbow pointed out that there are related/similar problems in Layout Builder: 🐛 Imported layout builder settings fail if inline blocks are present due to missing revisions Active .

  • 🇎🇧United Kingdom catch

    I'm confused - even though it only allows linking to a node, it still allows linking to an entity that's not a file, whereas the issue summary is specifically saying only files (not even media) should be supported because they can be serialized into YAML. If you can add a default value for a link that points to a node, the problem is there.

  • 🇎🇧United Kingdom catch

    We labeled this the Big Default Content Question: instead of referencing content entities and hence depending on them, and hence needing recreate them/ensure they exist on another site, we base64-encode files/images (the 95% use case) to avoid that problem altogether.

    So to double check - what this does is to only base64 encode the image when configuration is exported, and then save it as a file entity when the configuration is imported - then on the site itself, the default value is still a file reference, not the actual base64 encoded image?

    That bit seems superficially OK but I'm not sure I get the full consequences yet - if I export that config again (to config/sync) then import it, is it going to keep getting converted between base64 and and a file reference (presumably with a new file over and over again) or is the file uuid also stored so that can be used if the file already exists?

  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    #9:

    Ah, I see how that's confusing!

    âœĻ Allow specifying default props values when opting an SDC in for XB Fixed landed, and it did add support for a number of field types. But link is actually not one of them — the validation constraints only allow string, uri, file and image field types: https://git.drupalcode.org/project/experience_builder/-/blob/9ccf646d71c...

    "link" was just intended to be an example â€Ķ but as you demonstrated: a poorly chosen one! So, updated issue summaries to rectify that.

    The link field type is out of scope here, that's what 📌 [later phase] Support props defaults that have config dependencies Postponed exists for.

    This issue is intended only to make it possible to specify a default value for the file and image field types. âœĻ Allow specifying default props values when opting an SDC in for XB Fixed did not add support for that: it only allowed selecting those field types/widgets, but no default value can be specified yet.

    Tried to clarify that. (And spotted some very confusing language because of it 🙈)

    Cross-post! ðŸ’Ĩ

    #10:

    So to double check - what this does is to only base64 encode the image when configuration is exported, and then save it as a file entity when the configuration is imported - then on the site itself, the default value is still a file reference, not the actual base64 encoded image?

    Yes, exactly!

    That bit seems superficially OK but I'm not sure I get the full consequences yet - if I export that config again (to config/sync) then import it, is it going to keep getting converted between base64 and and a file reference (presumably with a new file over and over again) or is the file uuid also stored so that can be used if the file already exists?

    Excellent question. Agreed we should not do that. But this seems easily solved: rather than generating a random UUID, generate a UUID from the base64-encoded representation of the file.

  • 🇎🇧United Kingdom catch

    the validation constraints only allow string, uri, file and image field types:

    OK, but uri has exactly the same problem, it supports anything you can pass into Url::fromUri() which can includes entity:. As above, I don't think this is a new problem at all, but don't want people to be lulled into a false sense of security.

  • Issue was unassigned.
  • Status changed to Closed: outdated about 1 month ago
  • 🇧🇊Belgium Wim Leers Ghent 🇧🇊🇊🇚

    Just discussed #3461499-17: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → with @laurii, he agrees that all defaults should be specified in the SDC, not in a Component. See #3461499-19: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → and preceding comments.

    That means this is obsolete.

Production build 0.71.5 2024