Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Updated: Comment #0
Problem/Motivation
Follow-up for #1855402-110: Add generic weighting (tabledrag) support for config entities (DraggableListController)
The entities are serialized with all injections
<berdir> andypost: I think what it does right now is confusing yes. and scary
<berdir> andypost: try this debug(serialize($form), $form_id); in a hook_form_alter() for a draggable form
<berdir> andypost: there's *everything* in there ;)
<berdir> request, full container, module lists, database connection, ...
Proposed resolution
TBD
Remaining tasks
TBD
User interface changes
-
API changes
-
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#44 | reroll_diff_2074875_25-44.txt | 6.34 KB | ankithashetty |
#44 | interdiff_2074875_42-44.txt | 2.56 KB | ankithashetty |
#44 | 2074875-44.patch | 6.24 KB | ankithashetty |
| |||
#43 | interdiff_42-43.txt | 4.76 KB | sahil.goyal |
#43 | 2074875-43.patch | 8.61 KB | sahil.goyal |
|
Comments
Comment #1
tim.plunkettSure, doing
$this->storage->loadMultiple($form_state['values'][$this->entitiesKey])
in submit is fine. I actually had that.The only reason I moved it into $this->entities was because of
locale_form_language_admin_overview_form_alter()
, which assumes it has access to the full list of entities (in$form['languages']['#languages']
).Comment #2
andypostTaxonomy should hide weight header's column when there's only one vocabulary
Comment #3
BerdirThe title is a bit misleading, this isn't about DX in any way. The serialize stuff actually comes from the form class itself and I hope that will improve with the now RTBC' serialization issue.
This just feels weird to me, given how these form classes work, to store the entities on the class and re-use them. At least validate() runs on a cached form, so I'm really unsure on what list of entities they will actually run, the one that got serialized before?
Comment #4
tim.plunkettHere's one approach.
Comment #6
tim.plunkettMissed the failure message.
Comment #7
tim.plunkettOh, that was silly. Missed an array_keys()
Comment #8
tim.plunkettGreat
Comment #9
andypostThis really awesome!
this needs comment in upcoming change notice, because it's only example of alter in core to access entities within list controller
Comment #10
andypostSuppose @berdir's approval is needed here.
I'd prefer to store a list of IDs in form_state or actually in state() service for cache, because load() should happen on cached list
Comment #11
andypostTaking over, talked in IRC
Comment #12
BerdirI don't really understand #10/#11 yet but the previous patch looks fine to me.
The entity ID's are already available as element_children($form[$entity_type]), are they not? At least that's what the submit method assumes as well (the only children in that form element being entity id's)
Comment #13
alexpott#7: draggable-2074875-7.patch queued for re-testing.
Comment #15
mtiftstraight reroll
Comment #16
andypostthis should not happen, so we need to store entities, just get rid of serialization
Comment #17
andypostni idea how to prevent serialization
Comment #20
andypostSearch pages now affected, also cleaned a code
Comment #22
jhedstromPatch no longer applies.
Comment #23
ashishdalviWe will take this issue on Drupal Mumbai code sprint today.
Comment #24
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #25
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedAdded a rerolled patch.
Resolved the conflicts in files :
1. DraggableListBuilder.php
2. LanguageListBuilder.php
3. locale.module
Comment #26
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #28
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPatch #25 still applies cleanly.
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Tagging for reroll of 10.1
Tagging for issue summary update for proposed solution and remaining tasks (relying on the community for this one)
Comment #42
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedPatch for 10.1.x.
Please review.
Comment #43
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUpdating the patch against #42 trying to resolve the CCF errors.
Comment #44
ankithashettyThanks, @sahil.goyal for the fix. But the patch in #43 has additional changes like
StringTranslation
which is out-of-scope of this issue.Compared the old patches in #42, #25, #20... The change in
DraggableListBuilder.php
affectedLanguageListBuilder.php
as well.Rerolled the old patch, and made some additional changes to the patch to fix the errors in dependent classes.
Thanks!
Comment #45
smustgrave CreditAttribution: smustgrave at Mobomo commented@ankithashetty thank you for working on this and good work!
Brought this up on #contribute with @andypost and agree that a change record would be best.
Also an IS update for the proposed solution.