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

-

#2004282: Injected dependencies and serialization hell

Files: 
CommentFileSizeAuthor
#20 2074875-20.patch4.67 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es).
[ View ]

Comments

tim.plunkett’s picture

Component:entity system» configuration entity system
Issue tags:+Configuration system, +Configurables

Sure, 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']).

andypost’s picture

Taxonomy should hide weight header's column when there's only one vocabulary

Berdir’s picture

The 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?

tim.plunkett’s picture

Title:Provide better DX for DraggableListController» Reload entities in DraggableListController::submitForm()
Status:Active» Needs review
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,494 pass(es), 18 fail(s), and 27 exception(s).
[ View ]

Here's one approach.

Status:Needs review» Needs work

The last submitted patch, draggable-2074875-4.patch, failed testing.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Missed the failure message.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new910 bytes
new3.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch draggable-2074875-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oh, that was silly. Missed an array_keys()

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

Great

andypost’s picture

Status:Needs review» Reviewed & tested by the community

This really awesome!

+++ b/core/modules/locale/locale.module
@@ -724,7 +724,7 @@ function locale_library_info_alter(&$libraries, $module) {
+  $languages = $form_state['build_info']['callback_object']->load();

this needs comment in upcoming change notice, because it's only example of alter in core to access entities within list controller

andypost’s picture

Status:Reviewed & tested by the community» Needs review

Suppose @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

andypost’s picture

Assigned:Unassigned» andypost
Status:Needs review» Needs work

Taking over, talked in IRC

timplunkett, I know that there's 3rd way to solve this by taking serialization - so alter should run on loaded entities within form builder, the validation and submit should have the same list of loaded entities
Berdir’s picture

I 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)

alexpott’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system, -Configurables

#7: draggable-2074875-7.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, draggable-2074875-7.patch, failed testing.

mtift’s picture

Status:Needs work» Needs review
StatusFileSize
new3.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch draggable-2074875-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

straight reroll

andypost’s picture

+++ b/core/modules/locale/locale.module
@@ -694,7 +694,7 @@ function locale_library_info_alter(&$libraries, $module) {
-  $languages = $form['languages']['#languages'];
+  $languages = $form_state['build_info']['callback_object']->load();

this should not happen, so we need to store entities, just get rid of serialization

andypost’s picture

Assigned:andypost» Unassigned
Issue summary:View changes

ni idea how to prevent serialization

Status:Needs review» Needs work

The last submitted patch, 15: draggable-2074875-15.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new4.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es).
[ View ]

Search pages now affected, also cleaned a code