Immediately delete a media's file when pressing 'Remove' in the media creation modal

Should we add a usage/status check before deleting to be sure?

I could imagine a few scenarios where it's not guaranteed to have a 1:1 relationship between media and file entities. For example, an automatic file deduplication module that would re-use an existing file if you upload one that was already used.

Nitpicks:

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -556,9 +572,16 @@ public function removeButtonSubmit(array $form, FormStateInterface $form_state)
+    // Immediately discard its file.

'... when the file is not in use anywhere.' or something? On its own this comment doesn't add much.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -83,6 +94,11 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Med
+      @trigger_error('The file.usage service must be passed to AddFormBase::__construct(), it is required before Drupal 9.0.0.', E_USER_DEPRECATED);

This normally needs a change record and a 'See' to that change record. Is see this didn't happen for the $opener_resolver change, maybe it's not needed for experimental modules?

Removing defunct "D8Media" tag.

Thank you for writing a test of this! That will be hugely helpful in committing this. Would you mind posting a fail patch (i.e., one that only contains the new test coverage), so we can be sure that the tests prove that the bug is fixed? :)

The patch looks pretty damn good to me.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -83,6 +94,11 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Med
+    if (!$file_usage) {
+      @trigger_error('Calling AddFormBase::__construct() without the file.usage service is deprecated in drupal:8.8.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/3075165', E_USER_DEPRECATED);
+      $file_usage = \Drupal::service('file.usage');
+    }
+    $this->fileUsage = $file_usage;

I think my only major complaint about this patch is that we don't need to worry about this file usage stuff in the OEmbed add form. So let's only add this to the FileUploadForm class, and only do this removal stuff there.

This looks very, very good to me. Thanks for working on it!

