[PP-1] Default props values should support files/images

Created on 9 July 2024, 9 months 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, link etc. field types).

This issue is for adding support for static prop sources whose default value causes it to depend on another config 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

Postponed

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 9 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! πŸ€ͺ

  • πŸ‡¬πŸ‡§United Kingdom f.mazeikis Brighton
  • πŸ‡§πŸ‡ͺ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 9 months 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.

  • Status changed to Active 19 days ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Actually, πŸ› Error when adding a section to the page Active reminded me that this is not obsolete, because XB allows creating a "Section" (a Pattern config entity) from any existing component instance tree on a content entity, which automatically comes with a content entity dependency.

    The simplest possible solution is to remove the component instance explicit inputs whose values depend on content entities, which is possible since Drupal 8.8: https://www.drupal.org/node/3066005 β†’

    But that will only work for optional explicit inputs.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    AFAICT that means that target_id: 42 is unacceptable in the context of a Pattern config entity; it must be transformed to target_uuid: eddf9bdb-78bc-4ca0-a8ba-b58ddd1d00ec instead, which then would allow @Felix' PoC MR to work: https://git.drupalcode.org/project/experience_builder/-/commit/8ecc46f91....

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    We have a default content import solution in core that is used by recipes.

    It has support for files in \Drupal\Core\DefaultContent\Importer::copyFileAssociatedWithEntity

    We don't yet have support for exporting (still requires contrib default_content module) but perhaps we could depend on default_content module and do the actual exporting of the media/file entities as part of config export/save of patterns.

    We could then listen to the 'missing content' event in config sync and run the import?

    Longer term we could work with the Recipes initiative to add the exporter bits of default content to core.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Importer::copyFileAssociatedWithEntity requires a file to exist on disk in the "default content" directory.

    @lauriii has indicated many times now that that is a "hard no" from him. All XB config MUST be exportable, importable, shareable. That means #16's proposal does not meet his product requirements.

    https://git.drupalcode.org/project/experience_builder/-/commit/8ecc46f91... solved it. We "just" need to port it into the XB codebase.

Production build 0.71.5 2024