Problem/Motivation

When adding new media through the Media Library, it's possible to add more than the allowed number of media items.

Steps to reproduce:
1) In the Media Library, if you can select two items, select two.
2) Now upload a third item.

Expected: You should not be able to select more than the allowed number, or should receive a warning if you do.
Actual: no warning appears, and if you then insert the media you get nothing is attached. It fails without warning.

Proposed resolution

  1. When the number of selected items exceeds the maximum allowed after adding one or more items in the media library, display a warning.
  2. If the user then persists on inserting the media items with an overage, do not allow it, but display an error message.

Video:
https://www.drupal.org/files/issues/2019-12-21/3082690-71-walkthrough_0.mov

Remaining tasks

  • Discuss solution
  • Write patch
  • UX Review
  • A11Y Review
  • Code review
  • Frontend framework manager review
  • Commit

User interface changes

Added error / warning messages to the media library.

Video of #21: https://www.drupal.org/files/issues/2019-07-17/media-library-exceeds-lim...

The warning after adding and exceeding the limit:
https://www.drupal.org/files/issues/2019-07-17/media-library-add-warning...

The warning after clicking 'Save and select':
https://www.drupal.org/files/issues/2019-07-17/media-library-save-and-se...

The error after clicking 'Save and insert' or 'Insert selected':
https://www.drupal.org/files/issues/2019-07-17/media-library-save-and-in...

API changes

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#182 upload-pdf-not-working-issues-3059955-182.patch7.21 KBSwayanka
#178 interdiff.txt1.86 KBlauriii
#176 interdiff.txt717 byteslauriii
#176 3059955-176.patch20.43 KBlauriii
#169 3059955-169.patch20.41 KBameymudras
#168 3059955-nr-bot.txt86 bytesneeds-review-queue-bot
#167 3059955-167.patch22.14 KB_utsavsharma
#167 interdiff_166-167.txt3.75 KB_utsavsharma
#166 3059955-d9.patch20.77 KBmei2020
#165 aftr_164.mov3 MBsonam.chaturvedi
#164 3059955-164.patch20.45 KBsmustgrave
#164 interdiff-159-164.txt1.69 KBsmustgrave
#162 aftrpat-multi-mediatype.png754.96 KBsonam.chaturvedi
#162 aftrpat-one-mediatype.png740.23 KBsonam.chaturvedi
#159 3059955-159.patch18.64 KBsmustgrave
#159 interdiff-156-159.txt4.75 KBsmustgrave
#156 interdiff-155.txt6.36 KBxjm
#156 ml-3059955-155-10.1.x.patch17.94 KBxjm
#154 3059955-146-101x.patch11.58 KBharshitthakore
#145 3059955-145-101x.patch17.94 KBbnjmnm
#144 3059955-144-9.4.x.patch1.69 KBaludescher
#143 reroll_diff_136-143.txt1.21 KBravi.shankar
#143 3059955-143.patch1.55 KBravi.shankar
#136 3059955-media-library-items-allowed-136.patch1.64 KBNievesWunder
#135 media-library-2of1-selected.png60.32 KBmErilainen
#131 3059955-131.patch2.23 KBNikolaAt
#130 test--3059955--mr-760-patch----after-unchecking-the-extra-media.png431.18 KBRajab Natshah
#130 test--3059955--mr-760-patch----on-upload-warning-message.png492.07 KBRajab Natshah
#129 3059955-129.patch2.19 KBNikolaAt
#127 media-library--after-the-fix.png303.36 KBRajab Natshah
#122 After patch Warning Message.png50.99 KBmanojithape
#122 Before Patch Files.png258.21 KBmanojithape
#112 3059955-112.patch16.78 KBnikunj.shah
#109 error_mesage_retains.png175.27 KBsaurabh.tripathi.cs
#107 3059955-107.patch16.79 KBpaulocs
#107 interdiff-101-107.txt2.85 KBpaulocs
#101 interdiff_96-101.txt550 bytesraman.b
#101 3059955-101.patch16.95 KBraman.b
#98 TestLocally.png13.45 KBpaulocs
#96 3059955-96.patch16.95 KBchr.fritsch
#96 interdiff-3059955-80-96.txt5.2 KBchr.fritsch
#80 3082690-80.patch17.01 KBrensingh99
#80 interdiff.txt1.08 KBrensingh99
#80 code_standard_error.png12.57 KBrensingh99
#80 Screenshot_5.png19.47 KBrensingh99
#80 Screenshot_4.png18.5 KBrensingh99
#80 Screenshot_3.png28.74 KBrensingh99
#80 Screenshot_2.png61.29 KBrensingh99
#80 Screenshot_1.png5.64 KBrensingh99
#78 3082690-78.patch17 KBoknate
#78 3082690--interdiff-77-78.txt3.28 KBoknate
#77 3082690--interdiff-76-77.txt5.07 KBoknate
#77 3082690-77.patch14.51 KBoknate
#76 3082690-76.patch9.44 KBoknate
#76 3082690--interdiff-74-76.txt4.63 KBoknate
#74 3082690-74.patch7.03 KBoknate
#74 3082690--interdiff-71-74.txt3.33 KBoknate
#71 safari-errors.png211.66 KBoknate
#71 3082690-71-walkthrough.mov21.07 MBoknate
#71 3082690-71.patch7 KBoknate
#70 2-file-maximum.png550.56 KBoknate
#68 3082690-68--reroll--minus-test-coverage.patch19.54 KBoknate
#62 last_step.png6.38 KBrensingh99
#62 got_warning_message.png37.02 KBrensingh99
#62 clicked.png34.23 KBrensingh99
#62 warning_message.png32.33 KBrensingh99
#62 choose_file.png39.74 KBrensingh99
#62 allowed_no.png5.22 KBrensingh99
#60 3082690-60--reroll.patch22.17 KBoknate
#58 Screenshot_1.png91.89 KBrensingh99
#55 diff_against_3082690-81___3087305026.txt16.41 KBbnjmnm
#55 3082690-55-REROLL.patch138.09 KBbnjmnm
#52 3082690-81--3087305-26--3059955--52.patch139.53 KBoknate
#52 3059955--52--do-not-test--NEEDS-WORK.patch17.56 KBoknate
#51 3059955-51--do-not-test.patch18.67 KBoknate
#51 3059955-51--combined-w--3087456-46.patch132.67 KBoknate
#51 3059955--interdiff-46-51.txt11.13 KBoknate
#50 3059955-50--do-not-test--reroll-of-46.patch21.59 KBoknate
#49 3059955-49--do-not-test--reroll-of-46.patch140.42 KBoknate
#46 interdiff-3059955-44-46.txt9.19 KBphenaproxima
#46 3059955-46-do-not-test.patch32.33 KBphenaproxima
#44 3059955-44.patch31.36 KBWim Leers
#42 3059955–42--combined-with--3086096-34.patch43.75 KBoknate
#42 3059955--42-do-not-test.patch31.21 KBoknate
#42 3086096--interdiff-38-42.txt3.46 KBoknate
#40 overflow-fix-demo-2-two-media-types.mov11.47 MBoknate
#40 overflow-fix-demo.mov4.91 MBoknate
#40 overflow-bug-demo.mov3.28 MBoknate
#38 3059955-38--combined-with--3086096-20.patch43.37 KBoknate
#38 3059955-38--do-not-test.patch30.73 KBoknate
#36 3059955-38-with-3081526-11.patch35.82 KBoknate
#36 3059955--interdiff-37-38.txt1.63 KBoknate
#35 3059955-36-with-3081526-11.patch36.62 KBoknate
#35 3059955--interdiff-34-36.txt812 bytesoknate
#34 3059955-34-with-3081526-11.patch36.44 KBoknate
#34 3059955--interdiff-29-34.txt21.51 KBoknate
#34 3059955--interdiff-32-34.txt8.65 KBoknate
#32 3059955-32-combined-with-ajax-command-from-3081526-11.patch36.02 KBoknate
#32 interdiff.txt16.21 KBoknate
#29 3059955-29.patch32.53 KBoknate
#29 3059955--interdiff-21-29.txt848 bytesoknate
#26 media-library-overflow.mp4935.55 KBseanB
#21 3059955-21.patch32.36 KBseanB
#21 interdiff-18-21.txt30.56 KBseanB
#21 interdiff-18-21.txt30.56 KBseanB
#21 media-library-save-and-insert.png1.09 MBseanB
#21 media-library-save-and-select.png1.09 MBseanB
#21 media-library-add-warning.png585.51 KBseanB
#21 media-library-exceeds-limit-warning-and-error.mp43.56 MBseanB
#18 media-max-allowed-items-message.mp42.99 MBseanB
#18 3059955-18.patch10.6 KBseanB
#17 image-field-more-uploaded.png126.11 KBseanB
#17 image-field-3-items.png12.32 KBseanB
#8 media-library-widget-with-limits-3059955-8.patch1.79 KBAndySipple
#6 media-library-tabs.mp4426.09 KBblazey
1.png223.95 KBNikolaAt
2.png127.08 KBNikolaAt
3.png208.17 KBNikolaAt
4.png133.93 KBNikolaAt