+++ b/core/modules/media_library/src/Form/FileUploadForm.php
@@ -68,12 +76,15 @@ class FileUploadForm extends AddFormBase {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, MediaLibraryUiBuilder $library_ui_builder, ElementInfoManagerInterface $element_info, RendererInterface $renderer, FileSystemInterface $file_system, OpenerResolverInterface $opener_resolver, FileUsageInterface $file_usage) {
     parent::__construct($entity_type_manager, $library_ui_builder, $opener_resolver);
     $this->elementInfo = $element_info;
     $this->renderer = $renderer;
     $this->fileSystem = $file_system;
+    $this->fileUsage = $file_usage;

I think we'll want an E_USER_DEPRECATED error for $file_usage here, similarly to what you had in a previous patch; it's quite likely that this class has been extended, so it's probably safer for us to have a deprecation notice for the new parameter. I just wanted to move it into FileUploadForm, rather than have it in AddFormBase.

I don't see anything else particularly wrong with this patch, but I'd ideally like @seanB to sign off as a fellow subsystem maintainer, and get a framework manager to commit it. File handling, especially usage tracking, can be tricky/flaky, so I want to be sure I'm not missing anything.

Apart from the things phenaproxima already noticed, this is looking good to me. So +1.

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -68,12 +76,15 @@ class FileUploadForm extends AddFormBase {
   +    $this->fileUsage = $file_usage;
   

   Yep, this needs a deprecation notice.

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -290,4 +302,30 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    $file = $removed_media->get($this->getSourceFieldName($removed_media->bundle->entity))->entity;
   +    if ($file instanceof FileInterface && empty($this->fileUsage->listUsage($file))) {
   

   Checking the usage is probably no problem. Usage is not 100% reliable, but this is better than always blindly deleting the file.

FWIW, I think change records on constructor argument changes are not necessary and just add noise, we've done hundreds of these. Both controllers and constructors are @internal, we're just being nice here. Base classes are a bit different, but we usually don't do change records for that either :) Now that you have it, possibly fine to keep?

Looks good to me. Thanks!

This is at least major and maybe critical-ish because of the impacts on data integrity and privacy expectations. Thanks!

+++ b/core/modules/media_library/src/Form/FileUploadForm.php
@@ -68,12 +76,19 @@ class FileUploadForm extends AddFormBase {
+      @trigger_error('Calling FileUploadForm::__construct() without the file.usage service is deprecated in drupal:8.8.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/3075165', E_USER_DEPRECATED);

This is internally contradictory. I think we mean "and the $file_usage argument will be required in 9.0.0"?

OK settling on "major" for now, pending the following information:

• What happens when you follow the equivalent upload and "Remove" steps using the default file upload widget?
• Is the data deleted later, after the whole form is submitted, or is it never deleted?

An IS update with more specific steps to reproduce and a comparison to the core widget will help us here. Thanks!

> What happens when you follow the equivalent upload and "Remove" steps using the default file upload widget?

The same that happens with media AFAIK, the file never becomes permanent and is then eventually deleted after the configured timeframe (6h).

IMHO, this is a small improvement that isn't actually that big of a deal. Compared to the mess that we havewith not actually ever marking a file as temporary again based on file usage (by default) and having no automated or manual way at all to delete a file again in core ;)

Thanks idebr! Everything seems to be addressed. Also updated the steps to reproduce in the IS. RTBC!

To clarify, the remove button when adding media through media/add is the remove button of the source field. For local files this is a managed file element. This executes file_managed_file_submit() . This deletes the actual created file entity.

For the media library add form, the remove button in step 2 is a custom remove button, not the remove button of the source field. This executes Drupal\media_library\Form\AddFormBase::removeButtonSubmit() and only removes the data from the add form, ensuring no new media item is created. The actual uploaded file is not deleted.

That is why this patch is a great improvement.

Cleaning up the IS to clarify what's going on here.

Let's confirm that a normal file field and a media library item with this patch behave the same way when deleting a file (individual steps and what happens to the file at each step). Thanks!

Starting from 8.8.x HEAD with this patch applied, on the Standard profile with Media Library installed, and logged in as user 1:

Here's how file fields behave (this is what we want Media Library to do as well):

1. Go to /node/add/article. You'll see an Image field. This is a standard image field, which has identical file storage behavior as file fields.
2. Upload an image into the field. You'll see that it's now in /sites/default/files/[YYYY-MM].
3. Click the "Remove" button for the image field. You'll see that the file is instantly removed from the file system.

Now, add a media field to the Article content type. Just use the default settings, any cardinality, and ensure that the Image media type is one of the ones that can be referenced by the field.

1. Go to /node/add/article. You'll see the new field you added.
2. Click the "Add media" button.
3. In the modal, you'll see the "Add files" area. Upload an image into it. You'll see it's now in /sites/default/files/[YYYY-MM].
4. Click the little "X" button at the top right of the new, unsaved media item (NOT the button to close the modal).
5. You'll see that the file has been instantly removed from the file system. Without this patch, this doesn't happen; the file remains where it is until it's cleaned up later by cron.

So, this worked for me as expected. Restoring RTBC.

Adding a release note.

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -68,12 +76,19 @@ class FileUploadForm extends AddFormBase {
   +      @trigger_error('Calling FileUploadForm::__construct() without the file.usage service is deprecated in drupal:8.8.0 and the file usage service will be a required argument in drupal:9.0.0. See https://www.drupal.org/node/3075165', E_USER_DEPRECATED);
   

   Can we align this with the proposed standard in #3024461: Adopt consistent deprecation format for core and contrib deprecation messages - @xjm asked about this earlier too

   "... and the $file_usage argument will be required in drupal:9.0.0..."

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -290,4 +306,30 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    if ($file instanceof FileInterface && empty($this->fileUsage->listUsage($file))) {
   +      $file->delete();
   +    }
   

   Can we have additional test coverage for when there is a file usage record - that ensures the file is not deleted?

   Thanks!

Addressing #39:
1) Adding test coverage for file not being deleted if in use.
2) Updating the deprecation notice to follow the recommended pattern.
3) I made a few changes because of random failures. For example:

-    $assert_session->assertWaitOnAjaxRequest();
+    $this->assertNotEmpty($assert_session->waitforButton('Remove'));

4) I made one coding standard change.

@@ -237,8 +237,8 @@ public function testWidgetWithoutMediaTypes() {
     $field_empty_types_message = 'There are no allowed media types configured for this field. <a href="' . $field_empty_types_url->toString() . '">Edit the field settings</a> to select the allowed media types.';
 
     $field_null_types_url = new Url('entity.field_config.node_field_edit_form', [
-        'field_config' => 'node.basic_page.field_null_types_media',
-      ] + $route_bundle_params);
+      'field_config' => 'node.basic_page.field_null_types_media',
+    ] + $route_bundle_params);

