Allow media to be uploaded with the Media Library field widget

Oh, let the trumpets ring out a major +1 for this! What brilliant work. Thank you, @samuel.mortenson!

Adds a new action to /admin/content/media and a new user-facing form.

In my experience, this kind of thing tends to cause a lot of weird political bikeshedding. I suggest we remove the local action from the patch for now, then re-add it in a follow-up.

Other than that...

This feature will form the basis for integration between media entities and content which references media entities, as outlined in #2831940-151: Create file field widget on top of media entity. So this bulk upload form partially supersedes #2831940: Create file field widget on top of media entity.

Let's also open a follow-up to integrate this with a field widget (i.e., the other half of the code in #2831940-144: Create file field widget on top of media entity.)

I'd also like to add this issue (plus the aforementioned follow-up) to #2825215: Media initiative: Roadmap as official blockers before Media can be included in Standard.

Add a new Form Display Mode, "minimal", that only shows required fields by default (but is user configurable).

To keep this patch simple and easier to review, that might also be worth moving to a follow-up.

Also, for easier review, let's add some screenshots to the IS.

Added #2938123: [PP-1] Streamline media creation form presented during bulk upload to address the creation of a "minimal" form mode for media items. So that task is now out of this patch's scope.

Also added #2938121: Expose a local action to bulk upload media items to add the local action for bulk upload. So this patch will no longer introduce any UI changes (although it does introduce a new, isolated UI; not sure if that counts).

Gave the patch a read-through. Overall this code is clean, well-organized, and exceptionally clear when you consider what it's accomplishing.
Bravo!

1. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +/**
   + * An AJAX multi-step form to bulk create media entities.
   + */
   

   The doc comment should probably mention that the media entities can only be created from uploaded files.

2. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +  use MediaUploadTrait;
   

   Nice idea!

3. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +    // Store media types the current user can create as early as possible.
   +    $media_type_storage = $this->entityTypeManager->getStorage('media_type');
   +    $types = $media_type_storage->loadMultiple();
   +    $types = $this->filterTypesWithFileSource($types);
   +    $types = $this->filterTypesWithCreateAccess($types);
   +    $this->types = $types;
   

   This seems a bit heavy for a constructor. Let's move this to a helper method so that $this->types is populated on-demand.

4. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +      case NULL:
   +        $form = $this->buildUploadForm($form, $form_state);
   +        break;
   

   'case NULL' seems a bit odd. Maybe this should simply be the default case instead?

5. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +      case 'select_media_type':
   +        $form = $this->buildMediaTypeSelectionForm($form, $form_state);
   +        break;
   +
   +      case 'show_media_form':
   +        $form = $this->buildMediaForm($form, $form_state);
   +        break;
   +
   +      case 'finished':
   +        $form = $this->buildFinishedForm($form, $form_state);
   +        break;
   

   Nit: We can remove all these break statements in favor of directly returning the result of the helper method.

6. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +   * Returns a render array representing the form process.
   

   s/process/progress. And maybe this should be rephrased to something like "...the current progress of the bulk upload."

7. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +    if ($this->originalFileCount > 1 && $this->step !== 'finished') {
   +      $remaining = ($this->originalFileCount - count($this->files));
   

   This logic is a bit opaque, can there be a comment explaining it a little?

8. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +    $element_info = $this->elementInfo->getInfo('managed_file');
   

   Nit: You can use $this->elementInfo->getInfoProperty('managed_file', '#process'), rather than retrieve the whole element info array. :)

9. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,537 @@
   +    // Hide the Managed File element's table display - we don't use it.
   +    $element['remove_button']['#access'] = FALSE;
   

   The remove button has no point in this form and never should, so I suggest we simply unset it.

10. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +      if (isset($element['file_' . $fid])) {
    +        $element['file_' . $fid]['#access'] = FALSE;
    +      }
    

    I think we can unset all of these as well, rather than disable them. I see no reason to keep them around.

11. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +      '#markup' => t('<p>Select media type to create with <strong>@file</strong></p>', [
    +        '@file' => $this->currentFile->label(),
    

    Can we say "Select which media type to create..." (just add the word 'which')?

12. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +      $form[] = [
    +        '#type' => 'submit',
    +        '#name' => $type->id(),
    +        '#value' => $type->label(),
    +        '#submit' => ['::mediaTypeSelectSubmit'],
    +        '#ajax' => [
    +          'callback' => '::updateFormCallback',
    +          'wrapper' => 'media-build-upload-form-wrapper',
    +        ],
    +      ];
    

    Clever repurposing of the #name property!

13. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +    // Replace relative (::*) callbacks in the media form.
    +    foreach (['#process', '#entity_builders'] as $key) {
    +      foreach ($form['subform'][$key] as &$callback) {
    +        if (is_string($callback) && substr($callback, 0, 2) == '::') {
    +          $callback = [$this->mediaForm, substr($callback, 2)];
    +        }
    +      }
    +    }
    

    Should we handle things like #after_build callbacks as well? Also, let's expand the comment to explain more fully what's going on (and more importantly, why).

14. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +   * @return \Drupal\Core\Entity\EntityFormInterface|false
    +   *   A media form object, or FALSE if there was an error.
    

    It doesn't look like the method ever actually returns FALSE...?

15. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +  protected function prepareForNextFile(FormStateInterface $form_state) {
    +    $form_state->setRebuild();
    

    I've noticed that callers of prepareForNextFile() call $form_state->setRebuild() anyway. We can probably remove those calls.

16. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +  protected function clearFormState(FormStateInterface $form_state) {
    +    $form_state->setStorage([]);
    +    $input = $form_state->getUserInput();
    +    $input = array_intersect_key($input, array_flip([
    +      'form_build_id',
    +      'form_token',
    +      'form_id',
    +    ]));
    +    $form_state->setUserInput($input);
    

    Is it possible that this could break media forms which have widgets that depend on storing stuff in $form_state storage? I'm not sure (and therefore a little worried) about any possible side effects this could have.

17. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function submitForm(array &$form, FormStateInterface $form_state) {}
    

    !!!

18. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
    @@ -0,0 +1,537 @@
    +  /**
    +   * Access callback to check that the user can create file based media.
    +   */
    +  public function access() {
    +    return AccessResultAllowed::allowedIf(count($this->types))->mergeCacheMaxAge(0);
    +  }
    

    We'll need a test of this for authenticated users as well. We should probably test a few different cases.

19. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +      if (is_a($source, 'Drupal\media\Plugin\media\Source\File')) {
    +        $valid_types[] = $type;
    +      }
    

    I think instanceof is preferred to is_a(), but I could be wrong about that. Also, is there any reason for this method not to use array_filter()?

20. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +      $validators = $this->getUploadValidatorsForType($type);
    +      $max_size = max($max_size, $validators['file_validate_size'][0]);
    +      $extensions = array_unique(array_merge($extensions, explode(' ', $validators['file_validate_extensions'][0])));
    

    This should account for the possibility of file_validate_size and file_validate_extensions not being set in $validators.

21. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +    $file_item = $this->getFileItemForType($type);
    +    return $file_item->getUploadValidators();
    

    Nit: This can be a single line.

22. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +    $file_item = $this->getFileItemForType($type);
    +    return $file_item->getUploadLocation();
    

    Nit: This can also be a single line.

23. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +   * Creates a file item for a given Media Type.
    +   *
    +   * @param \Drupal\media\MediaTypeInterface $type
    +   *   A Media Type.
    

    Supernit: "Media Type" should be lowercase.

24. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +    $source_data_definition = FieldItemDataDefinition::create($source->getSourceFieldDefinition($type));
    +    return new FileItem($source_data_definition);
    

    Is there any way we can avoid hardcoding the FileItem class?

25. +++ b/core/modules/media/src/MediaUploadTrait.php
    @@ -0,0 +1,155 @@
    +    $access_handler = $this->getEntityTypeManager()->getAccessControlHandler('media');
    +    foreach ($types as $type) {
    +      if ($access_handler->createAccess($type->id())) {
    +        $valid_types[] = $type;
    +      }
    +    }
    

    This seems like a perfect candidate for array_filter().

This is really awesome. Kudos!

One thing about the UX. I think it would be helpful if the creation form provides somehow from which media type the current item will be.

Also I just found some minor issues.

1. +++ b/core/modules/media/media.routing.yml
   @@ -5,6 +5,14 @@ entity.media.multiple_delete_confirm:
   +media.bulk_upload:
   +  path: '/admin/content/media/upload'
   +  defaults:
   +    _form: '\Drupal\media\Form\MediaBulkUploadForm'
   +    _title: 'Bulk Upload Media'
   +  requirements:
   +    _custom_access: '\Drupal\media\Form\MediaBulkUploadForm::access'
   +
   

   In #2670730: Provide a delete action for each content entity type we are introducing a delete-multiple-form, maybe this could be a add-multiple-form link relation and then add the route by the DefaultHtmlRouteProvider.

2. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,544 @@
   +  public function __construct(EntityTypeManagerInterface $entityTypeManager, ElementInfoManagerInterface $element_info) {
   

   This should be $entity_type_manager

3. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,544 @@
   +    $process = $this->elementInfo->getInfoProperty('managed_file', '#process');
   +    $upload_validators = $this->mergeUploadValidators($this->getTypes());
   +    $form['upload'] = [
   +      '#type' => 'managed_file',
   +      '#process' => array_merge($process, ['::processUpload']),
   

   getInfoProperty returns a string, but array_merge expects an array

In #2670730: Provide a delete action for each content entity type we are introducing a delete-multiple-form, maybe this could be a add-multiple-form link relation and then add the route by the DefaultHtmlRouteProvider.

That's an interesting idea, but because this add-multiple stuff is a new pattern in core, I think we should discuss/implement that in a follow-up.

getInfoProperty returns a string, but array_merge expects an array

In my experience, getInfoProperty() returns mixed. Whatever type the property is, is what is returned. This may be an example of an inaccurate doc comment, in which case we should open a follow-up to change it.

In #2670730: Provide a delete action for each content entity type we are introducing a delete-multiple-form, maybe this could be a add-multiple-form link relation and then add the route by the DefaultHtmlRouteProvider.

That's an interesting idea, but because this add-multiple stuff is a new pattern in core, I think we should discuss/implement that in a follow-up.

I am fine with moving that into a follow-up. But at least we should consider giving the route a future-proof name. Something like entity.media.add_multiple_form

But at least we should consider giving the route a future-proof name. Something like entity.media.add_multiple_form

+1 for that.

This looks like a really nice patch! Thank you @samuel.mortenson for this feature!

+++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
@@ -0,0 +1,544 @@
+  protected function getFormProgress(FormStateInterface $form_state) {

Nitpick: This isn't really a getter. I like using build() when you build a render array. Thoughts?

Nitpick: This isn't really a getter. I like using build() when you build a render array. Thoughts?

Sounds reasonable, I would refactor this function, if it's consensus

@mmbk, go for it.

Applied the path and it worked fine. Have not checked the code. The functionality worked.I was able to upload multiple images together.
PFA - gif

@mmbk, go for it.

The method is called by 'buildForm', and builds the progressbar, so I'll name it 'buildProgressbar'

Starting nickpicking as well:
While examining the progressbar, I noticed, that it displays a very short bar when it got a #percentage of 0 (see screenshot)
I was a bit confused, but this seems to be the normal behavior of the progressbar.
Maybe it's less confusing when the progressbar displays 25% when entering for the first of four entites, and 100% when editing the last entity.
Saying this while knowing that progressbars normally display the percentage of finished tasks

While examining the progressbar, I noticed, that it displays a very short bar when it got a #percentage of 0 (see screenshot)
I was a bit confused, but this seems to be the normal behavior of the progressbar.
Maybe it's less confusing when the progressbar displays 25% when entering for the first of four entites, and 100% when editing the last entity.
Saying this while knowing that progressbars normally display the percentage of finished tasks

That is normal progress bar behavior, indeed. It's not great, but it's not in scope of this patch to address it. So no worries -- we can just rename the method and leave it at that. :)

Fixed failing tests.

@mmbk You'll need to change the bulk upload route name in the ::buildFinishedForm method as well, to fix tests.

Yes I know - and i fixed it already - but i wanted to test it locally before I do the upload.

Should have written a notice earlier that I'm still working on this issue. To avoid doubled work.

Applied https://www.drupal.org/files/issues/2938116-24.patch

Clear drupal cache

Bulk upload form is at

/admin/content/media/upload

Bulk upload works.

Note: there is no bulk upload button at /admin/content/media

I've tested #25 and it works for me.

Also tested use cases attempting to break it.

- Where more than 1 media type matches file extension and it presents a "select media type" dialogue.
- Tested No file extension (throws an error and rejects upload)
- Uploading a word doc renamed extension to jpg (Error is a bit cryptic, "This value should be of the correct primitive type." but that's outside the scope of this issue

Looks good to me. Great work.

Note: there is no bulk upload button at /admin/content/media

As I learned here on the sprint, this should not be part of this issue, but of new following issue.

Sorry to be "that guy", but this needs further review (not least from the usability team) before it's ready for RTBC :) However, thanks to everyone who has manually tested this and confirmed it works! That is much appreciated and will surely help "grease the wheels" to get this into core.

Added label for the creation form which shows which media type will be added.

The refactoring to the use of html_tag is great, totally agree. But you should probably rewrite this as well:
line 278, MediaBulkUploadForm.php

<strong>@file</strong>

to

%file

Because % add the tag automatically.

@samuel.mortenson #37
In my opinion this makes the translation of the string cleaner. If for some reason the tag changes from 'p' to 'span', than its not in your translatable string, so your translatable would still be valid.

Implemented the change suggested in #36.

Also noticed a bug regarding the validation of file uploads. When uploading multiple files of which one has validation errors, the correctly uploaded files were hidden but still in the field values. When uploading the valid files again, they occurred twice in the values of the field.
Discussed this issue with @seanB and @Mirnaxvb. This implementation will unset these files and remove them from the file storage. Not really happy about the current unsetting of the element values yet, so open for any suggestions.

Unfortunately #files is not yet set in that processor. The next processor adds the #files attribute, but we have to unset it before that step.

I think for our use case this is fine, but it does look funny. We could avoid this logic and all our managed file process methods if users had to confirm uploaded files before moving to the next step - so we show a completely "vanilla" managed file element and have a "Use these files" button or something. I like the way the form currently works (feels more seamless), but it's worth bringing up as an alternative.

Discussed this with @seanB. Showing a completely "vanilla" managed file elements makes this field too complex for users. The current implementation is more seamless.

Fixed tests. A text was changed in #34.

@sjerdo

Patch in #44 looks good.

Discussed this with @seanB. Showing a completely "vanilla" managed file elements makes this field too complex for users. The current implementation is more seamless.

Yeah, I think showing the uploaded files could lead to confusion. When you see an error for 1 of the files, what happens if I start uploading again? Does it overwrite the previous set, or add to it? Especially if you made a complex selection. How can a user find out which of the files were already added or not?

Comparing those lists is not very user-friendly. Erasing all files and letting the user start over is also not very user-friendly, but to me that is at least a little better.

When you see an error for 1 of the files, what happens if I start uploading again? Does it overwrite the previous set, or add to it?

My understanding is that the file which caused the error is kicked out of the list, and ignored. If you upload again, it will add to the list, not overwrite the previous set. However, @samuel.mortenson should be able to confirm or deny that understanding.

Still, it'd be real nice to have a test of that. AFAICT it is impossible to test multiple file uploads in an automated test, so a manual test may need to suffice for this one.

My understanding is that the file which caused the error is kicked out of the list, and ignored

That is correct, but it's not very clear what you should do next and what will happen when you start uploading more files (does it overwrite files or just add to the list?). That's why removing the "partial" set seems like the best action. You get an error, your previous uploaded set failed and you should try again. When showing the list of uploaded files users might also get the idea that they are already done. These might all be solvable issues but my general feeling is we save ourselves a whole bunch of trouble just removing the partial list.

@sjerdo, this needs a bit more work dude.

+++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
@@ -232,13 +232,43 @@
+    $fids = $form_state->getValue('upload')['fids'];

This is big issue in the making. If for some reason there are any errors but no fids are in the form state values, all file entity ids will be loaded.
These will then be deleted, every single one.

That is correct, but it's not very clear what you should do next and what will happen when you start uploading more files (does it overwrite files or just add to the list?). That's why removing the "partial" set seems like the best action. You get an error, your previous uploaded set failed and you should try again. When showing the list of uploaded files users might also get the idea that they are already done. These might all be solvable issues but my general feeling is we save ourselves a whole bunch of trouble just removing the partial list.

We should probably defer this question to the UX team.

This is big issue in the making. If for some reason there are any errors but no fids are in the form state values, all file entity ids will be loaded.
These will then be deleted, every single one.

Oh lord...that's a great catch! Let's fix that by checking that $fids is not NULL.

A quick note to others who are working on this patch -- please do not iterate on @samuel.mortenson's work in #52. We're intending to demo both solutions to the UX team on Tuesday during the UX meeting, to decide which option is better to proceed with. So rather than having two "forks" of this same patch in this one issue, continue any work based on #44.

Added checks to the fids to make sure that only non temporary files and files from the submit are deleted.
Iterated these changes on top of #44.

Something went wrong in my IDE, auto formatting changed to much.

Fixed it, implemented the correct code this time and interdiff.
Based it (again) on #44 and also used the suggestion to check through ->isTemporary() from @samuel.mortenson.

Some review comments for #57:

1. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +    if (is_null($upload_value['fids'])) {
   +      return $element;
   +    }
   

   $upload_value['fids'] can be an empty array when eg. uploading only files with an incorrect file extension. You can add an empty check for an early return.

2. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +    $removeable_files = [];
   +    foreach($uploaded_files as $file) {
   +      if (!$file->isTemporary()) {
   +        continue;
   +      }
   +      $removeable_files[$file->id()] = $file;
   +    }
   

   You could use array_filter for filtering the array.
   If you still prefer filtering using foreach, you should add a space between foreach and (

Updated patch with interdiff as mentioned in #58

Started reviewing this but only scratched the surface so far. Have to jump now, though, so.... here goes.

1. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   + * An AJAX multi-step form to bulk create media entities from uploaded files.
   

   Nitpick: I guess in theory the class description should start with a verb. Not sure how to rephrase it so that it stays under 80 characters.

2. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +   * Media types the current user has access to.
   

   Nitpick: Maybe: "A list of media types..."

3. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +    switch ($this->step) {
   +      case 'select_media_type':
   +        return $this->buildMediaTypeSelectionForm($form, $form_state);
   +
   +      case 'show_media_form':
   +        return $this->buildMediaForm($form, $form_state);
   +
   +      case 'finished':
   +        return $this->buildFinishedForm($form, $form_state);
   +
   +      default:
   +        return $this->buildUploadForm($form, $form_state);
   

   Would it make sense to split this up into separate form classes analogously to #2918761: Break up MigrateUpgradeForm into smaller forms? I would say, yes, but not sure if e.g. the fact that we are heavily using Ajax here would be a reason against it.

4. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +    // Cache results if possible.
   +    if (!$this->types) {
   

   Does the "if possible" part refer to the chance of no types being returned at all? If so, I think that could be solved by checking !isset($this->types)

5. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +   * Gets a render array representing the current progress of the bulk upload.
   

   Super nitpick: s/Gets/Builds/ ?

6. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +        '#percent' => floor((($remaining - 1) / $this->originalFileCount) * 100),
   

   I think this could/should use \Drupal\Core\Batch\Percentage::format()

7. +++ b/core/modules/media/src/Form/MediaBulkUploadForm.php
   @@ -0,0 +1,607 @@
   +    $process = $this->elementInfo->getInfoProperty('managed_file', '#process');
   +    if (is_string($process)) {
   +      $process = [$process];
   +    }
   

   I don't think we support setting #process to a string?! But I guess we should pass $default = [] just in case there are no #process callbacks are predefined.

I fixed everything from #60 except 3. because I am not sure of it. It would be nice if it could be done, it would make the code much more readable.

I would like to get @samuel.mortenson opinion on it.

based on #61, marking as 'needs work'

Setting #2834729: [META] Roadmap to stabilize Media Library as the parent issue, and postponing it on #2962110: Add the Media Library module to Drupal core and the as-yet-uncreated issue to create a field widget that integrates with the media library.

Don't mind me; just the friendly neighbourhood credit fairy, pulling things over from #2962110: Add the Media Library module to Drupal core.

...aaaaand done.

Back to your regularly scheduled issue. ;)

Oh, crap. I meant to do that from #2831940: Create file field widget on top of media entity.

Credit fairy: FIRED. :P

If I understood correctly the dependency chain, this is now blocked only on #2962525: Create a field widget for the Media library module.

And now that the media library is in core, this is no longer postponed right? But sill, like said on #2938116-89: Allow media to be uploaded with the Media Library field widget the direction of this ticket should be reviewed.

Thanks! Nice freaking work. Removing the tag.

Awesome work! Hope this can sneak into 8.6❣

Parent issue was committed and pushed to 8.7.x (and will be backported to 8.6.x). Let's try to get this in 8.6.x as well :)

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -198,6 +198,42 @@
   +.media-library-upload__media,
   +.media-library-upload__file {
   +  display: flex;
   +  flex-wrap: wrap;
   +  padding: 20px 0 20px;
   +}
   

   Isn't the padding directive missing a value?

   Also, can we get some comments in this file explaining what these selectors are targeting?

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -198,6 +198,42 @@
   +.media-library-upload__source-field .file,
   +.media-library-upload__source-field .button,
   +.media-library-upload__source-field .image-preview,
   +.media-library-upload__source-field .form-type-managed-file > label,
   +.media-library-upload__source-field .file-size {
   +  display: none;
   +}
   

   I think this is okay for the time being, what with this being an experimental module, but eventually we should find a way to suppress this on the server side.

3. +++ b/core/modules/media_library/media_library.module
   @@ -42,6 +44,36 @@
   +function media_library_preprocess_views_view(&$variables) {
   

   It's too bad there's no area plugin for this. Can we open a follow-up to move this link into the actual widget itself, as per the mockups?

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +  public function testWidgetUpload() {
   

   We need to add additional coverage to this test -- for example, some more required metadata fields on some of the uploads, including complex AJAX-ey ones like images, to prove that sort of functionality works (and also that they reject invalid input correctly).

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +    foreach ($this->getTestFiles('image') as $image) {
   +      $extension = pathinfo($image->filename, PATHINFO_EXTENSION);
   +      if ($extension === 'png') {
   +        $png_image = $image;
   +      }
   +      else if ($extension === 'jpg') {
   +        $jpg_image = $image;
   +      }
   +    }
   +
   +    if (!isset($png_image) || !isset($jpg_image)) {
   +      $this->fail('Expected test files not present.');
   +    }
   

   This seems prone to random failures. Is there any way we can guarantee a PNG and a JPEG? Maybe an infinite while loop until both $png_image and $jpg_image are set?

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +    $file_storage = \Drupal::entityTypeManager()->getStorage('file');
   +    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
   +    $file_system = \Drupal::service('file_system');
   

   Let's use $this->container->get() instead of \Drupal::service().

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +    $assert_session->pageTextContains('Media library');
   +    $this->clickLink('Add media');
   

   If we use $modal_element = $assert_session->elementExists('css', '#drupal-modal'), we can press buttons and assert things in the context of the modal element, rather than the entire page. This might be useful to prevent weirdness if things are refactored or changed here.

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +    $page->attachFileToField('Upload', \Drupal::service('file_system')->realpath($png_image->uri));
   

   Let's use the existing $file_system object we got above.

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -289,2 +292,102 @@
   +    $this->assertEquals('public://type-three-dir', $file_system->dirname($file->getFileUri()));
   

   Should be assertSame().

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    $this->assertEquals($assert_session->fieldExists('Name')->getValue(), $png_image->filename);
    

    assertSame() for strings.

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    $files = $file_storage->loadMultiple();
    +    $file = array_pop($files);
    

    Why not $file_storage->load($file->id())?

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    $this->assertEquals('temporary', $file_system->uriScheme($file->getFileUri()));
    

    assertSame()

13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    // Both the type_three and type_four media types accept jpg images.
    +    $assert_session->elementExists('css', 'input[value="Type Three"]');
    +    $assert_session->elementExists('css', 'input[value="Type Four"]')->click();
    

    I think these are buttons, so we should be using $assert_session->buttonExists('Type Three')->press().

14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    $this->assertEquals('public://type-four-dir', $file_system->dirname($file->getFileUri()));
    +
    +    $this->assertEquals($assert_session->fieldExists('Name')->getValue(), $jpg_image->filename);
    

    assertSame()

15. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -289,2 +292,102 @@
    +    $assert_session->pageTextNotContains('Media library');
    

    Maybe we should assert that the modal is no longer visible instead.

16. +++ b/core/modules/media_library/config/install/core.entity_form_mode.media.media_library.yml
    @@ -0,0 +1,10 @@
    +uuid: 0d0b5c0b-3328-4082-8c80-d2891f7b97fb
    

    This probably shouldn't be here :)

17. +++ b/core/modules/media_library/config/install/core.entity_form_mode.media.media_library.yml
    @@ -0,0 +1,10 @@
    +cache: true
    

    What's this do?

18. +++ b/core/modules/media_library/config/optional/core.entity_form_display.media.image.media_library.yml
    @@ -0,0 +1,36 @@
    +content:
    +  field_media_image:
    +    type: image_image
    +    weight: 1
    +    region: content
    +    settings:
    +      progress_indicator: throbber
    +      preview_image_style: thumbnail
    +    third_party_settings: {  }
    

    Why does this media type show its source field, but the others don't?

19. +++ b/core/modules/media_library/media_library.info.yml
    @@ -5,6 +5,7 @@ package: Core (Experimental)
    +  - drupal:file
    

    No need for this; Media already depends on File and Image.

20. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +/**
    + * Creates a form to create media entities from uploaded files.
    + */
    +class MediaLibraryUploadForm extends FormBase {
    

    Form classes are internal by policy, but let's mark this explicitly internal to cover our asses.

21. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +  use MediaLibraryUploadTrait;
    

    Although the trait provides useful methods, I'm not sure we should have a trait for this stuff yet. It's an invitation for people to use this module as an API, which is not something we want at this stage. Let's remove the trait for now, and open a follow-up to move useful upload-related methods into a trait or service.

22. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +  /**
    +   * Indicates whether the 'medium' image style exists.
    +   *
    +   * @var bool
    +   */
    +  protected $mediumStyleExists = FALSE;
    

    Why make this a boolean? Let's just load the image style entirely.

23. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getEntityTypeManager() {
    +    return $this->entityTypeManager;
    +  }
    

    Why is this needed?

24. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +      if (is_string($process)) {
    +        $process = [$process];
    +      }
    

    We know that ManagedFile's #process is an array, so I don't know if we need this check.

25. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +        '#process' => array_merge(['::validateUploadElement'], $process, ['::processUpload']),
    

    I'm surprised that a method named 'validate' is going into the #process group. Perhaps we should take advantage of #element_validate here?

26. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +        if ($this->mediumStyleExists && $thumbnail_uri = $media->getSource()->getMetadata($media, 'thumbnail_uri')) {
    +          $element['preview']['thumbnail'] = [
    +            '#theme' => 'image_style',
    +            '#style_name' => 'medium',
    +            '#uri' => $thumbnail_uri,
    +          ];
    +        }
    

    The fact that we've hard-coded the medium style is...kinda icky. Can we make that a route default or something? (This is fine for a follow-up if that's easier.) Also, if there is no thumbnail, or the image style doesn't exist, $element['preview']['#access'] should probably be false.

27. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +        $form_display = EntityFormDisplay::collectRenderDisplay($media, 'media_library');
    +        $form_display->buildForm($media, $element['fields'], $form_state);
    

    Nit -- this can be one line. EntityFormDisplay::collectRenderDisplay($media, 'media_library')->buildForm().

28. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +        if (isset($element['fields']['revision_log_message'])) {
    +          $element['fields']['revision_log_message']['#access'] = FALSE;
    +        }
    

    We're going to need a more robust way of dealing with this kind of detritus, but we can defer that to a follow-up.

29. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +      $form_display = EntityFormDisplay::collectRenderDisplay($media, 'media_library');
    +      $form_display->extractFormValues($media, $form['media'][$i]['fields'], $form_state);
    

    Nit: Can be one line.

30. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +    list(,,$i,$type) = explode('-', $element['#name']);
    

    Need some spaces in that list() call :) Also, getting the type and index from the name of the element feels brittle; maybe just set properties on the element itself and use those?

31. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +    $widget_id = \Drupal::request()->query->get('media_library_widget_id');
    

    We can use $this->getRequest().

32. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +      throw new BadRequestHttpException('The "media_library_widget_id" query parameter is required and must be a string.');
    

    Can we make this a required route parameter, rather than DIY'ing it here?

33. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +      $form_state->setError($element,$this->t('A maximum of @count files can be uploaded.', [
    +        '@count' => $element['#cardinality'],
    +      ]));
    

    We should use formatPlural() here.

34. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +          $type = reset($types);
    +          $this->media[] = $this->createMediaEntity($file, $type);
    

    Nit: Let's call reset($types) inline with createMediaEntity().

35. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,464 @@
    +      $types = $this->filterTypesWithFileSource($types);
    +      $types = $this->filterTypesWithCreateAccess($types);
    

    Eventually it'd be nice to move these methods to the storage handler for media types -- let's open a follow-up to discuss.

36. +++ b/core/modules/media_library/src/MediaLibraryUploadTrait.php
    @@ -0,0 +1,152 @@
    +    return array_filter($types, function ($type) {
    +      /** @var \Drupal\media\MediaTypeInterface $type */
    

    Let's type hint $type as MediaTypeInterface.

37. +++ b/core/modules/media_library/src/MediaLibraryUploadTrait.php
    @@ -0,0 +1,152 @@
    +      return ($type->getSource() instanceof MediaFileSource);
    

    I think we should just filter on the source field's class being an instance of FileItem, rather than a concrete media source plugin class. However, this is okay for the moment; let's open a follow-up if changing this now is too complicated.

Awww yeah. Very few things to complain about now.

1. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ElementInfoManagerInterface $element_info) {
   

   This shouldn't be @inheritdoc.

2. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +      $process = $this->elementInfo->getInfoProperty('managed_file', '#process', []);
   +      if (is_string($process)) {
   +        $process = [$process];
   +      }
   

   No need to change this here, but pro-tip: if you cast a string to array, you get [$string] :)

3. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +        '#process' => array_merge(['::validateUploadElement'], $process, ['::processUpload']),
   

   Let's open a follow-up, and put a @todo here, to separate validation logic from #process (we might have to do it by overriding the #value_callback, but that's a potentially infinite pain train so let's not do it right now).

4. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +      foreach ($this->media as $i => $media) {
   

   Can we move the contents of this foreach loop into a helper method?

5. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +        if ($this->mediumStyleExists && $thumbnail_uri = $media->getSource()->getMetadata($media, 'thumbnail_uri')) {
   +          $element['preview']['thumbnail'] = [
   +            '#theme' => 'image_style',
   +            '#style_name' => 'medium',
   +            '#uri' => $thumbnail_uri,
   +          ];
   +        }
   

   If the medium style doesn't exist, there will be no preview image. That's kind of unfortunate; let's at least open a follow-up to address this later or make it configurable.

6. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +      foreach ($this->files as $i => $file) {
   

   Let's also move the contents of this foreach loop into a helper method.

7. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +      if ($file instanceof FileInterface) {
   +        $file->setPermanent();
   +        $file->save();
   +      }
   

   Why wouldn't $file be an instance of FileInterface? This could use a comment.

8. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +    $i = $element['#media_library_index'];
   +    $type = $element['#media_library_type'];
   

   Let's throw exceptions if either of these properties are empty.

9. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,602 @@
   +    $this->media[] = $this->createMediaEntity($this->files[$i], $this->types[$type]);
   

   Should we use $this->getTypes()[$type] instead?

10. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,602 @@
    +  public function processUpload(array $element, FormStateInterface $form_state) {
    

    Can this be renamed to processUploadElement, for clarity?

11. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,602 @@
    +    file_prepare_directory($location, FILE_CREATE_DIRECTORY);
    +    $file = file_move($file, $location);
    

    Both of these functions will return FALSE on error, so we should account for those cases by throwing exceptions.

12. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,602 @@
    +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
    +   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
    +   */
    +  protected function getTypes() {
    

    We don't need the @throws tags, since this method never explicitly throws.

13. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,602 @@
    +   *   A File Entity.
    

    Supernit: "A file entity"

14. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -0,0 +1,602 @@
    +      return (is_a($type->getSource()->getSourceFieldDefinition($type)->getClass(), FileFieldItemList::class, TRUE));
    

    Nit: There's a superfluous pair of parentheses here.

Oops, forgot to change status.

+++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
@@ -252,26 +254,66 @@
+      $this->submitMediaForm($media, $form['media'][$i]['fields'], $form_state);

Ah -- sorry for the miscommunication!! The logic I wanted moved into helper methods is the stuff that builds all of the preview/metadata fields for each media item or uploaded file. Sorry about that! #phenaisanidiot

All my code feedback is addressed. We're awaiting some testing and input from @DyanneNova for UX, and then we're RTBC once green on all backends.

+++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
@@ -0,0 +1,615 @@
+                'media-library-upload__media__preview',
...
+                'media-library-upload__media__fields',
...
+            '#markup' => '<strong class="media-library-upload__file__label">' . $this->t('Select a media type for %filename:', [

In my opinion double underscore should only appear once in a class name. BEM doesn't really specify this kind of rule but in my opinion it is something we should choose to not do in Drupal.

The naming convention is there to help you identify relationships with the top level component block. If we were to use the double underscore multiple times the class names would quickly get out of control making the class names hideous and unreadable.

Interdiff looks good to me.

@samuel.mortenson and I demoed this patch to @webchick. She was, I'm pleased to say, pretty blown away. There were only two commit-blocking points of concern, both addressed in #116. We also looked over the code, and found nothing untoward.

So, given:

• This patch is critical functionality for the Media Initiative, and an 8.6 target;
• @webchick, the world's most talented patch breaker, tried it and it worked beautifully;
• It passed front-end framework manager review, with all feedback (which was minor) addressed;
• Follow-ups are filed for additional work (including stable gates);
• @webchick received a guided tour of the code and thought it looked good;
• The patch has now been tested and reviewed by a product manager;
• No new APIs whatsoever are introduced, nor are existing core APIs used in weird ways; and
• There is extensive test coverage...

...I think it's time to land this.

1. +++ b/core/modules/media_library/media_library.module
   @@ -42,6 +44,36 @@ function media_library_theme() {
   +        '#title' => t('Add media'),
   +        '#url' => $url,
   

   Why not use local action for this and attributes in media_librar_template_preprocess_menu_local_action?

2. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,615 @@
   +    $form['#prefix'] = '<div id="media-library-upload-wrapper">';
   +    $form['#suffix'] = '</div>';
   

   This can be wrapper '#type' => 'container', with '#id'. See https://github.com/drupal/drupal/blob/8.6.x/core/modules/system/tests/mo...

3. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
   @@ -0,0 +1,615 @@
   +      $file->save();
   

   This is not needed media save will save the file.

2. Nope, it can't. ajax wrapper ID's *must* put there like that because #id will be uniqified on ajax requests and then it will be ...--hash and then the replacement will not work anymore. That said, I have no clue why we aren't using our very own drupal-identifier things instead and also enforce that an ajax identifier must be an ID instead of a that or a class.

Why not use local action for this and attributes in media_librar_template_preprocess_menu_local_action?

Because they are not local actions; they are arbitrary buttons and must be rendered as such. It's not our fault that local actions and primary buttons have similar styling, and that's way out of scope for us to address here. Trying to make that button into a local action will likely remove it from the modal, and cause more problems than it solves (routing + modal = migraine). We can explore that path in a follow-up, if you like, but it should not block commit.

This is not needed media save will save the file.

That's news to me; can you point out where that happens in Media?

Nope, it can't. ajax wrapper ID's *must* put there like that because #id will be uniqified on ajax requests and then it will be ...--hash and then the replacement will not work anymore.

This is simply not correct. A lot of people think the container #id will be uniqified that's why I linked the example also please have a look at template_preprocess_container

    // Assign an html ID.
    if (!isset($element['#attributes']['id'])) {
      $element['#attributes']['id'] = $element['#id'];
    }

Drupal doesn't touch that id it is passes as is everytime.

This can be wrapper '#type' => 'container', with '#id'

It is much more important to land this functionality in 8.6 before beta than it is to make sure that the wrapper ID is set in a specific way. Especially given that this module is experimental.

Please, let's open a follow-up issue to fix that in beta. There is no reason why that cannot change during beta, but unless we land this now, we cannot get this much-needed feature into core.

That's news to me; can you point out where that happens in Media?

This is not mdia magic this is ER field magic. See \Drupal\Tests\field\Kernel\EntityReference\EntityReferenceItemTest::testEntityAutoCreate

IMO $form['#prefix'] and $form['#suffix'] should never be used to add html becaue it breaks stuff all the time.

Can't find anything right now, but I know I've had problems with setting id through #id in the past. prefix.+id=".+wrapper" finds 26 matches in core, so if you think it should work with #id, maybe open an issue to change all of them?

This is not mdia magic this is ER field magic. See \Drupal\Tests\field\Kernel\EntityReference\EntityReferenceItemTest::testEntityAutoCreate

The media item is what's being referenced by the ER field; the file is being referenced by the media item's source field. I'm still not clear on how EntityReferenceItemList is be smart enough to know to save both; the test does not make it clear. I would be quite surprised to learn that it knows to save references-within-references.

Additionally, the logic of this form is rather unlike any other form or entity reference widget in core. It's best, for now, to stick with what is working. If we want to delegate the save logic to Entity Reference, we should explore it in a follow-up (which I'm sure could land in beta). It should not block commit of this patch.

+++ b/core/modules/media_library/media_library.module
@@ -42,6 +44,36 @@ function media_library_theme() {
+function media_library_preprocess_views_view(&$variables) {
...
+      $variables['header']['add_media'] = [

Because they are not local actions; they are arbitrary buttons and must be rendered as such.
I understand that. We shouldn't be adding link to views header like that. This should be converted to view area plugin so that user can remove the block if needs be. This makes customization of the this view harder for non-dev site builder user.

This should be converted to view area plugin so that user can remove the block if needs be.

100% agreed, and I in fact mentioned that in an earlier round of review. However, it's out of scope for now; I think we filed a follow-up. If not, I'll make sure we have one :)

Follow-up filed: #2988451: Add an area plugin to render a link

@phenaproxima and @samuel.mortenson sat and walked me through the patch and its functionality a bit earlier today. I have no idea how you guys pulled this off, but it looks and works AMAZINGLY!! Exactly what was in the mocks, in terms of embedding multiple media entry forms in the same place. (The required alt text is definitely a friction point, but I understand its importance and why we force that on default installs.) The tests are comprehensive, as is the validation for various edge cases (such as upload a file covered by multiple media types).

The patch is entirely UI-facing, and entirely contained in experimental code, so relatively low risk of commit. Given the enormous end-user impact this patch has on 8.6's release, and given that the unresolved feedback in #118 and below seems like it can be solved in follow-up issues (if it's deemed valid; sounds like there's some disagreement there), I'm going to go ahead and commit this so we can get it in for 8.6's beta.

Committed and pushed to 8.7.x, cherry-picked to 8.6.x. YEAH!!!!! :D

This is an amazing work, thanks for everyone that worked on this!

As a follow-up, I've discovered #2988617: Creating media with the media library upload is broken for unlimited cardinality while testing this today.

Thanks again!