Problem/Motivation

The order of the media items is messed up when using Media Library widget in a particular use-case (when arranging the media items and after that inserting a new media).

Steps to reproduce:
1. Create a Content Type with an unlimited media reference field.
2. In form display for that field chose the Media library widget.
3. Add new content of that Content Type.
4. In the media field add more than 3 media items.
5. Save the node.
6. Go to the edit page of that node.
7. Re-arrange the media by drag and drop.
8. Press the "Add Media" button.
9. Select a new image to insert and press the "Insert Selected" button.
10. The media items will be ordered correctly but their weights will be messed up. You can look at them by pressing "Show media item weights".
11. Save node.

Expected behavior:
The order of the media should remain the way it was arranged before inserting a new image and the new image should be the last one.

Current behavior:
The media items order is not the way it was arranged.

Proposed resolution

I didn't find what was the cause of this problem but one of the solutions/workarounds will be to get the weights from the $field_state (which are the correct ones) and put them in the $element in the updateWidget memthod.

Remaining tasks

Add tests.

CommentFileSizeAuthor
#17 before-patch.jpg225.58 KBranjith_kumar_k_u
#17 after-patch.jpg224.5 KBranjith_kumar_k_u
#16 Before Patch #14.png109.41 KBjanmejaig
#15 After Patch #14 .png111.47 KBjanmejaig
#14 interdiff_10-14.txt901 bytesvsujeetkumar
#14 3115054_14.patch3.15 KBvsujeetkumar
#10 interdiff_6-10.txt859 bytesvsujeetkumar
#10 3115054_10.patch3.24 KBvsujeetkumar
#6 interdiff-2-6.txt3.9 KBVidushi Mehta
#6 3115054-rerolled-6.patch2.22 KBVidushi Mehta
#2 media_library-3115054-2.patch2.06 KBsergiuteaca

Issue fork drupal-3115054

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:

  • 3115054-media-library-wrong Comparechanges, plain diff MR !1427
  • 9.3.x Comparecompare
  • 9.4.x Comparecompare
  • 10.0.x Comparecompare
About issue forks

Comments

sergiuteaca created an issue. See original summary.

sergiuteaca’s picture

sergiuteaca commented
StatusFileSize
new media_library-3115054-2.patch2.06 KB

The workaround patch

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.

ramya balasubramanian’s picture

ramya balasubramanian commented
Status: Active » Needs review
pranali.lanjewar’s picture

pranali.lanjewar commented
Status: Needs review » Needs work

As patch #2 applied failed, so setting as a need work.

Vidushi Mehta’s picture

Vidushi Mehta commented
Status: Needs work » Needs review
StatusFileSize
new 3115054-rerolled-6.patch2.22 KB
new interdiff-2-6.txt3.9 KB

As #2 is failed to apply so added a new rerolled patch against 9.1.x

Status: Needs review » Needs work

The last submitted patch, 6: 3115054-rerolled-6.patch, failed testing. View results

shailja179’s picture

shailja179 commented

I have reproduced the issue. I am working on this.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Priority: Minor » Major
Issue tags: +Media Initiative, +Triaged Media Initiative issue, +Usability

This sounds like a major bug to me. If the widget is getting the order wrong, it's losing user input and degrading the experience.

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3115054_10.patch3.24 KB
new interdiff_6-10.txt859 bytes

Fixing Test.

Status: Needs review » Needs work

The last submitted patch, 10: 3115054_10.patch, failed testing. View results

naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Assigned: Unassigned » naresh_bavaskar
naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Assigned: naresh_bavaskar » Unassigned
vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3115054_14.patch3.15 KB
new interdiff_10-14.txt901 bytes

Fixing Tests

janmejaig’s picture

janmejaig commented
StatusFileSize
new After Patch #14 .png111.47 KB

I have verified the above issue and found that after applying the patch #14 it is working fine as expected on Drupal 9.1.x .Please find the attachment for the issue.

Steps To Test:
1. Create a Content Type with an unlimited media reference field.
2. In form display for that field chose the Media library widget.
3. Add new content of that Content Type.
4. In the media field add more than 3 media items.
5. Save the node.
6. Go to the edit page of that node.
7. Re-arrange the media by drag and drop.
8. Press the "Add Media" button.
9. Select a new image to insert and press the "Insert Selected" button.
10. The media items will be ordered correctly but their weights will be messed up. You can look at them by pressing "Show media item weights".
11. Save node.