Issue fork drupal-3059955

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NikolaAt created an issue. See original summary.

NikolaAt’s picture

Issue summary: View changes
NikolaAt’s picture

Issue summary: View changes
NikolaAt’s picture

Issue summary: View changes
Lincoln-Batsirayi’s picture

I'm having the same issue as well, I've also got a media field inside a paragraph. The issue for me occurs when the apply filters button is clicked, it allow's you to select more than one image (which is wrong).

I've been able to work out that when the Apply filters button is clicked, the MediaLibraryWidgetRemaining behaviour inside the media_library module, is ran BUT with the context of the filters and not the whole view. I thought about writing some js to rerun this behaviour but it is using a .once inside it which means once its initially ran on page load, it doesn't load again.

blazey’s picture

FileSize
426.09 KB

Same here. The error shows up but only after the page is reloaded. Here's a video recorded on a clean 8.7.3 installation.

phenaproxima’s picture

Issue tags: -media library, -widget, -Allowed number of values
AndySipple’s picture

This appears to be an issue with the media library 'Drupal.behaviors.MediaLibraryItemSelection' in 'core/modules/media_library/js/media_library.ui.js'. Switching over to another media tab from the primary one doesn't update the active tabs items.
The issue seems to be coming from this chunk of code:

 $('#media-library-modal-selection', $form).once('media-library-selection-change').on('change', function (e) {
    updateSelectionCount(settings.media_library.selection_remaining);
​
    if (currentSelection.length === settings.media_library.selection_remaining) {
     disableItems($mediaItems.not(':checked'));
     enableItems($mediaItems.filter(':checked'));
    } else {
     enableItems($mediaItems);
    }
   });

The selections logic when switching tabs doesn't follow over.
Copying the code inside that on change function above it fixes the issue.

mikelutz’s picture

Priority: Major » Normal
Status: Active » Needs work
Issue tags: +Needs tests

Setting to NW for test coverage.

NikolaAt’s picture

The patch is working 50%. When I select media and change the filter, I am not able to select another. This is very well. Thanks.
The problem that, when you select media and choose to add file from you local system, the two files are selected and you can select more. This is not ok.

phenaproxima’s picture

Title: Media library widget doesn't work correct with media limitation number. » When creating media in the media library, it is possible to overflow the number of items than allowed by the field
Issue tags: +Media Initiative

Re-titling to make it a bit clearer what's going on.

phenaproxima’s picture

I think the problem here is that the file upload field does not validate the number of incoming files. You can set a managed_file element to either allow one file, or many, but there's no way to limit it to a specific number of files.

Although, such validation does already exist in core, in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::validateMultipleCount(). But that's tied to the file field widget, rather than the managed_file element itself. Since the media library doesn't use the file field widget, it is affected by this problem.

I think this is something we should fix in the managed_file element itself. We could add a property which forces it to validate the number of uploaded files, if the #multiple property is set, and change the FileWidget code to use this property.

effulgentsia’s picture

There might be things about #12 that's worth opening a separate issue for, but I don't think it's the root cause of this issue.

I think the root of this issue is: if you already have the maximum number of items selected, what should the "Add file" portion of the media library do? Should it be disabled until you've unselected something? Should it let you upload more but after upload not have them selected? Or something else? I don't see how adding more validation to the managed_file element type is part of solving that.

phenaproxima’s picture

Issue tags: +Usability

The more I think about it, the more I think I agree with @effulgentsia. I think this is primarily a usability issue and should be brought before the UX team so we can find a solution to this problem.

phenaproxima’s picture

Issue tags: +Accessibility

This was discussed in the usability meeting on July 2nd, 2019: https://www.youtube.com/watch?v=x3ztx9_kT88

In what has got to be the single fastest UX meeting consensus in history, we reached an agreement on how to approach this:

  • The upload field will no longer limit the amount of uploaded items. It'll take as many files as you want to throw at it. So, while using the media library, you can always add an unlimited number of things to it. This is consistent with our fundamental design assumption that "what happens in the media library, stays in the media library".
  • If you have already selected the maximum number of files prior to uploading (or if your previous selections + your uploaded items will exceed the limit), you will not be given the option to immediately save your new uploads and insert them into the entity reference field. Instead, you will only be given the option to return to the media library.
  • When you return to the media library, the "insert" button will be disabled, and you will see a message (perhaps a warning?) explaining that you have chosen the maximum number of items and now need to trim down your selection. The stuff you have uploaded, and previously selected, will be selected; everything else will be greyed out.
  • Once your selection is within the limits, the message will disappear, the "insert" button will be re-enabled, and items will no longer be greyed out (assuming you have any more wiggle room left to make more selections).

This will probably have accessibility implications too, so tagging for that.

AaronMcHale’s picture

Re #15:

Thanks @phenaproxima for summariesing the discussion. Just regarding:

If you have already selected the maximum number of files prior to uploading (or if your previous selections + your uploaded items will exceed the limit), you will not be given the option to immediately save your new uploads and insert them into the entity reference field. Instead, you will only be given the option to return to the media library.

The way I was picturing it during the UX call was that once the items were uploaded you would also get the warning on the uploaded screen and the button to save and insert would just be disabled not totally gone - I think that would be better from a UX perspective because it avoids the issue of the user wondering why the button they were expecting to see isn't there.

Essentially the logic should look like: Has the user gotten themselves into a situation where they have selected more items than the field allows (currently selected + currently being uploaded)? If yes, display a warning message on all of the Media Library Dialogue screens indicating how many items over the limit they are (update the warning each time their total selection changes, warning message should be contextually appropriate for each screen, so would be worded differently on selection screen vs upload screen), and disable any buttons which would result in the media being inserted until they are under the limit.

Thanks

seanB’s picture

Added the tag for A11Y review since I'm a bit worried disabling the 'Insert selected' button could be an issue.