Looks great except for three very small things.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1352,6 +1354,31 @@ public function testWidgetUpload() {
   +    // When a file is already in usage, it should not be deleted. To test,
   +    // let's add a usage for the second file.
   

   Isn't this a usage for the third file, not the second?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1352,6 +1354,31 @@ public function testWidgetUpload() {
   +      'field_media_test_image' => [
   +        [
   +          'target_id' => $target_file->id(),
   +          'alt' => 'I find your lack of faith disturbing.',
   +          'title' => 'I find your lack of faith disturbing.',
   +        ],
   

   We can probably remove the 'title' key :)

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1352,6 +1354,31 @@ public function testWidgetUpload() {
   +    // Assert the file was not deleted, due to being in use elsewhere.
   +    $this->assertNotEmpty($file_storage->loadByProperties(['filename' => $filenames[3]]));
   

   Can we also assert that the physical file exists? Something like:

   $files = $file_storage->loadByProperties([
     'filename' => $filenames[3],
   ]);
   $this->assertNotEmpty($files);
   $this->assertFileExists(reset($files)->getFileUri());
   

Responding to #40:
1. ✅Yes, now that I think about it, we removed the second file, but this was the fourth, so it's now in the third position. Counting under five is hard.
2. ✅Updated
3. ✅ Now with assertion that the physical file exists!

Still looks good, but a few more requests :)

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -290,4 +306,30 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    /** @var \Drupal\media\MediaInterface[] $added_media */
   +    $added_media = $form_state->get('media');
   +    $removed_media = $added_media[$delta];
   

   Teensy nit: This can be $removed_media = $form_state->get(['media', $delta]). No need for $added_media to be its own variable.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1335,19 +1334,51 @@ public function testWidgetUpload() {
   +    // Assert the file was deleted.
   +    $this->assertEmpty($file_storage->loadByProperties(['filename' => $filenames[1]]));
   

   We should probably also do a $this->assertFileNotExists(), to ensure it was physically deleted.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1335,19 +1334,51 @@ public function testWidgetUpload() {
   +    // When a file is already in usage, it should not be deleted. To test,
   +    // let's add a usage for the fourth file (now in the third position).
   

   Why is the fourth file now in the third position? What does that mean?

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1335,19 +1334,51 @@ public function testWidgetUpload() {
   +      'field_media_test_image' => [
   +        [
   +          'target_id' => $target_file->id(),
   +          'alt' => 'I find your lack of faith disturbing.',
   +        ],
   

   No real need for the 'alt' key either, but I'll let it slide. :)

Addressing #42
1. ✅ Updated. We should probably add an issue to update this in AddFormBase too:

    $added_media = $form_state->get('media');
    $removed_media = $added_media[$delta];

    // Update the list of added media items in the form state.
    unset($added_media[$delta]);

    // Update the media items in the form state.
    $form_state->set('media', $added_media)->setRebuild();

2. ✅ Added the assertion.
3. Literally, there are three images on the page, the fourth of the original four is now the third on the page. See the screenshot (on the next . comment). Perhaps we should refer to them differently? I updated it to refer to the variable name to be clearer.

I ran into frequent random failures. I added fixes as they were blocking me. But perhaps, I should leave them out and leave them for #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest.

Here's a patch with the fixes. I'll try to post one without too.

Screenshot referred to in #43.3:

Same as #43, but in two versions:
1) One patch has all the extraneous changes removed.
2) The second patch has changes to fix the test when I was experiencing random failures on my local while running the test ::testWidgetUpload(), and one coding standard fix. I experienced many random failures in different places. Lots of room for improvement here.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1336,19 +1336,51 @@ public function testWidgetUpload() {
   +    $target_file = reset($files);
   +    $this->assertCount(1, $files);
   

   These lines should be swapped. If they're not, and $files is empty, then we'll be calling methods on FALSE, which will cause the test to die with a fatal error.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1336,19 +1336,51 @@ public function testWidgetUpload() {
   +    $this->assertJsCondition('jQuery(".media-library-add-form__media[data-media-library-added-delta=2]").is(":focus")');
   

   Why are we asserting this focus? We didn't change focus behavior elsewhere in the patch, so I'm not sure why this assertion is here? (I'm not complaining, of course; focus assertions are useful.)

RTBC for the no-extra-changes patch, once the first point is fixed. Please go right ahead and set this issue to that status once a new patch is up.

Addressing #46.
1. ✅Swapped the lines.
2. ✅ Dropped the focus assertion. I was following the pattern above, but I don't think it's necessary here.

Seemingly unrelated random Postgres failure on #45, so I’ve queued #47 on all backends. Hopefully it passes; RTBC once it does.

Saving issue credit. Everyone seems to have contributed positively to the solution here, so...

Ahem.

There we go.

This is a highly sensible patch that brings Media inline with Image/File fields. +1. I manually tested various permutations and couldn't find any problems (shock and horror!).

Committed and pushed to 8.8.x. Thanks a ton!! One fewer must-have out of the way. :D