Creating media with the media library upload is broken for unlimited cardinality

I'll take a look

This was an easy fix, but need to add a test. I'm taking a look at writing the test as well.

+++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
@@ -358,7 +359,7 @@ public function validateUploadElement(array $element, FormStateInterface $form_s
+    if (count($values['fids']) > $element['#cardinality'] && $element['#cardinality'] !== FieldStorageConfig::CARDINALITY_UNLIMITED) {

We should do this check on FieldStorageDefinitionInterface, instead of its config implementation :)

Thanks, @bkosborne! Running testbot anyway, and tagging for test coverage.

It's worth mentioning that the Mink driver used by the tests doesn't support multiple file uploads (at all), so we may need to manually test this. If it can be reproduced by uploading a single file into an unlimited-cardinality field, though, let's do that.

Yes it can be reproduced by uploading a single file. Test getting there.

Didn't run this test locally yet, but maybe I'll get lucky. Also addressed #4

Tests are written; removing tag.

Very frustrating trying to get local testing environment setup =/. I have it going with PHPUnit. The error I'm currently getting is tough to figure out:

vendor/bin/phpunit --stop-on-failure -c core core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
E

Time: 1.78 minutes, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testAdministrationPage
PHPUnit_Framework_Exception: Fatal error: Call to a member function execute() on null in /Users/bosborne/Projects/drupal8-core-dev/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php on line 969

Call Stack:
    0.0006     139988   1. {main}() -:0
    1.2012    1598548   2. __phpunit_run_isolated_test() -:100
    1.2284    3351492   3. PHPUnit_Framework_TestCase->run() -:52
    1.2309    3477356   4. PHPUnit_Framework_TestResult->run() /Users/bosborne/Projects/drupal8-core-dev/vendor/phpunit/phpunit/src/Framework/TestCase.php:724
    1.2324    3530076   5. PHPUnit_Framework_TestCase->runBare() /Users/bosborne/Projects/drupal8-core-dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:612
  104.9655  132532788   6. Drupal\FunctionalJavascriptTests\WebDriverTestBase->tearDown() /Users/bosborne/Projects/drupal8-core-dev/vendor/phpunit/phpunit/src/Framework/TestCase.php:805
  104.9655  132532968   7. Behat\Mink\Session->wait() /Users/bosborne/Projects/drupal8-core-dev/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:69
  104.9655  132533012   8. Behat\Mink\Driver\Selenium2Driver->wait() /Users/bosborne/Projects/drupal8-core-dev/vendor/behat/mink/src/Session.php:354

I think I followed everything in the guide but this is blocking me.

Sorry all, if anyone can maybe assist with local setup, ping me in #media, but I have to move on to some other work for the moment.

Working on this.

Wow, this bug was hard!

First, the "Unlimited media" field in the test module does not allow you to select any media types that use the file/image source. As a result, I would expect that without config changes the "Add media" button shouldn't even be present. Unfortunately, because of #2956747: Pass query parameters to the route access check query parameters used to build a Drupal\Core\Url object are not passed when determining access, so the "Add media" button showed up on every instance of the Media library widget _if_ the user had access to create _any_ media. This only affected the "Add media" button in the field widget, which is why I didn't catch this before.

I resolved this problem by calling the form's access method directly in the field widget, which we can remove when #2956747: Pass query parameters to the route access check is closed. Alternatively, the work done in #2987924: Define an API for finding media types based on an arbitrary value could also be used by the field widget to determine if the current user has access to create any media allowed by the field that uses a file source. So we have at least two paths forward for this temporary fix.

I also added test coverage for the above.

Once that resolved, I could actually fix the relevant test failure here, which I did by letting the "Unlimited media" reference a media type that used the file source.

😅All done, phew!

What a tricky bug! Thanks for chasing that down. I guess we'll have to try and accelerate #2987924: Define an API for finding media types based on an arbitrary value, since I imagine it's a lot easier to fix things in Media than in the routing system.

Fixed config bug in test.

@samuel.mortenson 💪!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -56,23 +64,30 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, EntityTypeManagerInterface $entity_type_manager, $add_access) {
   ...
   +    $this->addAccess = $add_access;
   ...
   +      MediaLibraryUploadForm::create($container)->access($target_bundles)->isAllowed()
   

   I am wondering if the "Add access" should also respect the "Create referenced entities if they don't already exist" setting from the field definition.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -348,10 +351,26 @@ public function testWidgetUpload() {
   +    // Also make sure that we can upload to the unlimited cardinality field.
   +    $unlimited_button = $assert_session->elementExists('css', '.media-library-add-button[href*="field_unlimited_media"]');
        $unlimited_button->click();
        $assert_session->assertWaitOnAjaxRequest();
   +
   +    $page->attachFileToField('Upload', $this->container->get('file_system')->realpath($png_image->uri));
   +    $assert_session->assertWaitOnAjaxRequest();
   

   Could we test the upload of two or three images here?

@chrfritsch The handler settings are sadly tied to the Entity Reference Autocomplete widget, and don't make sense for other widgets. "Create referenced entities if they don't already exist" for instance also reveals the "Store new items in" option, which determines what bundle auto-created entities are in. Since this widget allows you to create Media for any of the target bundles that use a file source, this setting wouldn't apply.

"View used to select the entities" similarly seems like a good setting for the library, but doesn't make sense here either - it just executes the view and returns all the entity labels to the entity reference widget.

I almost think that what we need is a custom handler with settings specific to the Media Library, or a message that informs users that handler settings don't really apply here. We should open a new issue for this I think.

We can't test multiple uploads. :-( It's a limitation of the driver as far as I know, which is a shame for this use case.

I've read the patch a few times now, and the problem makes sense (as does the fix). It's too bad we have to place these kinda icky temporary hacks in the widget to deal with this, but there's no other option in the short term. Good thing the media library is experimental!

RTBC once green on all backends.

Re-titling for clarity.

Retitling to sound less like an access bypass. :)

Not sure if it's exactly the same but in https://www.drupal.org/project/drupal/issues/2894193 there's a test (credits to vaplas) to upload multiple files. Not sure if this could be used here.

I've confirmed the added tests fail as expected by the fix is not included:

Testing Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest
...E                                                                4 / 4 (100%)

Time: 1.29 minutes, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload
Behat\Mink\Exception\ExpectationException: An element matching css ".media-library-add-button[href*="field_noadd_media"]" appears on this page, but it should not.

/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:770
/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/behat/mink/src/WebAssert.php:445
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php:321

I've also confirmed by watching the tests run in chromedriver that the fixed behaviour works as expected.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -56,23 +64,30 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
+    $target_bundles = isset($settings['target_bundles']) ? $settings['target_bundles'] : FALSE;

Fortunately target_bundles is always set because if not this would fail bad - see https://3v4l.org/Cpicf

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -78,7 +78,7 @@ public function __construct($plugin_id, $plugin_definition, FieldDefinitionInter
+    $target_bundles = isset($settings['target_bundles']) ? $settings['target_bundles'] : NULL;

I think it's worth noting that the target_bundles setting of the ER field has three states:

- NULL: all bundles are referenceable
- []: no bundle is referenceable
- ['some_bundle']: only some_bundle is referenceable

This is documented in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::defaultConfiguration, and maybe some logic of this patch needs to be updated based on this information.

Thanks for the review @amateescu. I think that the current patch doesn't make anything worse. It just might not be a complete fix for all possible entity reference field settings. Therefore I think we should file a follow-up to add tests to ensure the three possible states of target_bundles work as expected.

Committed and pushed 057b54e03f to 8.7.x and 1dbb94162f to 8.6.x. Thanks!

Thanks @amateescu - I opened #2989503: Add tests to prove that the media library widget works when target_bundles is NULL to address this. What's funny about that docblock is that in practice target_bundles can never be NULL or an empty array - it's a required field! If you got to configure a field you won't be able to not select a bundle. We should still handle this "un-configured" case.

We missed something here - the input element itself also needs to allow for multiple uploads. This patch needs a quick review and commit, if anyone has time.

Marking for manual testing since Mink doesn't support multiple file uploads.

Gave this a quick manual test. It worked exactly as advertised -- the file input accepts multiple files at once, if more can be accepted by the field. Let's get 'er in.

Could an automated test be added that just asserts that the input element has the multiple attribute? And perhaps then an @todo for actually testing the multiple upload once https://github.com/minkphp/Mink/issues/358 is fixed?

Good call @effulgentsia, let's do that.

Yup, good call. Everything green, let's do it.

Pushed to 8.7.x and 8.6.x.

Acquia lighting (8.x-4.000) and Drupal 7.1, issue not fixed with the above mentioned patches. Thanks.