We could potentially show a message in the modal and not disable the button, and trim some of the items when inserting in the widget (and show another message we did that. Not saying this is perfect, but this is also what the "regular" image field does. Specially if we already show a warning when the users gets in this situation (after uploading too many items), this might be better than disabling the button.
When a user has selected multiple items over multiple tabs, and also when using pagers/filters, requiring a user to deselect stuff in the modal could be difficult until we have #3023809: Add a selection overview to the media library widget modal.

The "regular" image field currently already allows more uploads when the cardinality is > 1. Below the screenshots of what that looks like.
Image field with max 3 items.

After uploading 5 items:
Image field trimmed the last 2 items.

seanB’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs test
FileSize
10.6 KB
2.99 MB

Here is a first patch to implement #15. And a nice video showing it :)
Tests are still missing.

Status: Needs review » Needs work

The last submitted patch, 18: 3059955-18.patch, failed testing. View results

andrewmacpherson’s picture

I'm not keen on the disabled buttons idea at all.

The disabled state will not be obvious, for a lot of sighted users.

  • The visual distinction between a disabled button and enabled button isn't clear. This is generally the case on the Web as a whole, and certainly the case with the Seven theme.
  • In the video from #18 it's hard to tell visually that the "Save and insert" button is disabled (0:35 - 0:45). It's next to an enabled blue-primary button. The disabled "save and insert" button is has lighter font weight, and a paler border. However there isn't a normal button visible to compare it to. It's not clear whether the difference in font-weight and colour are because it's disabled, or whether that's just because it's not a primary button.
  • In the video from #18, the "insert selected" button is disabled (0:55) but there isn't an enabled button visible to compare it to.
  • The "add media" button is visible in the underlying page, but the shaded overlay outside of the dialog area conceals it's regular appearance, so it doesn't suffice as a comparison with a disabled button in the dialog.
seanB’s picture

We have discussed this in the UX meeting yesterday: https://youtu.be/TlYSuIxU8ZA?t=532

Glad andrewmacpherson could join us to discuss and explain the accessibility implications of the disabled buttons. Couple of things we agreed upon:

  • The disabled buttons are a potential problem for A11Y (for multiple reasons), so we should not disable any buttons.
  • The warning message was helpful.
  • When the user tries to insert items when the number of selected items exceeds the limit, the warning message should be shown as an error. Users should stay in the media library and see the error until this is fixed.
  • We should create a followup to see if we can improve the position of the error message on the page.

Attached are a new video (https://www.drupal.org/files/issues/2019-07-17/media-library-exceeds-lim...), some screenshots and a patch to implement what we have discussed (including tests). Removing the tags for tests and A11Y review, but adding a tag for frontend framework manager review because of all the JS changes I had to make.

Screenshot of the warning after adding and exceeding the limit:
Warning message shown after exceeding the limit.

Screenshot of the warning after clicking 'Save and select':
Warning message shown after clicking 'Save and select'.

Screenshot of the error after clicking 'Save and insert' or 'Insert selected':
Warning message shown after clicking 'Save and insert'.

*EDIT

+++ b/core/modules/media_library/js/media_library.ui.es6.js
@@ -2,6 +2,7 @@
+  debugger;

Crap, missed that, will fix on next patch!

phenaproxima’s picture

This is really nice. Great work, @seanB!

I gave this patch a quick read and I think my main concern is the way the messages are handled. Honestly, there should be a generic AJAX command which can manipulate a Drupal.Message instance on the page. I don't know if that's a thing we need to block on and do in a separate issue, but I think it should be implemented correctly here the first time so that we don't have to bother with backwards compatibility stuff in follow-ups.

So what I think I'd prefer here is something like this:

  1. First, both the media library selection and the add form should receive a CSS class like js-media-library-messages-target.
  2. Upon initialization, the media library's JavaScript should always create an empty message wrapper element and prepend it to the first available js-media-library-messages-target element.
  3. We should add a new AJAX command, \Drupal\Core\Ajax\AddMessageCommand, which can add messages to a Drupal.Message instance associated with a particular selector. It should also have an option to clear existing messages.
  4. This is the command we should use when returning errors.

So, the JavaScript side:

// In JavaScript, always do this unconditionally.
$('.js-media-library-messages-target').prepend('<div class="js-media-library-messages"></div>');

Drupal.AjaxCommands.prototype.addMessage = function (response) {
  const messages = new Drupal.Message(document.querySelector(response.selector));
  messages.add(response.message, response.id, response.type);
}

And on the PHP side:

use Drupal\Core\Ajax\AddMessageCommand;

return (new AjaxResponse())
  ->addCommand(new AddMessageCommand('.js-media-library-messages', 'You chose too many things!', 'limit exceeded', 'error')->clearAll());
lauriii’s picture

  1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -2,6 +2,7 @@
    +  debugger;
    

    ✌️

  2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -15,6 +16,47 @@
    +    $('<div class="js-media-library-messages" data-drupal-messages/>')[0],
    

    Is there a specific reason why we're adding the data-drupal-messages here? It shouldn't be necessary because we are already passing the message wrapper to the constructor.

AaronMcHale’s picture

Been on holiday last couple of weeks, so haven't been keeping up.

I am very happy with displaying an error message instead of disabling the buttons, it seems to avoid the accessibility issues while still preventing the user from getting into an overflow state, great work :)

One small concern is that in the demo video while on the upload screen: when the user clicks the "Save and insert" button, they are taken to the media library screen if the error is triggered. This I think could be improved by not navigating away from the upload screen and instead displaying the error on that screen, this I think would provide a more consistent UX.

Thanks
-Aaron

xjm’s picture

Priority: Normal » Major

When you have too many items attached, what happens when you try to save the form? Is there a validation error? (This affects whether this is "only" a major UX bug or actually a blocking data integrity problem.)

seanB’s picture

FileSize
935.55 KB

One small concern is that in the demo video while on the upload screen: when the user clicks the "Save and insert" button, they are taken to the media library screen if the error is triggered. This I think could be improved by not navigating away from the upload screen and instead displaying the error on that screen, this I think would provide a more consistent UX.

That would mean you are not able to save the added media items, and forces the user to delete instead of de-select. I think saving their work first, and them allowing them to correct the selection is a lot nicer.

When you have too many items attached, what happens when you try to save the form? Is there a validation error? (This affects whether this is "only" a major UX bug or actually a blocking data integrity problem.)

See the attached video of what happens when trying to insert too many item. The widget doesn't accept your selection. In the background and error message is generated but not shown. And when you re-open the library you see the error. It's quite the WTF.

bnjmnm’s picture

From #22.3

We should add a new AJAX command, \Drupal\Core\Ajax\AddMessageCommand, which can add messages to a Drupal.Message instance associated with a particular selector. It should also have an option to clear existing messages.

I happened to see this a few hours after adding that functionality (with tests) to a separate issue: #3081526: Add announcement after clicking 'Save and select' in the media library.

If that issue gets stalled for some reason, we could create a separate issue for just the AJAX command so it can be made available here and elsewhere.

Wim Leers’s picture

oknate’s picture

Reroll of #21, as it doesn't currently apply to HEAD.

oknate’s picture

Title: When creating media in the media library, it is possible to overflow the number of items than allowed by the field » [PP-1] When creating media in the media library, it is possible to overflow the number of items than allowed by the field

Marking postponed on #3086096: Add a generic Ajax Message command

@bnjmnm already implemented the requested message command here #3081526-22: Add announcement after clicking 'Save and select' in the media library., but I think this should be moved to a separate non-media-library specific issue.

Status: Needs review » Needs work

The last submitted patch, 29: 3059955-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

oknate’s picture

I have tested out #21 and it makes some nice improvements. It needs some work on the Ajax returned messages. I'm getting double messages right now.

I updated the code based on the feedback in #22. I attach the message container when the library is loaded up and attach the message container to both places so we can drop the conditional placement code.

This includes the relevant parts of @bnjmnm's patch for #3081526-11: Add announcement after clicking 'Save and select' in the media library. that adds the new Message ajax.

I'll keep working on it tomorrow morning, unless Sean B or Bnjmjm wants to take another look.

oknate’s picture

Title: [PP-1] When creating media in the media library, it is possible to overflow the number of items than allowed by the field » [PP-1] It is possible to overflow the number of items allowed in Media Library
oknate’s picture

I was able to fix the repeating messages by adding an option to the Ajax command to clear out the previous messages:

diff --git a/core/misc/ajax.es6.js b/core/misc/ajax.es6.js
index 18db6412c5..e4ba97add4 100644
--- a/core/misc/ajax.es6.js
+++ b/core/misc/ajax.es6.js
@@ -1578,11 +1578,16 @@
      *   The message text.
      * @param {string} response.messageOptions
      *   The options argument for Drupal.Message().add().
+     * @param {bool} response.clearPrevious
+     *   If TRUE, clear previous messages.
      */
     message(ajax, response) {
       const messages = new Drupal.Message(
         document.querySelector(response.messageWrapperQuerySelector),
       );
+      if (response.clearPrevious) {
+        jQuery(response.messageWrapperQuerySelector + ' .messages').remove();
+      }
       messages.add(response.message, response.messageOptions);
     },
   };

The repeating messages happen when the ajax response adds a message, it doesn't delete the previous ones.

I didn't go with the suggestion in #22 ->clearAll(), as I didn't see any other Ajax commands that do similar things.

One thing I preferred in #21 was that it took the message type directly as an argument rather than as an option:
$response->addCommand(new AddMessageCommand($message, 'maxItemsExceeded', 'error'));
rather than
$response->addCommand(new MessageCommand($message, '.js-media-library-messages', ['type' => 'error']));
But perhaps it's better to allow an array of options for those who want to override it.

Another change that's worth mentioning. I was having trouble preventing double '.js-media-library-messages ' wrapper elements from being added, and problems with the AJAX not adding messages to it, I guess because it gets replaced?

So I'm testing out adding it on the back end, like so:

