gabesullice → created an issue.
Addressed :)
Responded 🏓
gabesullice → created an issue.
gabesullice → created an issue.
Empty commit for credit ^^
gabesullice → created an issue.
griffynh → credited gabesullice → .
@joachim, what if each of the sections are a field item row using a similar number scheme? The value column of the sections field table would still be JSON though. Something like:
Section field item row 2:
[
{ "type": "field", "reference": "field_text:1" },
{ "type": "field", "reference": "sections:3" }
]Section field item row 3:
[
{ "type": "field", "reference": "field_picture:1" },
{ "type": "field", "reference": "field_text:2" },
{ "type": "field", "reference": "field_picture:2" }
]
This would preclude REST/JSON:API and could be tricky for translations or other programmatic updates.
wonky idea: use a single object to make changes, pass the updated object to a smart system, let that system atomically apply the changes using the field API so things can't drift out of sync.
Making the implicit explicit:
I'm suggesting a new PATCHable resource at something like /node/article/{uuid}/layout would the jsonapi module's way of supporting editing fields referenced by the layout field so that the "smart system" would do the job of actually keeping all the fields in sync. That's opposed to forcing JSON:API clients to update 5 fields and the layout field correctly.
This would preclude REST/JSON:API and could be tricky for translations or other programmatic updates.
Warning ⚠️ this is a more free flowing brainstormy thought than a well reasoned suggestion/answer…
Perhaps the design tension you're feeling comes from wanting to directly update the storage model (field by field) vs. updating a single mutable representation (one big blob that combines everything into a nested object)
It's easy to update the blob and keep everything in sync, but inconvenient to store/query. Conversely, it's inconvenient and error prone to update a bunch of interdependent field tables, but easy to store & query.
What is we separate how it's updated from how it's stored?
We could invent a format that can be mutated and saved as a whole? On save, some smart system would translate the changes and apply them to the database using entities and fields as the underlying storage model.
It's not so crazy, that is exactly what would be happening in a visual editor using AJAX or a hidden form input. I.e. the editor would be transferring a "representation" in the application/x-www-form-urlencoded format, then the smart form system would read it and store the changes by making updates via the field API.
Could we come up with another mutable abstract representation that can be updated programmatically? Maybe a JSON doc or a PHP object with mutation methods similar to the Layout Builder's appendComponent. Then when the representation is saved, its state would be translated to field storage?
So this:
If a field is referenced by the layout field, maybe that field should be uneditable outside of the visual editor. That way the layout field can be computed whenever the editor's form is submitted.
…would become: "uneditable via the field API (there be dragons) but editable via the mutable representation"
And then the visual editor and REST et al. could use the same mutable abstraction and it would be the only safe/supported way to update data used in a layout?
tl;dr; wonky idea: use a single object to make changes, pass the updated object to a smart system, let that system atomically apply the changes using the field API so things can't drift out of sync.
1. If field values are updated programmatically, what would happen if they go out of sync with the JSON layout?
I'm not quite sure what you meant by out of sync, but I think you were asking what happens if the field items are reordered?
If so, that feels like it should be treated as an undefined behavior. It reminds me of deleting an inline block programmatically. The result is a message that says "Broken or missing block". Not sure what to do here but it feels like a data integrity issue.
2. Similarly what happens if you delete delta 2 of a multi-value field, would you need to re-delta the other fields and their position in the layout, or would it leave a gap?
I think there should be a field validation constraint that prevents dangling references on the JSON field. It may need to be an entity validation constraint to have access to both fields.
If a field is referenced by the layout field, maybe that field should be uneditable outside of the visual editor. That way the layout field can be computed whenever the editor's form is submitted.
3. As discussed above, it will struggle to support multi-value field within a delta (e.g. two image slider components on one page, where the image field itself is multiple as well as the slider component being multiple) - although this is fairly extreme edge case and there might be workarounds.
I think this might be a faulty example/requirement.
You wouldn't create an image slider field union, you'd create an image slide (no "r") field and place them in a custom section type for a slider. E.g. o support two sliders, you'd have two slider sections. The first would be filled with deltas 0-3 and the second would be filled with deltas 4-7.
Maybe we need a pair of rules like:
- Field unions are for cardinality-locked data. I.e. every subfield has the same cardinality and the same number of items.
- Inline block content is for enforced field sets that are not cardinality-locked. I.e. if you have a component with a single image field but unlimited links then use a custom block type.
Note that rule 2 would not preclude you from using field unions in the custom block type. E.g. if you wanted those links to store a link and an icon choice, the block type would have an image field and a "Download link" field (that is a union of a link and an options_text subfields).
FWIW @gabesullice this … Is basically what field_union module is doing
🤦 I'm sorry. That was faulty memory. I swear I saw a module that was reading field schemas and weaving them together into a combined schema, but I should have done more fact checking.
Regardless, that's awesome news!
I realized that those diagrams conflate the layout containers with fields. So here are two more technically "truthful" diagrams. I also attached a JSON representation of the tree field data → .
The box layout:
Behind the scenes, every picture would be stored in the two picture subfield tables explained above and the JSON field would only store a reference to the field machine name and delta in its component column.
The picture field order would match the order of a left depth first search of the tree to match the top to bottom order of how they'd appear in HTML.
To expand on this, imagine that we have two components represented as two fields. A Picture element composed of an image and text field for a caption. And a Text element for narrative content.
If they're placed into a nested layout in an arbitrary order, it might look something like this if the Text elements are represented in blue and the Picture elements are represented in red.
That layout forms a tree like so:
In the database, there would be 4 tables:
- node_field_text
- node_field_picture_image
- node_field_picture_caption
- node_json_tree
The node_json_tree table would store a JSON object representing the tree from the diagram above. A node in that tree would have a reference like field_text:1 or field_picture:3. Note that it would not have a reference like node_field_picture_caption.
When loading the node, the system would JOIN from the node table to all of the tables above, but the field system would put rows from the composed field tables "together" based on their delta.
The render system would recursively descend the tree stored in a row in the JSON table, effectively calling $node->get('field_text')->get(1) or $node->get('field_picture')->get(3) depending on whether the tree element is red or blue or numbered 1 or 3.
Another proposal for composed fields…
Let's conceptually separate tree structure from field composition. They're really separate concerns. But if we fix field composition, the tree problem becomes simpler. Let's set aside tree structure for now and focus on composed fields.
The difficulty with many of the existing solutions is that they have to retrofit themselves into the field module's framework as it exists, which has no concept or affordances for field composition.
A core-driven approach doesn't need to suffer from that missing abstraction.
The field module could do all the necessary bookkeeping to make it possible and it wouldn't be very complex with regard to the database (all the real complexity would be in the field_ui module).
Let's say you want to compose an image field and a formatted text field to create a "Picture" field that stores an image and a caption (imagine media wouldn't work because the caption is page-specific and different than alt text).
The table schema for each subfield would be indistinguishable from a typical image or text field, but instead of naming their tables node_field_image and node_field_caption, they'd be named node_field_pictur_image and node_field_picture_caption, respectively, where field_picture is the composed field's machine name and image and caption are the subfield machine names.
If the field module and entity system were aware of this composition all they would need to do is JOIN against both tables and then weave the field deltas together in PHP, instead of JOINing agaist a single table per field as they do today.
We would need a new ComposedFieldItemList class to access a ComposedFieldItem with magic methods to get at $node->field_picture->image->uri or $node->field_picture->caption->value.
That missing bookkeeping in the field module is essentially what Paragraphs is using the entity table schema for and why entity reference revisions is necessary. Or why field_union is forced to weave columns together in the field table schema.
Since storage is easy, the trick would be making and intuitive UI for building composed fields.
As for the field ecosystem, the problem with this proposal is that field formatters and widgets expect to receive their own field item lists.
To solve that, existing field types would need to become "composable" by implementing FieldItemWidget and FieldItemFormatters (note the added "Item").
How does that simplify tree structures?
It would mean effulgentsia's components don't need to store any field data.
The layout part of his proposal would remain the same, but the components would point to field deltas or block configuration.
If you're a site builder, you'd add fields for every allowable page element. But that would now include composed fields, so if you wanted a "Picture" component (with image and caption) you'd add a composed field for it.
Behind the scenes, every picture would be stored in the two picture subfield tables explained above and the JSON field would only store a reference to the field machine name and delta in its component column.
The picture field order would match the order of a left depth first search of the tree to match the top to bottom order of how they'd appear in HTML.
Field data doesn't need to be nested, only layouts do.
As for SDC, this still works nicely because one could define a component for the composed field and reuse that field across many bundles. Heck, across entity types of the composed field is configured separately à la Paragraph types.
Added tests.
is there a simple way to add to our test coverage?
Missed this. Moving back to NW.
tl;dr: not sure yet
Back to NR.
I pushed two commits to the issue branch. Thanks for the head start @scotwhith1t! I ended up starting from a blank slate based on the 3.0.x branch, but I used your patch as inspiration.
The two commits:
- Add an input to configured a theme hook suggestion
- Set the menu depth to be relative to the active menu item
I propose any remaining functionality from the menu_block module be broken out into their own feature request issues to keep the scope of changes to a minimum. I probably should have broken the two commits above apart.
gabesullice → changed the visibility of the branch 3366930-theme-suggestion-and-dynamic-level to hidden.
gabesullice → made their first commit to this issue’s fork.
gabesullice → made their first commit to this issue’s fork.
Converted your patch to a merge request ^
I didn't make any changes. Let's let a maintainer review this.
gabesullice → created an issue.
gabesullice → created an issue.
Took a swing at this and updated the IS
gabesullice → created an issue.
gabesullice → created an issue.
gabesullice → made their first commit to this issue’s fork.
For those looking for a simple workaround to fix their kernel tests, the following additions to my kernel test class suppressed the exception.
I imported Drupal\Tests\user\Traits\UserCreationTrait and added that trait to my kernel test class with use UserCreationTest. Then I added $this->setUpCurrentUser() in my class's setUp method. In my case, I didn't need to call that method any optional arguments.
effulgentsia → credited gabesullice → .
Thanks @zrpnr!
tim.plunkett → credited gabesullice → .
A gaunt hand breaks through the damp earth of the cemetery grounds, once dead, the zombie (issue) rises to torment us again.
heh, jokes aside. I stumbled in here because I came across a similar need. @tim.plunkett, you're right that you can make this work with the form system as-is, but one needs to customize the validateForm method and call setValueForElement() method. So yes, it's already possible, but only if you have a deep enough understanding of the form API to understand what's going on in FieldStorageAddForm and are willing to set data during the validation step.
I think it's a reasonable feature request to make this part of the machine name form element API via a #value_prefix property or similar. Perhaps as a #process callback. I guess that's why this issue hasn't ever been closed.
Maybe this bump will breath new life into the idea ;)
Thank you so so so much for all the heart and soul you've poured into our little internet outpost. We're all better for it and we'll honestly never be able to repay it entirely.
Thank you from the bottom of my heart.
We'll always love ya and we'll never forget ya!
#12++
I'm trying to do that in
MenuLinkTree, but I'm running into a hiccup because, at some point in the process of building a menu link tree, a particular item may not have any visible children even though one will be added later. Thus, my current code is removing items that it shouldn't because it doesn't "know" about the child that doesn't exist yet. I think I just have to spend more time with Xdebug to figure out where these missing child links are added.
To follow up on this: the issue was in the toolbar module. It does a quirky thing where it only loads the top level of the admin menu, then it loads the subtree of each of those top level items separately, rather than loading the whole tree at once. This explains why I wasn't seeing any children when I was step-debugging the first load of the admin menu (it was only loading a shallow depth).
Needs work to update the toolbar tests, which makes sense.
To clarify: it makes sense because I'm fiddling with the menu and the toolbar tests probably make some assumptions that don't hold any longer.
Needs work to update the toolbar tests, which makes sense.
Okay, I added code to update the database schema on existing sites. I was expecting to write an update hook to do this, but it seems like the default menu tree storage service handles its own schema changes, so I made my changes in there instead. This probably needs special attention.
Needs review for tests.
Needs work for an update hook to add the transitive column to existing sites.
I spent some time trying to get this working by adding an additional access check to all of the routes which use the SystemController::systemAdminMenuBlockPage controller, but I don't think that's going to be a fruitful approach. It's difficult to check access to menu links from that controller and after digging in, it felt a little hacky.
Instead, I think it might be more useful and generally useful to add a new flag to menu items to indicate whether it should or shouldn't be shown if doesn't have children. Tentatively, I'm naming this flag transitive with a default value of FALSE. The idea is that menu links like People and Development can be marked "transitive" since their only purpose is to transit users from the root page to a child page. Thus, if they have no visible child links, they can be safely elided.
I'm trying to do that in MenuLinkTree but I'm running into a hiccup because at some point in the process of building a menu link tree, a particular item may not have any children but will have one added later. So, my current code is removing items that it shouldn't because it doesn't have a way to "know" about the child that doesn't exist yet. I think I just have to spend more time with Xdebug.
The drawback of the menu link tree based approach is that the system menu block pages would still be accessible, even without any links, if a user visited the right URL directly. That feels like a separate issue that could be handled in a follow-up though. All we'd need to do would be to throw a 403 or 404 HTTP exception in the controller instead of printing out the message.
Failing test.
I was able to reproduce this on the tip of the 9.5.x branch and I updated the issue description with steps to reproduce this bug from a clean installation.
+1 to UpperCamel casing for enum names.
The current approach is not workable because it is a significant feature regression.
@xjm, I think there's a legitimate bug here, but the issue went a little off track by mixing in performance and scalability concerns.
What's the bug?
In short, if I'm on the /admin/structure/menu/manage/main/add route (note the main route param), I shouldn't be able to create a new menu link in the footer menu, but I can.
To expand on that: imagine if you're editing the and you follow the button. Now also imagine that you probably have a menu link with the same name in both the and menu. On the create form, you accidentally choose the link in the menu because you (understandably) assumed that you're adding a link to the . Worse, after you click save, you'll be redirected back to the overview and you'll be presented with a green status message saying "The menu link has been saved," however you will not see your newly created menu link since it was actually added to the menu.b
gabesullice → made their first commit to this issue’s fork.
Thank you @Wim Leers! I'm going to go ahead and make you a maintainer since I know you have some critical deps on this module. This change looks good to me!