Fix sortable support for select2 4.x

[jorrit]

Subject

The ModelType supports sorting the results. This functionality broke when Sonata Admin 4 updated Select2 to version 4. This PR fixes the javascript function setup_sortable_select2 make this function work again.

Please note that this function still depends on deprecated functionality of select2, because it uses a hidden input instead of a select element. I don't see an easy fix for this.

I am targeting this branch, because it is broken in this branch.

Changelog

### Fixed
- Sortable `ModelType`

[jorrit]

No, I didn't make a screenshot. It still looks the same though. The only difference that I know of is that the options given to Select2 are also in the sorted order. I could not make it such that the options are displayed right on page load in a different way.

[jorrit]

By the way: right now submitting a sortable ModelType field is not possible, the form says that the value invalid. It seems this is not the most popular feature, perhaps you should deprecate it as it is hard to maintain.

[VincentLanglet]

By the way: right now submitting a sortable ModelType field is not possible, the form says that the value invalid. It seems this is not the most popular feature, perhaps you should deprecate it as it is hard to maintain.

Since we didn't receive any issue/complaints, seems like you're the only one using it indeed ^^

I personally don't know

• how useful it can be
• how hard is it to maintain
• how much code we save by deprecating/removing the feature You may know better than me, so it's kinda your call on it.

I would say that we almost never touch this, So if this PR fix it ; we can keep it like this and there is no real need to deprecate it.

WDYT @jordisala1991 ?

[jordisala1991]

TBH I dont know enough this part of the code, kinda hard to approve since is a big change (not by a lot of lines, but we must assume the previous code was completely broken) and there are no tests that cover the feature.

[VincentLanglet]

TBH I dont know enough this part of the code, kinda hard to approve since is a big change (not by a lot of lines, but we must assume the previous code was completely broken) and there are no tests that cover the feature.

But I don't feel like we can do a lot about this. We don't have easy way to test js features.

[VincentLanglet]

What can be done to get your approval on this PR @jordisala1991 ? A screenshot ?

[jordisala1991]

I would like to test it, can you show me what kind of configuration was not working before and works with this PR?

[VincentLanglet]

Can you give an example @jorrit ?

[jorrit]

My field is configured as follows:

            ->add(
                'field',
                ModelType::class,
                [
                    'class' => SomeClass::class,
                    'multiple' => true,
                    'btn_add' => true,
                    'expanded' => false,
                    'choices' => ['a' => 'a', 'b' => 'b'],
                    'sortable' => true,
                ]
            )

Sortable is implemented using jQuery UI's sortable helper. This makes the HTML elements generated by Select2 draggable. However, Select2 does not pick this up by itself. In the version of Select2 used by admin bundle 3.x, there was a simple call to update the internal state of Select2 after drag and drop, but in the new version this call doesn't exist. This PR uses the text of the HTML nodes to sort the values in the hidden HTML field. It's very hacky, but it works for now.

In addition, the new Select2 version does not display the selected items in the order given by the current value when loading the page.

[jordisala1991]

I tried that form field with and without your PR, and the things that I observe are the following:

1. Without your PR , when I add sortable true, select2js breaks 👍
2. With your PR, when I add sortable true, select2js does not break 👍
3. With your PR, I can move around the items in the select 👍
4. When I save, the position is not saved 👎

There are no errors for this on my side, my entity has the position field but not sure how does he knows it needs to use that property. Maybe your example is incomplete?

5. When I add a new Item with the modal, with your PR, when I save, it cleans the list 👎

There is a validation error that does not happen without sortable true:

The selected choice is invalid.

The choices "4,2,3" do not exist in the choice list.

I tried to debug a little bit and I think something might be wrong around here but can't confirm: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Form/ChoiceList/ModelChoiceLoader.php

To be noted that without activating sortable, everything works when I save childs.

---

More information, I tried to use an "easy" reproducible field to test it, so I picked this one:

https://github.com/sonata-project/SonataMediaBundle/blob/4.x/src/Admin/GalleryAdmin.php#L105-L112

I installed SonataMediaBundle on my project, and edited manually vendors to replace the field for this other one:

                ->add(
                    'galleryItems',
                    ModelType::class,
                    [
                        'multiple' => true,
                        'sortable' => true,
                        'by_reference' => false,
                    ], [
                    ]
                )

Without the by_reference items won't get added. With that change, you can enable / disable 'sortable' property and test my findings too.

[jorrit]

Thanks for your review! I have not tested my code in combination with adding a new button via a modal. The btn_add option doesn't seem to do anything for me. I don't see an add button, even when I remove the choices option.

You are right about not saving the item. I didn't put this in the PR because it did not work in v3 either and I forgot about the workaround I added in my code. The problem is that the MergeCollectionListener discards the order of the posted data because it loops through the original value and adds new items to the bottom of the list. I guess it does that because the user might not have access to all possible entities and in this way the inaccessible related items are not removed from the database. I worked around this by adding a second listener that does $event->stopPropagation() so the default MergeCollectionListener is not run.

So in a sense, this feature didn't work in v3 either.

[VincentLanglet]

You are right about not saving the item. I didn't put this in the PR because it did not work in v3 either and I forgot about the workaround I added in my code. The problem is that the MergeCollectionListener discards the order of the posted data because it loops through the original value and adds new items to the bottom of the list. I guess it does that because the user might not have access to all possible entities and in this way the inaccessible related items are not removed from the database. I worked around this by adding a second listener that does $event->stopPropagation() so the default MergeCollectionListener is not run.

Do you think a PR to fix https://github.com/sonata-project/SonataAdminBundle/blob/bc3871e06cffc7498058bb225a29a43cb74f51ff/src/Form/EventListener/MergeCollectionListener.php would be possible ?

[jorrit]

I would first need to understand why it exists in the first place.

@jordisala1991 If you retry your tests with MergeCollectionListener disabled, does the order save correctly?

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

I kinda lost the track of this PR.

@jorrit @jordisala1991 what is the status / next steps ?

[sterrien]

Up !

I'm encountering the same issue 😄

[VincentLanglet]

I'm encountering the same issue 😄

Unfortunately I didn't really follow the discussion on this PR and @jordisala1991 is really busy this month. Feel free to help :)

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

There are a few problems but I think they are not related with this PR but with how Select2 works, (spoiler, we might want to replace this library with TomSelect in a future version of Sonata)

Given this PR fixes a bug, even if it is not perfect I think we should merge it, the remaining problems are more problems of the library used rather than this implementation.

I have tried this with one of our big projects using Sonata on a field that previously was working with Sonata 3, and it wasn't working with Sonata 4 until I applied this PR.

wdyt @VincentLanglet ?

If it fix a bug without any drawback I'm ok to merge this.

[jordisala1991]

Thank you @jorrit

[sterrien]

Many thanks @jorrit @VincentLanglet @jordisala1991 !

[jorrit]

Thanks for merging. I was pondering about withdrawing the PR because it was such a hack. Tom Select seems fine, this small piece of code seems to do exactly what we need:

new TomSelect("#select-state",{
  plugins: {
    drag_drop: {},
		remove_button: {
			title:'Remove this item',
		}
	},
});