Since day 1, it has bothered me that XB's config entities store component trees as JSON blobs. For example, the auto-generated (based on Standard install profile's Block config entities) "Header" PageRegion config entity:
uuid: fefb6b93-d378-49bc-a005-815674e6ca36
langcode: en
status: true
dependencies:
config:
- experience_builder.component.block.system_branding_block
- experience_builder.component.block.views_block.comments_recent-block_1
theme:
- olivero
id: olivero.header
region: header
theme: olivero
component_tree:
tree: '{"a548b48d-58a8-4077-aa04-da9405a6f418":{"0":{"component":"block.system_branding_block","uuid":"3fec9bc4-dc58-42a6-85af-b92268338c3b"},"1":{"component":"block.views_block.comments_recent-block_1","uuid":"870e9006-2d93-4ac2-9294-192d4f6ee724"}}}'
inputs: '{"3fec9bc4-dc58-42a6-85af-b92268338c3b":{"label":"Site branding","label_display":"0","use_site_logo":true,"use_site_name":true,"use_site_slogan":false},"870e9006-2d93-4ac2-9294-192d4f6ee724":{"label":"asdfsdfsdf","label_display":"visible","views_label":"asdfsdfsdf","items_per_page":"none"}}'
It's not just "ugly" for developers though, it makes certain use cases much more painful!
For example: using config actions in Recipes to manipulate XB config entities.
If that was not a single JSON blob but a proper tree structure, then it'd be much easier to use config actions! Something like this:
component_tree:
tree:
a548b48d-58a8-4077-aa04-da9405a6f418:
- component: block.system_branding_block
uuid: 3fec9bc4-dc58-42a6-85af-b92268338c3b
- component: block.views_block.comments_recent-block_1
uuid: 870e9006-2d93-4ac2-9294-192d4f6ee724
inputs:
3fec9bc4-dc58-42a6-85af-b92268338c3b:
label: 'Site branding'
label_display: 0
use_site_logo: true
use_site_name: true
use_site_slogan: false
870e9006-2d93-4ac2-9294-192d4f6ee724:
label: 'asdfsdfsdf'
label_display: visible
views_label: asdfsdfsdf
items_per_page: none
The same problem exists in a bunch of places in Drupal core. XB should not repeat the same mistakes. See #2871354: Improve "allowed_html" to provide a better config diff for an example.
Ideas:
ConfigEvents::STORAGE_TRANSFORM_(IM|EX)PORT to transform the at-rest representation. But will that work with config actions? 🤔config/schema/experience_builder.json_schema.yml. Can we do something similar here?\Drupal\experience_builder\Plugin\DataType\Component(Inputs|TreeStructure)::setValue() to simply accept both an array and a JSON-encoded array, and treat both identically.
None! (Just simpler-to-read YAML at /admin/config/development/configuration/single/export for XB config entities.)
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3521137-future-for-xb-field-props-PoC.patch | 3.45 KB | wim leers |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
wim leersClarified the goal.
Comment #3
wim leersComment #4
catchThank you so much for opening this. Translation from JSON to YAML and back seems hard, but incredibly worth it if the config entities can be treated as normal config entities.
Comment #5
phenaproximaI don't think this will affect config actions much, either way. They are severely limited in their ability to manipulate complex arrays, and regardless of how the data is stored at rest, we'll need a slew of custom config actions to make XB useful to recipes.
But it'd certainly improve DX to have the tree be a YAML structure. I personally don't see any reason why it needs to be a JSON blob, since YAML is a superset of JSON. Nor do I understand why it would be hard to move back and forth between JSON and YAML if we wanted to -- if you can decode some YAML to a PHP array, you can re-encode it as JSON.
Comment #6
phenaproximaI asked @effulgentsia if there was any particular reason why the XB trees are stored as JSON blobs. His answer on Slack:
Comment #8
phenaproximaI think that, to not break everything everywhere all at once, we might want to do this in multiple issues (first deal with
inputs, thentreein another issue).And I'm also thinking that, until #2232427: Allow field types to control how properties are mapped to and from storage is done, we might want to have the ComponentTreeStructure and ComponentInputs data types handle both arrays and JSON blobs. That would ease the difficulty of the transition without blocking us, it looks like.
Comment #9
phenaproximaOkay, I made
inputsa standard array -- does this look more or less correct? (I know thatignoreis the wrong config schema type, but the tree and its inputs are validated as a whole, so I'm not sure it needs to be anything more specific).Comment #10
wim leersA few questions, but this definitely looks to be on the right way! 👏
Next up:
tree, not justinputsdisallowingtriggering a deprecation error for a JSON blobs for config entities (allows smooth upgrade, but ensures we as XB developers area ware)Actually, hard disallowing seems fine given we're in alpha. Thoughts? :)
Comment #11
phenaproximaOK, I think I mostly have
treeusing regular arrays now, too.The lone exception, and the reason the BC layer that converts between strings and arrays is needed, is that field items on content entities won't be storable as JSON transparently until #2232427: Allow field types to control how properties are mapped to and from storage, so until then, we probably need to keep supporting strings of JSON. But we definitely don't seem to need those for storing trees in config.
Comment #12
wim leersComment #13
wim leersI'm shocked by how simple this turned out to be 🤯🙃
Awesome work! 👏
Two things remain:
::encodeXBData()calls as possible::encodeXBData()calls that MUST be removedI know that this is kinda boring, but … it makes the entire codebase easier to work in, and keeps the entire codebase consistent. The MR in its current state removes some
::encodeXBData()calls, but on my first try I got tests to pass with more of those calls removed. So if this MR doesn't make things as consistent as they are in HEAD, it's going to be confusing for everybody else: "when do we need this::encodeXBData()thing vs not?".Comment #14
phenaproximaI've removed as many json_encode() calls as I could find, except for one in \Drupal\Tests\experience_builder\Kernel\FieldTypeUninstallValidatorTest, which fails (for some reason!) if I remove it. I don't think we should remove the BC layer yet because when the value is pulled from the database as a JSON string (when loading the fields), the data type plugins need to be able to handle that...right?
Comment #15
phenaproximaUpdate: FieldTypeUninstallValidatorTest now passes everywhere except on MariaDB. I imagine this is not a blocker, but worth calling out.
Comment #16
wim leersComment #17
wim leersBumping to per #2871354: Improve "allowed_html" to provide a better config diff.
Comment #18
wim leersAs a byproduct, the
TestDataUtilitiesTraitthat @tedbow introduced in #3469253: Convert test cases to use PHP arrays instead of JSON strings where possible to ease test development pain during initial development of XB, will largely be obsolete — that's what I asked for in #13, and which @phenaproxima almost completely finished! 🤩In fact … since I'm feeling under the weather and my mind doesn't allow me to tackle anything complex right now, I just persevered and refactored it away:
TestDataUtilitiesTraitis GONE! No more need to remember to wrap an array in a::encodeXBData()! 🥳 All thanks to the one crucial insight by @phenaproxima! 👏So: we'll not only make interacting with XB config less painful for site builders/XB users, but also for us developing XB, and hence improve development velocity slightly :)
P.S.: I'd still don't like that
->getValue(), TRUEhas 7 matches in the codebase, but that's far better than the hundreds this MR removed. So, for the sake of iterative progress, didn't expand scope to that. Also: once #2232427: Allow field types to control how properties are mapped to and from storage lands, XB won't have to work around that flaw in Drupal core anymore. If somebody wants to take that on: see attached PoC patch to get you going.Comment #20
wim leersComment #22
wim leersFYI: with this done, I think it's time to re-assess #3499632: Remove `ClientServerConversionTrait` (C→S) and `ComponentTreeItemList::getClientSideRepresentation()` (S→C) in favor of (de)normalizers and friends, aka make a decision on whether to keep
ClientServerConversionTraitbecause it is good enough, or whether to transition to something else/better.