Move Media::save() logic into storage handler to prevent data integrity issues when media items with remote content are saved

This should be a push in the right direction.

Already found some things to improve. Also saw the tests were failing so added a fix for that as well.

Re #6.2, why is this patch adding the saveFromQueue() method at all? I thought the scope of this issue was *only* moving Media::save() to MediaStorage::save(). Everything else here seems out of scope for the Critical issue, and rather the scope of #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction. Or am I misunderstanding something?

If I'm right though, then I think that would mean no new interface needed yet by MediaStorage, and instead the queue worker could typehint to EntityStorageInterface, since all it would need to call is save().

Re #6.2, why is this patch adding the saveFromQueue() method at all? I thought the scope of this issue was *only* moving Media::save() to MediaStorage::save().

The code in Media::save() checks several things to determine if the thumbnail or metadata should be updated. Checking if the source field changed was the most important. When we move the hasSourceFieldChanged method to check if the source field changed, the Media::shouldUpdateThumbnail() makes no sense anymore, since it relied on that method being in Media and not the Storage. This led to a whole chain of things depending on each other (which was a big reason why we wanted to refactor it in the first place).

When updating the thumbnail becomes part of the storage, saving from the queue becomes more complex. The storage now needs to know if it's being saved from a queue or not. That's why the saveFromQueue() was also introduced in this patch. I personally agree with phenaproxima that we shouldn't add this to the API, but in #2878119-109: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction it was suggested we should add it.

First of all, I really apologize for not helping #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction land sooner. I'll do my best to help it land as soon as possible into 8.7.

However, now that we're already past 8.6 beta and with the RC being this week, I'd really like to keep this patch's changes as small as possible. What do you think of something like this?

For 8.6, I don't want to change the queue worker or anything else that's currently calling $media->save() to calling the storage handler's save() instead. That way, if there are existing modules out there that override the Media entity type class, they can keep working as they do now. However, what this patch does is simply makes core's Media class functional for contrib code that calls the storage handler's save() method directly, which is a legitimate thing for contrib to do, and which is why this issue is Critical. If there's agreement on this approach, I think all that's left is to add a test for that exact scenario (invoking the storage handler's save() directly).

+++ b/core/modules/media/src/PrepareSaveInterface.php
@@ -0,0 +1,27 @@
+ * @todo Consider making this a non-internal interface in Drupal 8.7.

I opened #2992426: Add entity methods and hooks for executing pre/post code outside of the save transaction to propose adding an API for all entity types to have the ability to run some of their save() code outside of the transaction.

Good point. How's this?

#13 seems like a pretty good compromise to me as well. The only question I had is what impact swapping out the storage handler would have on an existing site or module. It seems like at a minimum we should guarantee a cache clear to ensure an up-to-date entity definition?

we're marking it internal... which gives us the latitude to remove it in a minor release if we need to.

Well, not exactly: https://www.drupal.org/core/deprecation#internal

But yes, I agree with marking it internal and with not adding an interface until such time as it becomes a part of the parent API.

It seems like at a minimum we should guarantee a cache clear to ensure an up-to-date entity definition?

Done.

I think all that's left is to add a test for that exact scenario (invoking the storage handler's save() directly)

I still think that's a good idea to do, but not sure if it can be done in time for RC's commit freeze.

Not sure I agree with changing the existing some of the Media::save() calls to the storage save. I can see it happen that someone changes this back later without realising the purpose of those changes.

Can't we just have a specific test to verify there is no difference in saving from the storage or the entity?

Something like this (didn't test this yet, but it should give you an idea):

  /**
   * Tests that the media storage saves media the same way the entity does.
   */
  public function testMediaSave() {
    $field_name = 'field_to_map_to';
    $attribute_name = 'attribute_to_map';
    $storage = FieldStorageConfig::create([
      'entity_type' => 'media',
      'field_name' => $field_name,
      'type' => 'string',
    ]);
    $storage->save();
    FieldConfig::create([
      'field_storage' => $storage,
      'bundle' => $this->testMediaType->id(),
      'label' => 'Field to map to',
    ])->save();

    // Define mapping and make sure that the value was stored in the field.
    \Drupal::state()->set('media_source_test_attributes', [
      $attribute_name => ['title' => 'Attribute to map', 'value' => 'Snowball'],
    ]);
    $this->testMediaType->setFieldMap([$attribute_name => $field_name])->save();

    // Assert the media item saves itself correctly.
    $media = Media::create([
      'bundle' => $this->testMediaType->id(),
      'field_media_test' => 'some_value',
    ]);
    $media->save();
    $this->assertSame('media:' . $media->bundle() . ':' . $media->uuid(), $media->getName(), 'Default name was not saved correctly.');
    $this->assertSame('Snowball', $media->get($field_name)->value, 'Metadata attribute was not mapped to the field.');

    // Assert the media storage saves the media item correctly.
    $media = Media::create([
      'bundle' => $this->testMediaType->id(),
      'field_media_test' => 'some_value',
    ]);
    $this->storage->save($media);
    $this->assertSame('media:' . $media->bundle() . ':' . $media->uuid(), $media->getName(), 'Default name was not saved correctly.');
    $this->assertSame('Snowball', $media->get($field_name)->value, 'Metadata attribute was not mapped to the field.');
  }

Loving it! Just on minor last thing, wouldn't it be better to check the actual values we expect instead of checking if the values are not empty?

+++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
@@ -15,6 +15,73 @@
+    $this->assertFalse($a->get('name')->isEmpty());
+    $this->assertFalse($b->get('name')->isEmpty());
...
+    $this->assertFalse($a->get('field_to_map_to')->isEmpty());
+    $this->assertFalse($b->get('field_to_map_to')->isEmpty());

Fair enough! I think this is ready. Since I didn't write any code in the final patch I should be able to RTBC this.

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!