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

Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
3.14 KB

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
FileSize
910 bytes
3.16 KB

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
FileSize
3.16 KB

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
FileSize
4.67 KB

Search pages now affected, also cleaned a code

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

ashishdalvi’s picture

We will take this issue on Drupal Mumbai code sprint today.

rasikap’s picture

Assigned: Unassigned » rasikap
rasikap’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Added a rerolled patch.

Resolved the conflicts in files :
1. DraggableListBuilder.php
2. LanguageListBuilder.php
3. locale.module

rasikap’s picture

Assigned: rasikap » Unassigned

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

Issue tags: -Needs reroll

Patch #25 still applies cleanly.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs reroll

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

_utsavsharma’s picture

Patch for 10.1.x.
Please review.

sahil.goyal’s picture

Updating the patch against #42 trying to resolve the CCF errors.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.24 KB
2.56 KB
6.34 KB

Thanks, @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 affected LanguageListBuilder.php as well.

Rerolled the old patch, and made some additional changes to the patch to fix the errors in dependent classes.

Thanks!

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.