Problem/Motivation

The DraggableListBuilder class is meant to be used to create draggable lists of configuration entities. However, it has specific requirements that are not clearly documented in the class itself, which can lead to confusion for developers trying to extend or use it correctly.

In particular:

  • The entity type must declare a 'weight' entity key in its entity_keys.

Steps to reproduce

  1. On a clean installation, generate a configuration entity with: drush gen config-entity
  2. Follow the prompt requests and drush will create your test config entity.
  3. Create some test content to check the list feature
  4. By default you will see the YourClassNameListBuilder file which extends ConfigEntityListBuilder
  5. Replace this to extends extends DraggableListBuilder
  6. The list page wont have the draggable feature on it.

Proposed resolution

Add proper documentation to the DraggableListBuilder class, including:

  • A class-level docblock that explains its purpose and requirements.

Remaining tasks

  • Add class documentation.

User interface changes

None.

Introduced terminology

None.

API changes

None. This change only adds documentation.

Data model changes

None.

Release notes snippet

Added documentation to the DraggableListBuilder class, clarifying the requirement for a 'weight' entity key in configuration entity types using this base class.

Issue fork drupal-3485650

Command icon Show commands

Start 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:

About issue forks

Comments

joachim created an issue. See original summary.

chandansha made their first commit to this issue’s fork.

nitishchopra’s picture

nitishchopra commented
Issue tags: +Singapore2024

isa.bel made their first commit to this issue’s fork.

isa.bel opened merge request !11745

isa.bel’s picture

isa.bel
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
Category: Bug report » Task
Status: Active » Needs review

Documentation added, moving to Needs Review status

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Pipeline appears to have issue.

For good practice issue summary should be filled in also, if sections don't apply can leave blank or add NA.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

brandonlira made their first commit to this issue’s fork.

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Hi,
I've rebased the branch onto the latest 11.x and fixed the coding standard issues.

All tests are now passing.

Please review when you have a moment, and feel free to let me know if anything else need.

Thank you!

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

As a novice task would be good practice to update issue summary.

brandonlira’s picture

brandonlira commented
Issue summary: View changes
Status: Needs work » Needs review

Hi @smustgrave,

I've updated the issue summary to reflect the purpose and outcome of this task, including steps to reproduce the issue from a documentation perspective, and clarifying that this was a documentation-only change.

Please let me know if any further adjustments are needed.

Thanks!

brandonlira’s picture

brandonlira commented
Issue summary: View changes
joachim’s picture

joachim commented
Status: Needs review » Needs work

- We only need class-level docblock
- getWeight() and setWeight() are not needed. The list builder uses the get() and set() method which all config entities have.

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Hi @joachim,

I've removed MR !11745 the method-level docblocks for getWeight() and setWeight() as suggested. The class-level and constructor documentation were kept to clarify usage requirements. Let me know if anything else should be adjusted.

Thank you!

joachim’s picture

joachim commented
Status: Needs review » Needs work

The IS needs updating too.

We really don't need docs on __construct -- they will just duplicate the class docs.

This constructor sets up the form builder for compatibility, disables
   *   pagination (to allow full drag-and-drop), and stores the weight key
   *   if it is defined in the entity type.

This is useful, but put it as an inline comment at the top of the method code.

The docs in this MR all seem very verbose -- it's not AI generated is it?

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Hi @joachim,

I’ve made the changes you suggested, removed the docblock from __construct() and added the inline comment at the top of the method.

About the wording: I’m not sure if any of it was AI-generated, as the work had already been started by someone else. I just continued from what was already there.

Let me know if anything else needs changing.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Left some comments.

igorgoncalves made their first commit to this issue’s fork.

igorgoncalves’s picture

igorgoncalves commented
Issue summary: View changes
Status: Needs work » Needs review

Issue summary update.

igorgoncalves’s picture

igorgoncalves commented

Hi guys,

i agree with @joachim that a lot of new comments seems redundant.
So, i made some changes which i think has enough to inform what and how we can enable the draggable list feature.

So, assuming you follow the steps-to-reproduce, the drush generator create my test entity type just like:

 *   entity_keys = {
 *     "id" = "id",
 *     "label" = "label",
 *     "uuid" = "uuid"
 *   }

so, simply adding the "weight" = "weight" into that, the draggable will works (as screenshot below)
Only local images are allowed.
then i thought add this simple code example at class docblock could be useful aswell.

i also removed the new "inline comment" from the construct, as the whole information at those lines already being explained at the method.

joachim’s picture

joachim commented

That seems like overkill to me. It's also less clear than saying in words what is needed.
Developers using this class will be creating custom entity types, so they should know what an entity keys property is.

igorgoncalves’s picture

igorgoncalves commented

Fair enough.

code example removed.

joachim’s picture

joachim commented

Looks good, though removal of the __construct() docblock seems unrelated?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Good catch! Can we add that back please

igorgoncalves’s picture

igorgoncalves commented
Status: Needs work » Needs review

Thanks for that Joachim.

Fix made.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Thanks!

LGTM!

nod_ closed merge request !11745

  • nod_ committed 5df135d5 on 11.x 
    Issue #3485650 by brandonlira, igorgoncalves, isa.bel, joachim,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

Committed 5df135d and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.