Expected behavior:
The order of the media should remain the way it was arranged before inserting a new image and the new image should be the last one.

janmejaig’s picture

janmejaig commented
StatusFileSize
new Before Patch #14.png109.41 KB
ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new after-patch.jpg224.5 KB
new before-patch.jpg225.58 KB

I have applied the #14 patch ,the patch applied cleanly and it resolved the current issue.
before applying the patchbefore patch
after after patch .RTBC

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs review » Reviewed & tested by the community
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

I don't think I understand the automated test coverage here. As far as I can tell, we are just removing an assertion, but I don't see how that actually tests the bug. Do we have an explicit automated test of this bug?

To clarify: what would be helpful here is a patch which contains ONLY an automated test which tries to do the thing which doesn't work, and therefore fails (what call a "fail patch"). That would prove that the bug exists, and that the automated test is actually testing for it. Then we could post an additional patch which contains that test and the code which fixes the bug.

Having said that, the patch in #10 seems to be on the right track. It doesn't remove any test coverage, but it modifies an existing test to more properly verify that what we actually expect to happen, happens. The fact that it fails means that it's actually checking for the bug. It also includes a fix, of course, but since it still fails, the fix probably isn't working working as intended. :)

So maybe we need to hack on this a bit more, but I don't think that removing test coverage is a viable path forward.

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.

tanubansal’s picture

tanubansal commented

Patch #14 is working for me as well on 9.1
RTBC +1

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.

chr.fritsch made their first commit to this issue’s fork.

chr.fritsch opened merge request !1427

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs review » Needs work

I'm trying to fix this, but I didn't found the solution, yet. But at least I have written some tests.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review

I think I have it. Let's see if all tests will pass.

I copied all the stuff from the FileWidget. I will update the comments next week.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Media Library wrong order after adding item » Media library widget forgets ordering when adding or removing items
Issue tags: -Needs tests

@chr.fritsch added tests, so removing the "Needs tests" tag.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

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.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review

Fixed the nitpicks

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Looks pretty good to me! The test coverage is solid, and although the inner mechanics of what's going on here are hard to grok in the best of times (mostly because the field system is so arcane, especially with a complex, AJAX-heavy widget like the media library), there are at least comments with cross-references to other places so that another developer will hopefully be able to piece it together. I don't see any reason to block this fine bug fix any further.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 6470ab797cb to 10.0.x and 3e90900fc58 to 9.4.x. Thanks!

We're currently in a code freeze on production branches - will backport once that is over.

  • alexpott committed 6470ab7 on 10.0.x 
    Issue #3115054 by chr.fritsch, vsujeetkumar, Vidushi Mehta, sergiuteaca...

  • alexpott committed 3e90900 on 9.4.x 
    Issue #3115054 by chr.fritsch, vsujeetkumar, Vidushi Mehta, sergiuteaca...

  • xjm committed 3a8f5c1 on 9.3.x authored by alexpott 
    Issue #3115054 by chr.fritsch, vsujeetkumar, Vidushi Mehta, sergiuteaca...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Patch (to be ported) » Fixed

Cherry-picked to 9.3.x. We should probably set things to RTBC rather than PTBP if cherry-pick works so we don't lose them.

Status: Fixed » Closed (fixed)

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

Christopher Riley’s picture

Christopher Riley commented

I Cannot update to Drupal 9.3.11. When I try to do my composer update I get the following:

Gathering patches for dependencies. This might take a minute.
- Installing drupal/core (9.3.11): Cloning b8978dd6a4 from cache
- Applying patches for drupal/core
https://git.drupalcode.org/project/drupal/-/merge_requests/1427.diff (Media library widget forgets ordering when adding or removing items)
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/1427.diff

In Patches.php line 326:

Cannot apply patch Media library widget forgets ordering when adding or removing items (https://git.drupalcode.org/project/drupal/-/merge_requests/1427.diff)!

I have pulled all of the drupal/core patches from my composer file but still no luck. Any ideas as to what to do now?

cilefen’s picture

cilefen commented

This is fixed and released so the patch is no longer needed. That's how patches work.

cilefen’s picture

cilefen commented

We have a report that this causes a regression #3277737: When a user clicks on Add new media (multiple value field) inside an entity reference field (Block) within a content, all existing medias disappeared and are replaced by the new one uploaded.