@@ -137,7 +139,7 @@ protected function getMediaType(FormStateInterface $form_state) {
   public function buildForm(array $form, FormStateInterface $form_state) {
     // @todo Remove the ID when we can use selectors to replace content via
     //   AJAX in https://www.drupal.org/project/drupal/issues/2821793.
-    $form['#prefix'] = '<div id="media-library-add-form-wrapper" class="media-library-add-form-wrapper">';
+    $form['#prefix'] = '<div id="media-library-add-form-wrapper" class="media-library-add-form-wrapper js-media-library-add-form-wrapper"><div class="js-media-library-messages"></div>';
     $form['#suffix'] = '</div>';
     $form['#attached']['library'][] = 'media_library/style';
 
oknate’s picture

oknate’s picture

The last change was breaking paging. I didn't investigate, but I suspect because I was trying to use ['#prefix'] in the viewsForm for the messages. Also, the class ''js-media-library-messages-target' isn't being used now, so removing that.

diff --git a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
index 5dfd07ea4e..1fd6c11779 100644
--- a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
+++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
@@ -51,10 +51,11 @@ public function render(ResultRow $values) {
    */
   public function viewsForm(array &$form, FormStateInterface $form_state) {
     $form['#attributes'] = [
-      'class' => ['media-library-views-form', 'js-media-library-views-form', 'js-media-library-messages-target'],
+      'class' => ['media-library-views-form', 'js-media-library-views-form'],
     ];
 
-    $form['#prefix'] = '<div class="js-media-library-messages"></div>';
+    $form['media_library_messages'] = ['#markup' => '<div class="js-media-library-messages"></div>'];
+

oknate’s picture

My previous patch was missing MessageCommand::clearPrevious code. See #3086096-11: Add a generic Ajax Message command, without that, duplicate messages will display.

oknate’s picture

oknate’s picture

Status: Needs review » Postponed
oknate’s picture

Simplifying the "Problem/Motivation" portion of the IS and adding before/after videos.

oknate’s picture

Issue summary: View changes
oknate’s picture

oknate’s picture

#3086096: Add a generic Ajax Message command is RTBC, so one that goes in, this will need a reroll. This could use a review from @phenaproxima that his feedback in #22 has been adequately addressed.

Wim Leers’s picture

Title: [PP-1] It is possible to overflow the number of items allowed in Media Library » It is possible to overflow the number of items allowed in Media Library
Assigned: Unassigned » phenaproxima
Status: Postponed » Needs review
Issue tags: -Needs tests
FileSize
31.36 KB

#3086096: Add a generic Ajax Message command landed, this is now unblocked! Assigning to @phenaproxima per #43.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -15,6 +15,45 @@
    +  function addMediaLibraryMessage(message, messageId, messageType) {
    +    const wrapper = document.querySelector('.js-media-library-messages');
    +
    +    if (!wrapper) {
    +      return;
    +    }
    +
    +    const messages = new Drupal.Message(wrapper);
    +    messages.clear();
    +    messages.add(message, {
    +      type: messageType,
    +      id: messageId,
    +    });
    +  }
    +
    +  /**
    +   * Clear messages.
    +   */
    +  function clearMediaLibraryMessages() {
    +    const wrapper = document.querySelector('.js-media-library-messages');
    +
    +    if (!wrapper) {
    +      return;
    +    }
    +
    +    const messages = new Drupal.Message(wrapper);
    +    messages.clear();
    +  }
    

    Do we really need these? They seem like very thin, and potentially unnecessary, wrappers around Drupal.Message.

    What might be better, if we need to have these, is make them helper methods in Drupal.MediaLibrary.

  2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -319,10 +371,13 @@
    +        const count =
    +          currentSelection.length +
    +          $('.js-media-library-add-form__media').length;
    

    This is a nice fix. 👍

  3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -368,25 +423,44 @@
    +          const selectedMediaCount =
    +            currentSelection.length +
    +            $('.js-media-library-add-form__media').length;
    

    This code is repeated elsewhere, which is not great. Ideally we'd be able to make it a method of Drupal.MediaLibrary, callable like so:

    Drupal.MediaLibrary.getSelectedCount();
    
  4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -471,6 +473,9 @@ protected function buildActions(array $form, FormStateInterface $form_state) {
    +        '#attributes' => [
    +          'class' => ['js-media-library-select-button'],
    +        ],
    

    What is this class used for?

  5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -479,6 +484,9 @@ protected function buildActions(array $form, FormStateInterface $form_state) {
    +        '#attributes' => [
    +          'class' => ['js-media-library-insert-button'],
    +        ],
    

    Same here.

  6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -741,12 +749,34 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
    +    $available_slots = $this->getMediaLibraryState($form_state)->getAvailableSlots();
    +    $selected_media_count = count($selected_media);
    +    if ($available_slots > 0 && $selected_media_count > $available_slots) {
    

    We should be using MediaLibraryState::hasSlotsAvailable(), not checking the value of getAvailableSlots(). So this should look more like:

    $state = $this->getMediaLibraryState($form_state);
    $selected_media_count = count($selected_media);
    $available_slots = $state->getAvailableSlots();
    
    if ($state->hasSlotsAvailable() && $selected_media_count > $available_slots) { ... }
    
  7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -741,12 +749,34 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
    +      // The message ID should be the same as the client side ID.
    +      // @see Drupal.behaviors.MediaLibraryItemSelection
    +      $response->addCommand(new MessageCommand($message, '.js-media-library-messages', ['type' => 'error']));
    

    What message ID?

  8. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -162,6 +162,7 @@ protected function buildLibraryContent(MediaLibraryState $state) {
           '#attributes' => [
             'id' => 'media-library-content',
             'class' => ['media-library-content'],
    +        'tabindex' => -1,
    

    What is the reason for this change? This may necessitate accessibility review, if needed, and might need manual testing.

  9. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -103,7 +108,7 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    -      'class' => ['media-library-select'],
    +      'class' => ['js-media-library-insert-button'],
    

    Are we using this for anything? This change might also constitute a front-end BC break in a stable module, so I'm not sure we should change it unless we need to. If we must, then we should add the js-media-library-insert-button class without removing the other one.

  10. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -125,26 +130,47 @@ public static function updateWidget(array &$form, FormStateInterface $form_state
    +    // When the total number of selected items after saving exceeds the number
    +    // of allowed items, we show an error message. When the remaining number of
    +    // items is a negative number, we allow an unlimited number of items. In
    +    // that case we can always insert the selected items. Since the message
    +    // needs to be shown outside of the view, we use a custom AJAX command to
    +    // add the message to the media library.
    +    $available_slots = $state->getAvailableSlots();
    +    if ($available_slots > 0 && $selected_media_count > $available_slots) {
    +      $diff = $selected_media_count - $available_slots;
    +      $message = new PluralTranslatableMarkup($diff, 'There are currently @total items selected. The maximum number of remaining items for the field is @max. Please remove @count item from the selection.', 'There are currently @total items selected. The maximum number of remaining items for the field is @max. Please remove @count items from the selection.', [
    +        '@total' => $selected_media_count,
    +        '@max' => $available_slots,
    +      ]);
    +
    +      $response = new AjaxResponse();
    +      // The message ID should be the same as the client side ID.
    +      // @see Drupal.behaviors.MediaLibraryItemSelection
    +      $response->addCommand(new MessageCommand($message, '.js-media-library-messages', ['type' => 'error']));
    +      return $response;
    +    }
    

    So, here's the thing.

    This is the third time we are effectively repeating this logic in this patch. Once in the add form, once in the view selection, and once in JavaScript. That's a lot of repetition, and a clear sign that our approach is probably not optimal.

    At the moment, I'm not 100% sure how to handle this. But I'm leaning towards doing this in the opener's getSelectionResponse() method. That method is responsible for building the actual AJAX response and is given the media library state and the selected item IDs -- including newly added ones, I believe -- so it would be trivial for us to add a base class that both existing openers would extend, and which would do this validation.

    I can write a proof of concept for this if you want, but I think this is my single biggest complaint with the patch as it currently exists.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
32.33 KB
9.19 KB

Here's a do-not-test PoC of what I'm talking about.

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.

oknate’s picture

Title: It is possible to overflow the number of items allowed in Media Library » [PP-1] It is possible to overflow the number of items allowed in Media Library

Postponing on #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate, which refactors Media Library widget and tests significantly, and will probably be committed before this one.

oknate’s picture

ignore this one, I failed to pull latest changes in the branch I was diffing against.

oknate’s picture

FileSize
21.59 KB

Reroll of 46. I'm dropping the test coverage temporarily in this patch. I'll reroll this on top of #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate, then add the coverage back.

oknate’s picture

Addressing #45
1. ✅ Since we’re using the new functions in only one place, I think for now, it’s better to drop them.
3. ¯\_(ツ)_/¯ I’m not sure what to do with this feedback. This seems redundant:

    getSelectedCount: function () {
      return this.currentSelection.length;
    }

since you could just check Drupal.MediaLibrary.currentSelection.length, and this seems out of place within the class:

    getSelectedCount: function () {
      return this.currentSelection.length + $('.js-media-library-add-form__media').length;
    }

Can you clarify why this is better than handling it outside the class. It seems to me, in the context of the add form, the new items should be calculated at that moment.
4. ✅ That class isn’t used, removing.
5. ✅ That class isn’t used, removing.
6.I changed this to use a new method ::hasUnlimitedSlotsAvailable(). I think this is worth adding even if we're using it one place for now. It allows for more readable/comprehensible code, without having to explain again that "a negative integer" means unlimited. it was checking $available_slots because ::hasSlotsAvailable() doesn't quite work. We need this to run if there are zero or more slots available. To make this more explicit I want to add hasUnlimitedSlotsAvailable(), because that's the only condition in which this shouldn't be validated. Doing $state->getAvailableSlots() < 0 is confusing. I think adding a method makes the logic clearer. This could be dropped if you think it's a bad idea and a comment could explain the logic.

+  protected function validateSelection(MediaLibraryState $state, array $selected_ids) {
+    if ($state->hasUnlimitedSlotsAvailable()) {
+      return;
+    }
     $available_slots = $state->getAvailableSlots();
     $selected_count = count($selected_ids);
 
-    if ($available_slots > 0 && $selected_count > $available_slots) {
+    if ($selected_count > $available_slots) {

7. ✅ The message ID was a vestige of an earlier iteration of the patch, added in 21. The new MessageCommand doesn’t use this parameter. So removing references to it.
8. ✅ Removing tab index change. That was added by me in 29, which was a reroll of 21, which didn’t have it. So I think it was patch noise.
9. ✅ ‘js-media-library-insert-button’ isn’t being used. I asked Sean B about this a while back and he said that it was to make it clearer which button was being pushed, butt that it was probably from iterating on the patch, and therefore can be removed.
10. @todo still need to evaluate your changes. I’m renaming the validate function to ::validateSelection(). It’s more general, in we want to add validation later on that isn’t related to available slots.

@todos:

  • Need to add back test coverage.
  • Need to test. The changes I made today weren't tested yet.
oknate’s picture

phenaproxima’s picture

Issue tags: +Amsterdam2019

Tagging this extremely important bug fix to be worked on at DrupalCon Amsterdam.

bnjmnm’s picture

Assigned: phenaproxima » bnjmnm

Assigning to myself for a bit as I'm working on the reroll (not the easiest I've encountered...) and want to be sure nobody else has to duplicate this effort.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs work » Needs review
FileSize
138.09 KB
16.41 KB

Here's a beast of a reroll 🐻

The patch includes #3082690-81: Mark Media Library as a stable core module and #3087305-26: Form validation error messages within the Media Library widget are not read by the screenreader so I also provided an diff against a branch with those two applied to provide an easy way to identify the changes specific to this issue.

Unassigning myself but can't re-assign to @phenaproxima who was the previous assignee

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Re-assigning to myself to review.

phenaproxima’s picture

Title: [PP-1] It is possible to overflow the number of items allowed in Media Library » It is possible to overflow the number of items allowed in Media Library

Media Library is now stable, so removing the PP-1 designation.

rensingh99’s picture

Status: Needs review » Needs work
FileSize
91.89 KB

Hi,

I tried patch #55 and it fails to apply with core 8.9.x. So, I am changing the status to "Needs Work".

Below is an error screenshot.

Thanks,
Ren

phenaproxima’s picture

Issue tags: +Needs reroll

Tagging for reroll.

oknate’s picture

Reroll, sorry about the issue number being wrong in in the patch filename. I don't know where I got that from!

oknate’s picture

Status: Needs work » Needs review
rensingh99’s picture

Hi,

I have reviewed the patch #60. Below is my update.

There are allowed no of an item to upload is one.

Then I have selected one image from uploaded media and clicked on "Choose File" to select another image.

I have not gotten a warning message as I saw in the video.

Then I clicked on the "Save and select" button.

Then I had got the warning message.

But when I clicked on the "insert selected" button then I didn't get an error message as I saw in the video.

Below is an output screenshot after clicking on the "insert selected" button.

So, it's not working as I saw in the video.

Thanks,
Ren

oknate’s picture

Status: Needs review » Needs work

Marking back to needs work based on Ren's review. The error message when the user has inserted more than the allowed number needs to be restored. It was working in an earlier version of the patch.

fkelly12054@gmail.com’s picture

Testing locally on 8.8 RC1. In Home/Administration/Structure/Media types/ Edit Image/Manage fields/ Image I set the allowed number of values to unlimited. Tried to upload seven images. Only the first one shows up in the media library. This first file shows up in the table media_field_data together with two other Image files I had uploaded (one file at a time) earlier. The other files do show up in sites/default/files/2019_12 and in the filename field of the file_managed table. So, they are getting uploaded and stored but not incorporated into the media library.

Interesting. Went into reports/recent log messages. See the line:
media 12/02/2019 - 15:08 image: added _MG_6080.jpg. frank View

The _MG_6080.jpg is the first file in the group of files I tried to upload. There are no messages for the other files. But If I click on View in that line it shows the first file and then the other six files I uploaded in the same batch.

fkelly12054@gmail.com’s picture

Behavior reported in #64 running RC1 is basically unchanged in 8.8.0 official release. Multiple files do get uploaded into sites/default/files ... but only one at a time gets put in the media library.

If this is confirmed maybe it should be mentioned more prominently in some of the release notes? It's going to take a cleanup effort when the behavior is fixed for users to go back through and delete files in the files directory that were added but not included in the media library so they can get them re-added and included.

phenaproxima’s picture

Home/Administration/Structure/Media types/ Edit Image/Manage fields/ Image I set the allowed number of values to unlimited. Tried to upload seven images.

I don't mean to sound like a jerk, so I apologize if I come off that way here, but this sounds like a totally separate problem from what this issue is about.

In fact, I think the behavior you describe might be intentional. Media Library was not meant to put multiple images into a single media item -- it creates a new media item for each image you upload, because that keeps the individual images reusable and is very much the "80% use case". As far as I know, attaching multiple images to a single media item is uncommon, and having Media Library support that out of the box could heavily compromise the reusability that is the main purpose of the media library. It would likely some very significant usability and accessibility questions as well, not to mention contradict some of the media library's fundamental design decisions.

That said, what you are asking for is very likely doable in a contrib module -- I can totally imagine something like Media Entity Slideshow needing to support a use case like this -- but I'm not sure it would be in scope for core. (At the very least, if you want to propose it as a feature for core, you are certainly welcome to open a separate issue to talk about that.)

oknate’s picture

The @todo here is to fix it so it works again like the video in the issue summary:
https://www.drupal.org/files/issues/2019-10-08/overflow-fix-demo.mov

Also, we need to restore test coverage, or add it.

oknate’s picture

Here's a reroll of 60, minus the test coverage which doesn't apply. The @todos in #67 are still pending.

oknate’s picture

I tested this a little tonight.

1. In the advanced UI, using select and insert it no longer prevents you from submitting when you have selected more than the allowed amount after uploading. The original bug is present where it submits, but then fails silently and the field displays 0 of 2 remaining. It should switch from a js error (yellow) to a form error (red). This was working in an earlier patch.

2. Removed this comment. I remember now what hasUnlimitedSlotsAvailable is for.

3. I think the behavior is pretty broken with the standard workflow (where there's just a save button). We were working on this before that was the default. Some of the behavior such as error messages don't appear in this workflow after upload: "There are currently 3 items selected. The maximum remaining for the field is 2. Please remove 1 item from the selection".

4. With the standard UI, if you have a cardinality of 2, and you select 2, it still displays 'Maximum 2 files.' under the add form field, and if you add more than 2 it fails and displays an error.
https://www.drupal.org/files/issues/2019-12-21/2-file-maximum.png
I think the idea here is now that you can upload as many as you want, so that should be fixed. The limit should basically never be there, once validation prevents you from inserting more than the limit in the media library view. It should always be unlimited in the add form, but should display a warning when you upload more than the allowed amount in the upload form. So it should let you upload three if you have already selected 2, and after they are uploaded it should display "There are currently 5 items selected. The maximum remaining for the field is 2. Please remove 3 items from the selection". If you then hit the insert button from the media library view, it should change to an error.

oknate’s picture

FileSize
550.56 KB
oknate’s picture

Status: Needs work » Needs review
FileSize
7 KB
21.07 MB
211.66 KB

Given how broken the patch currently is, I wanted to build up a new patch, with apologies to what has come before. I kept the error message, but I'm outputting it differently. It seems to work (in Chrome, see below) and it's much simpler. (Update: it works in Safari, I just needed to upgrade, see below).

I started by changing the file upload to allow an unlimited number of uploads (#69.4). Then I was able to find a different place to output the errors, in AddFormBase::UpdateLibrary. For the 'Select and Insert', which calls AddFormBase::updateWidget, we'd need to repeat this logic, and I came up with the (smart, if I do say so myself) idea to just call AddFormBase::UpdateLibrary when the number is exceeded, which means we don't need to reuse the code that outputs the warning for both buttons (Yeah!). This works for both the Advanced UI and the basic UI.

For the error when actually trying to submit the media library with an overage, I had to copy the logic into MediaLibrarySelectForm::updateWidget(). There may be a way to share the error message code between the two classes. I'm not sure it's worth it, as it's just one message, and it's an error in this case, but a warning in the other. But the message text is the same. An elaborate solution to prevent sharing the code may be more convoluted, and therefore not worth it, but I'm open to ideas there.

One fact that amazes me here, is that I don't use javascript at all, and I think I've covered all the use cases. Perhaps I'm missing something?

If not, then I can just work on adding the test coverage back.

I tried to test with VoiceOver on Safari, and the whole thing is busted due to a javascript error (see screenshot).
https://www.drupal.org/files/issues/2019-12-21/safari-errors.png
I was able to confirm this happens without the patch, so I created a separate issue for it: #3102572: Media Library upload broken in Safari versions less than 10.3

Here's a walkthrough of this patch:
https://www.drupal.org/files/issues/2019-12-21/3082690-71-walkthrough_0.mov

oknate’s picture

Issue summary: View changes

Updated the wording of the proposed solution slightly.

Status: Needs review » Needs work

The last submitted patch, 71: 3082690-71.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

oknate’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
FileSize
3.33 KB
7.03 KB

Fixed a bug in the approach from #71 with unlimited cardinality. Let's see what tests fail now.

Also, I tested with VoiceOver and Safari (with the patch for #3102572: Media Library upload broken in Safari versions less than 10.3), and I found that because the message is in the same place and has the same text, it wasn't read out when it switched from a warning to an error. I found if we change the text slightly, it will read out. So I just added the preface 'Error: '. I don't love this solution. I think perhaps we could update Drupal.announce to handle when a message is the same but switches from a warning to an error. But perhaps that could be a follow-up. It's better to have the message read out for now.

Status: Needs review » Needs work

The last submitted patch, 74: 3082690-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

oknate’s picture

1) I'm reverting the addition of the preface 'Error: ' to the error message. I was testing the screen reader functionality on a very old version of Safari (Safari 9). I upgraded my OS and Safari, so now I'm testing on Safari 13, and this just works now, i.e. it reads out the error message despite the text being identical to the warning. It also reads the Aria label first, so it prefaces the message with "Error message". So I don't need the work around added in 74.
2) I updated the tests to account for the fact that the approach in #71 allows unlimited uploads. We could move this change to a separate issue, if it proves contentious, because it not essential to this issue. But I think they go hand in hand. Adding validation of the number to be inserted means it's not necessary to return a form error when uploading more than the displayed maximum number of uploads.
3) This change addressed a random failure:

-    $assert_session->hiddenFieldValueEquals('current_selection', '');
+    $page->waitFor(10, function () use ($page) {
+      return $page->find('hidden_field_selector', ['hidden_field', 'current_selection'])->getValue() === '';
+    });

3) I want to make one unnecessary change:

-    $current_media_ids = array_map(function (MediaInterface $media) {
+    $media_ids = array_map(function (MediaInterface $media) {

This local variable is named differently in MediaLibrarySelectForm::updateWidget vs AddFormBase::updateLibrary. I'd like them to use the same variable name, $media_ids. I don't feel too strongly about this, and to reduce the amount of changes, we could allow leave it alone.

oknate’s picture

Status: Needs work » Needs review
FileSize
14.51 KB
5.07 KB

Adding test coverage for the approach in #71-76. I use a data provider to demonstrate that the behavior is the same with the advanced UI, i.e. all three button options in the upload form.

One thing we haven't tested yet is that it works as expected with a field with cardinality of -1.

oknate’s picture

Assigned: Unassigned » oknate
FileSize
3.28 KB
17 KB

Adding test coverage for unlimited cardinality. Basically demonstrating the new validation fields with unlimited cardinality.

I think this is in a good place for now. I'm assigning to myself in case any changes are needed.

oknate’s picture

Issue summary: View changes

Replacing video in the issue summary to use the new placement.

I'm leaving the screenshots alone for now. Before #71, the messages appeared in two places. Now I'm putting them both above the thumbnails in the view for both the warning and the error. I'll wait for feedback on the slight change of approach before updating that part of the issue summary.

rensingh99’s picture

Hi,

I have reviewed the patch #78. Below is my update.

There are allowed no of an item to upload is one.

Then I have selected one image from uploaded media and clicked on "Choose File" to select another image.

Then I clicked on the "Save and select" button.

Then I had got the warning message.

when I clicked on the "insert selected" button then I did get an error message

Below is an output screenshot after clicking on the "insert selected" button.

So, it's working great.

Also, I have checked the patch for Drupal coding standard. There was an issue in one file. I have solved that and uploaded the patch.

Thanks,
Ren

dorficus’s picture

Just a tiny nitpick here, but should the language of the warning/error message be clarified a bit? When in the media library modal the text There are currently @total items selected, but the maximum number of remaining items for the field is @max. Please remove @count item from the selection.', 'There are currently @total items selected. The maximum number of remaining items for the field is @max. Please remove @count items from the selection.<code> is a bit strange.

Maybe remove the word "remaining" so it reads more like There are currently @total items selected, but the maximum number of items for the field is @max. Please remove @count item from the selection.', 'There are currently @total items selected. The maximum number of items for the field is @max. Please remove @count items from the selection.

phenaproxima’s picture

+1, I think that is clearer.

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.

bkosborne’s picture

One of my users reported this bug. I tested the patch above and it works to resolve the issue for us. I didn't review the code so leaving at needs review.

phenaproxima’s picture

phenaproxima’s picture

janmejaig’s picture

After applying this patch #80 this is working fine for drupal 9.1.x , this is good to go for RTBC .

tanubansal’s picture

Patch #80 works fine for me as well on 9.1
This can be moved to RTBC

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.

tyler-paavola’s picture

I've applied Patch #80 after upgrading from 8.7.x to 8.9.x to resolve the overflow select. Thank you!

I came across an edge case which I wanted to share that allows selecting multiple images, aside from the "select image + upload image" scenario discussed in the thread.

After selecting 1 image by its checkbox, remaining images (and their checkbox) are greyed out as expected. However, I am able to select additional images by clicking their teaser image (rather than the checkbox). After selecting an image by its teaser image, all other images (and their checkbox) are no longer greyed out. Regardless, the error message from the patch is still displayed, but the different user experience with the teaser image vs checkbox is a unorthodox.

chr.fritsch’s picture

This looks pretty good to me and we already got a bunch of +1's. I tried to reproduce #95, but I wan't able to get that error.

I updated the patch by only fixing #81.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 96: 3059955-96.patch, failed testing. View results

paulocs’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.45 KB

It looks random fail.

Locally tests are working.
Set back to RTBC.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Hmm...I'm not so sure that's a random failure. It's specifically failing the test which tests this patch.

Upon occasion, a test will pass locally but fail on Drupal CI. That may be the case here. I'll try re-running the tests on Drupal CI, and if it is a random failure, we will need to trace and fix it before we can commit this. Random failures are not our friend, and we need to avoid introducing any new ones in core. :(

Status: Needs review » Needs work

The last submitted patch, 96: 3059955-96.patch, failed testing. View results

raman.b’s picture

Assigned: oknate » Unassigned
Status: Needs work » Needs review
FileSize
16.95 KB
550 bytes

Addressing the remaining deprecation notice

Remaining self deprecation notices (6)

  6x: Declaring ::setUp without a void return typehint in Drupal\Tests\media_library\FunctionalJavascript\WidgetOverflowTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    6x in DrupalListener::startTest from Drupal\Tests\Listeners
chr.fritsch’s picture

Status: Needs review » Reviewed & tested by the community

Nice, so it was because of the deprecation warnings. We didn't had the problem with #80 back in last December, because it was only running against D8.9.x

paulocs’s picture

I didn't know deprecation notices make the tests fail in the Drupal CI.
Good to know!

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -699,11 +700,29 @@ public function updateLibrary(array &$form, FormStateInterface $form_state) {
    +    if ($current_selection = $form_state->getValue('current_selection')) {
    +      $current_selection = $current_selection . ',' . implode(',', $media_ids);
    +    }
    +    else {
    +      $current_selection = implode(',', $media_ids);
    +    }
    ...
    +    $selected_count = count(explode(',', $current_selection));
    
    @@ -749,14 +768,29 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
    +    if ($current_selection = $form_state->getValue('current_selection')) {
    +      $current_selection = $current_selection . ',' . implode(',', $media_ids);
    +    }
    +    else {
    +      $current_selection = implode(',', $media_ids);
    +    }
    ...
    +    $available_slots = $this->getMediaLibraryState($form_state)->getAvailableSlots();
    +    $selected_count = count(explode(',', $current_selection));
    

    All of this exploding and imploding looks funny. Can this not be:

    $selected_count = count($media_ids);
    if ($current_selection = $form_state->getValue('current_selection')) {
      $selected_count += count(explode(',', $current_selection));
    }
    

    Less string manipulation and less work.

  2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -153,8 +153,10 @@ protected function buildInputElement(array $form, FormStateInterface $form_state
    +      '#cardinality' => -1,
    

    Use \Drupal\Core\Field\FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED

paulocs’s picture

Assigned: Unassigned » paulocs

I'll work on it.

paulocs’s picture

Assigned: paulocs » Unassigned
Status: Needs work » Needs review
FileSize
2.85 KB
16.79 KB

I did the changes suggested on comment #105.

saurabh.tripathi.cs’s picture

There is one issue found with error message.
The error message is appearing saying, no item selected.
This only happens, when we let validation error appear, and then insert the image, and try opening the media window again.

Steps to reproduce:

Create a node of article type.
In WYSIWYG editor>Click on Media icon.
Select one file image type and then document type.
Click on Insert Selected button>Error message will come.
Unselect other files and select one>Click on the Insert Selected button.
Click on Media icon again
You will still see an error message, saying no item selected.

Possible root cause: Since we are showing that error message, and page is not refreshed, the error message box retains on media popup.

saurabh.tripathi.cs’s picture

FileSize
175.27 KB
nightlife2008’s picture

Just found another buggy situation:

- Create a content type with a single value media reference field
- Create a node
- Click "Add" in the media library reference field
- In the modal, first select any existing item
- Now select a file in your local drive for upload
=> This will trigger an autosubmit of the upload field
=> "Save & Insert" will now append _both_ items to the field
=> The UI Is broken, and the user can't remove the second item anymore
=> When submitting the node-form, the user gets an error about the number of references in the field, but they can't alter anything anymore

=> Final result: you should delete the node, and start over.

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.

nikunj.shah’s picture

I have created Reroll of #107 for 9.3.x-dev. Please review.

dmezquia’s picture

This works for me the Reroll #112 !

kim.pepper’s picture

Issue tags: +#pnx-sprint

phenaproxima’s picture

Status: Needs review » Needs work

Gave this a shallow review. There is almost certainly going to be more feedback, but for the moment, I think this is enough to start with.

phenaproxima’s picture

phjou’s picture

Just tried out the merge request, it fixes the issue for me :)

paulocs’s picture

I'm on it...

paulocs’s picture

Status: Needs work » Needs review
manojithape’s picture

Assigned: Unassigned » manojithape
manojithape’s picture

Assigned: manojithape » Unassigned
FileSize
258.21 KB
50.99 KB

Verified and tested patch "MR !760 mergeable" (i.e. https://git.drupalcode.org/project/drupal/-/merge_requests/760.patch) on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.

Testing Steps:

  1. Install drupal 9.3.x-dev version.
  2. Login as administrator
  3. Create a content type with a single value media reference field
  4. Create a node
  5. Click "Add" in the media library reference field
  6. In the modal, first, select an existing item
  7. Now select a file in your local drive for upload
  8. "Save & Insert" will now append _both_ items to the field
  9. And observe the user is able to upload both files and user not receive any error message.
  10. Now click on the "Save" node option and observe when submitting the node-form, the user gets an error about the number of references in the field, but they can't alter anything by clicking on the media "remove" option.
  11. Now apply the patch and clear the cache.
  12. Again verify user should not able to select/upload more than the allowed number of file and if the user tries to do this verify user get error message.

Expected Result:

The user should not able to select/upload more than the allowed number of files and if the user tries to do this user should get an error message.

Actual Result:

After applying the patch, the user is not able to select/upload more than the allowed number of files, and if the user tries to do this user gets an error message.

After applying patch functionality working as expected and also verified comment#110 that issue also resolved.

We can move this ticket to RTBC.

paulocs’s picture

Hi @manojithape!
Did you test if #108 and #110 were addressed?

chr.fritsch’s picture

Status: Needs review » Needs work

I can confirm that #108 is still an issue.

I also tried to reproduce #110, but never came to the point that I was unable to unselect media items. But yes, if you select 1 item -> upload a second image, you have 2 items selected and you are still able to select more items. If you reduce your selected items to one, from there all the other items are disabled for selection.

Rajab Natshah’s picture

Thank you, Adam for the patch fix

Having the same issue in Drupal ~9.0 too. Which was reported by the QA team with a number of scenarios.

  • [QA] [Media library] The user can add more than one image if he swipe to add them from the next page from the media library window
  • [QA] Models - Media library model: images number exceed the limitation ( with Layout Builder)

Important to Freeze AJAX actions when the limit of media items is out

  • Search ( Auto submit )
  • Filters and Reset filters
  • Upload
  • Switch to another media type tab
  • Switch page when we have a pager

In case of using Media Library in a custom Block Type. Which used by the Layout Builder

For sure it should be fixed in Drupal core for normal edit/add forms and the 1st, 2nd, and 3rd AJAX level modal in back-end and front-end.

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

Rajab Natshah’s picture

Issue summary: View changes
FileSize
303.36 KB

Tested the #107 patch with Drupal 9.2.x
Working very well. Very much better than before.
Thanks, Paulo for patching this issue for Drupal 9.2.x

Having a message and a blocker of insert action is good
Media Library after the fix

Hoping to have a quick release for this issue in Drupal 9.3.x and 9.2.x or it will be only committed to 9.3.x

yogeshmpawar’s picture

Status: Needs work » Needs review
NikolaAt’s picture

Hello, recently upgrade to 9.2.6 and find out that changes committed to the core are not full. There is one more existing case from the first variations of the patch, where it handles if you switch pages or apply some filters for media library.

Rajab Natshah’s picture

Switched to test with the 760.patch to fix drupal-9.2.x
After

It's working well
After the fix:

Warning message
There are currently 2 items selected. The maximum number of items for the field is 1. Please remove 1 item from the selection.

1 was selected then on the upload of new image media file
1 was selected then on the upload of new image media file

Very good as not able to insert selected if they are out of the limit
After unchanging the extra old selected media item
Then we are able to insert

After unchanging the extra old selected media item

NikolaAt’s picture

Ignore #129 -> Proper patch is this.

This is fixing my issue with switching the pages in media browser.

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.

scotwith1t’s picture

#131 did not work for us. Using the pager to go to other pages of results, we can then add more than 1 item to the field which only allows a single image on D9.2.6.

danflanagan8’s picture

Status: Needs review » Needs work

Setting back to NW because there's a cspell error in #131 and the latest comment suggests that patch does not quite solve the bug.

mErilainen’s picture

The same issue as with the pager applies when filtering with a keyword. "2 of 1 item selected".

NievesWunder’s picture

I fixed the issue of the images being still enabled even though the limit was achieved when filtering and using the pager

dariemlazaro’s picture

I need help. The patch #136 not work in Drupal 9.3.7.

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.

webcultist’s picture

Tried 136. I could check just one picture, but when I already checked one image and filled another into the upload field above and clicked embed, both images get posted and you can't delete one in interface. Nothing happens when I clicked the "X".

bbu23’s picture

Tried patch #136 against Drupal core 9.3.13, but unfortunately this doesn't work for my case.

This is reproducible by adding medias with pagination. For example:
1. Add media (let's assume image, but it doesn't matter)
2. From the media library listing, select one item from the first page. Now all the other entities should be disabled for selection (for limit 1)
3. Navigate to the next page and see that the items are available for selection
4. Select another image from the second page and insert them
5. Now you have 2 images, with "-1 media items remaining."

Chris Matthews’s picture

Since 10.0.0-beta1 and 9.5.0-beta1 are planned to be released the week of September 12, should this issue now be targeted against 10.1.x-dev?

phenaproxima’s picture

Version: 9.5.x-dev » 10.1.x-dev
ravi.shankar’s picture

Added reroll of patch #136 on Drupal 10.1.x.

aludescher’s picture

Re-rolled patch #143 for 9.4.x.

bnjmnm’s picture

#129 / #131 are of an approach that was proposed very early in the process of this issue and the decision was to go in another directions. Now it appears like the comments since are reviews and rerolls of an outdated approach. Some really good changes happened since then and we should be working with those!

The work had moved to an MR. I'm providing a 10.1.x version of the work in that MR because

  • Moving an MR to a new major version isn't easy - if a new MR is desired it can be created and this patch applied to it
  • Since there's activity on this issue looking for patches instead of MR, i figured it couldn't hurt for the most recent MR to reflect the most recent code (which has test coverage) so we're not getting code from ~2020 reviewed.

There's one thread in the MR that wasn't addressed
core/modules/media_library/src/Form/FileUploadForm.php line 157

      '#multiple' => TRUE,
      // Do not limit the number uploaded.  There is validation based on the
      // number selected in the media library that prevents overages.

@phenaproxima requested "Let's add a @see comment referencing the method where the validation is taking place."

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Thank you @bnjmnm for the correct patch and catching that!

Seems the thread comment in #145 has been addressed.

Without the patch verified that the issue is present in 10.1.x following the testing steps in the issue summary.

After applying the patch.
I do get the message "There are currently 3 items selected. The maximum number of items for the field is 2. Please remove 1 item from the selection." as a warning

If I click save the warning becomes an error.

Tested uploading a 4th item also and the message correctly reflects that too.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this! I haven't reviewed it completely yet, but I found several things that need to be addressed:

  1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -750,17 +762,44 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
    +   * Get the number of selected medias.
    ...
    +  private function getNumberOfMediasSelected(array $media_ids, FormStateInterface $form_state) {
    

    I'm surprised "medias" didn't fail cspell. It's not a word, because "media" is already the plural of "medium".

    I'd suggest the method name should instead be:
    getSelectedMediaItemCount().

    Also, does it really make sense for this method to be private (vs. protected)?

    Finally, it should have a return typehint.

  2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -750,17 +762,44 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
    +   *   Array with the media ids.
    

    Nit: "IDs" should be capitalized. An id is something else.

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTestBase.php
    @@ -400,6 +404,18 @@ protected function selectMediaItem($index, $expected_selected_count = NULL) {
    +   * Unselects an item in the media library modal.
    

    This should begin with "De-selects", not "Unselects". "Media Library" should probably also be capitalized.

  4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetOverflowTest.php
    @@ -0,0 +1,183 @@
    + * Tests that uploads in the Media library's widget works as expected.
    

    Nit: "Media Library" should be title-case, since it's a module name. (Here and elsewhere.)

  5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetOverflowTest.php
    @@ -0,0 +1,183 @@
    +   * Tests that the media library constrains selection size.
    

    Here and elsewhere, it should say "constrains the number of selected items". And "Media Library".

  6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetOverflowTest.php
    @@ -0,0 +1,183 @@
    +   * @param string|null $save_and
    

    This is a really strange parameter name. Can we call it $selected_operation or something?

  7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetOverflowTest.php
    @@ -0,0 +1,183 @@
    +   * Tests that fields of unlimited cardinality don't constrain selection size.
    

    Maybe:

    Tests that unlimited fields' selection count is not constrained.

xjm credited codersukanta.

xjm credited pankaj.singh.

xjm credited tripurari.

xjm’s picture

It is difficult to assign credit on this issue because:

  1. There were two duplicates of it
  2. There was a whole lot of unnecessary repeat manual testing
  3. There was a whole series of patches being rolled against end-of-life branches from an old version of the patch (ignoring the MR and the work on it)

Due to these problems, I unchecked everyone and started over.

The following people have been credited for substantive helpful code review and work on the code changes:

  • SeanB
  • oknate
  • bnjmnm
  • Wim Leers
  • Lauri
  • effulgentsia
  • alexpott

For helpful usability and accessibility feedback:

  • Aaron McHale
  • andrewmacpherson
  • dorficus

For the listed contributions

  • AndySipple, for a useful first attempt at a patch
  • raman.b for fixing deprecations
  • Yogesh Pawar for resolving a coding standards issue
  • saurabh.tripathi.cs for useful manual testing uncovering a bug
  • nightlife2008 for useful manual testing and uncovering a bug
  • chr.fritsch for necessary improvements to the patch, as well as review identifying that some issues still needed to be addressed
  • nikunj.shah, for a necessary reroll
  • smustgrave, for manual testing after the MR against a 9.x branch was converted to a 10.1.x patch (verifying it hadn't regressed)

For work on #3092536: Possible to select multiple images in a media field where only 1 value is accepted

  • Hardik_Patel_12, for work on original patch, but not for simply converting this issue's patch into an MR without additional changes
  • himanshu_sindhwani, for work on the original patch
  • pankaj.singh, for original manual testing
  • snehalgaikwad, for a necessary reroll
  • priyanka.sahni, for manual testing after it had already been done, but with clearer illustrations (usually we do not credit duplicate manual testing)

For work on #3160240: User can select multiple media entities but only one is allowed

  • codersukanta for a useful original patch
  • tripurari for helpful review and manual testing

Users that received credit for some contributions, but who should take note of best practices regarding others in the future

  • paulocs for helpful code review and fixes to the MR, but not for attaching a screenshot of a local test run
  • NikolaAt, for helpful original issue summary, but not for #129 and #131, which went back to a previous version of the patch without taking into account the current status of the issue. Please be sure to take into account all recent comments and the current status of the issue before posting patches or reviews.
  • rensingh99, for your manual testing with screenshots in #62 and #80, but not for the screenshots of the patch not applying in #58. The automated testing infrastructure tells us when patches don't apply. Please don't upload screenshots of your CLI related to applying or not applying the patch.

This contributions might receive credit but I need a second opinion from another maintainer

  • scotwith1t in #133 and mErilainen in #135 (Manual testing on an old branch -- does this bug exist in newer versions of the patch?)
  • ravi.shankar (rerolling an outdated version of the patch instead of using the MR, but at least they were trying to make it 10.1.x-compatible, and maybe by that point it was impossible to tell that it was all rerolls of an old patch)
  • manojithape: Manual testing after converting to an MR is helpful to make sure nothing was lost; however, it was alsomanual testing without taking into account the current status of the issue.

Contributions which will not receive credit

For folks on this list, in the future, please be more careful to read and understand the current state of an issue before working on it, take note of when a merge request has been opened, and don't resurrect patches for old versions.

  • fkelly (reporting unrelated bug)
  • andreasderijcke (comment was just confirming the issue, which does not receive credit)
  • Rajab Natshah (just re-reporting the issue, manually testing an old patch against an end-of-life branch, etc. without taking into account the current status of the issue)
  • NievesWunder (rerolling a patch against an end-of-life branch, without taking into account the current status of the issue)
  • aludescher (rerolling a patch agains an incorrect branch, without taking into account the current status of the issue)
  • Various others for reporting patches did not work against end-of-life branches of Drupal, "Works for me" comments, "I have this bug too" comments, small issue metadata changes, etc.

If you have questions or concerns with these credit assignments, please carefully review the core issue credit guidelines.

Thanks everyone for your efforts!

harshitthakore’s picture

FileSize
11.58 KB

Updated patch as suggested by @xjm in comment #147.

xjm’s picture

Thanks @harshitthakore.

Please provide interdiffs when updating patches using the patch workflow.

Your reroll is also missing the entire test file. When you apply patches locally, use git apply --index to stage files, and also pay close attention to making sure your patch includes all the changes from the original. A good way to check this is to consider the diffstat and patch size. @bnjmnm's patch in #145 is 17.94 K, and has 6 files changes, 277 insertions, and 9 deletions. Yours is 11.58 K with 5 files changed, 94 insertions, and 9 deletions. That is missing a lot of the added code.

I will leave credit for your contribution because you did successfully update the method name, so there is still less work for subsequent reviewers, but please be more careful in the future to continue receiving credit for your contributions.

It looks like our instructions for applying a patch to a feature branch and creating a patch for an issue did not correctly include use of the --index flag, so I've updated them for future reference.

If you need assistance with the patch creation workflow, you can also discuss patch creation with other contributors in Drupal Slack.

xjm’s picture

I've attached #154 with the test from #145. Since it's the same version of that test from the earlier patch, points 4-7 in my review will still need to be addressed. Edit: Point 1 is also not fully addressed; the method has been renamed but there's no change to its typehint or visibility.

xjm’s picture

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -750,17 +762,44 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
+   * @return int
+   *   The number of medias currently selected.

This also still says "medias".

xjm’s picture

Fixing attribution.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
18.64 KB

Finished addressing the points in #147 + #157

mgifford’s picture

Issue tags: +wcag331

WCAG tagging.

joaopauloscho’s picture

The patch #159 works for us on 9.5.2 - thanks!

sonam.chaturvedi’s picture

Patch #159 works fine on 10.1.x-dev only when one Media type is enabled.

After applying patch:
1. When only "Image" media type is enabled in the Media Library then warning is displayed on exceeding allowed limit and error is displayed on clicking "Insert Selected".

after patch 159

2. When multiple media type is enabled say "Image" , "Document" then no warning is displayed on exceeding media selection. However, error is displayed on clicking "Insert Selected".
Expected Result: Warning should be displayed when combination of media type is selected and count exceeds allowed limit.

after patch 159 multi media

sonam.chaturvedi’s picture

Status: Needs review » Needs work
smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
20.45 KB

Wonder if this is any better?

sonam.chaturvedi’s picture

FileSize
3 MB

Patch #164 works fine on 10.1.x-dev and issue is resolved.

Test Result:
When multiple media type is enabled say "Image" , "Document" then user is not allowed to do multiple media selection. As user is able to select only one media at a time, there is no warning displayed. IMO this should be fine.

Please refer attached after patch video.

mei2020’s picture

core/modules/media_library/js/media_library.ui.js on 9.5.x is different from 10.1-x-dev, rerolled this patch for 9.5.x, hope it helps.

_utsavsharma’s picture

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
86 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

ameymudras’s picture

Status: Needs work » Needs review
FileSize
20.41 KB

Re rolled the patch #167, the patch doesn't apply anymore

smustgrave’s picture

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.

mgifford’s picture

@smustgrave in #166 it was reported that core/modules/media_library/js/media_library.ui.js on 9.5.x is different from 10.1-x-dev

I don't know, but just not sure if you're suggesting that #164 is the patch we should be using or what...

smustgrave’s picture

#164 should be used for testing on 11.x or 10.x

Can determine if we will backport to D9 after I suppose.

irene_dobbs’s picture

Issue tags: +Pittsburgh2023

Tested patch 3059955-164.patch on DrupalPod, I used Drupal 10.1.x

This patch is working as proposed solution suggests, if selected number of items exceeds maximum number of allowed items, a warning message is displayed. If user tries to click 'Insert Selected' an error message is displayed and the user is not allowed to insert any items.

irene_dobbs’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Ran yarn prettier to try to make the custom commands pass.

  • lauriii committed ae4b7247 on 11.x
    Issue #3059955 by oknate, Hardik_Patel_12, paulocs, phenaproxima, seanB...
lauriii’s picture

Version: 11.x-dev » 10.1.x-dev
FileSize
1.86 KB

Made a minor fix on commit, attached interdiff.

Committed ae4b724 and pushed to 11.x. Thanks!

Leaving open for potential backport to 10.1.x.

  • lauriii committed a0acceaa on 10.1.x
    Issue #3059955 by oknate, Hardik_Patel_12, paulocs, phenaproxima, seanB...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Got a +1 from @catch for the backport to 10.1.x during RC.

seanB’s picture

Wow, thank you all for getting this done! ❤️👏

Swayanka’s picture

Hi , I am adding a patch for the issue where i was not able to add any documents from "Add Media" .This has worked for me.

lauriii’s picture

@Swayanka I think we should file a new issue for the bug you are reporting 😊

Status: Fixed » Closed (fixed)

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