- Issue created by @Wim Leers
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
âĻ Allow specifying default props values when opting an SDC in for XB Fixed is in.
- Status changed to Active
2 months ago 2:48pm 9 July 2024 - ðŽð§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 supportnode
. 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 thelink
field type only allows linking toentity:node/*
, and hence only depends on thenode
module! ðĪŠ - ðŽð§United Kingdom f.mazeikis@gmail.com Brighton
Wim Leers â credited f.mazeikis â .
- ð§ðŠ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
and1.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 allowstring
,uri
,file
andimage
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
andimage
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 intoUrl::fromUri()
which can includesentity:
. 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 2:34pm 25 July 2024 - ð§ðŠ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.