Expose triggering update of media metadata + thumbnail to end users

I like your proposal @marcoscano, but I think this will need usability review. Additionally, I think we'll need to block this work on anything except #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction, since that finalizes the metadata updating API. Therefore, I'm postponing on that issue.

I like these ideas. I think it would make sense to use the BatchAPI for updating multiple media items.

Let's link #2603820: [PP-1] Allow selecting all entities in a given page or in every pages for bulk operations.

1. +++ b/core/modules/media/config/optional/views.view.media.yml
   @@ -182,7 +182,8 @@ display:
   +            - media_update_metadata
   
   +++ b/core/modules/media/media.routing.yml
   @@ -25,6 +25,17 @@ entity.media.revision:
   +media.update_metadata:
   ...
   +  requirements:
   +    _entity_access: 'media.update'
   

   Do we need to protect this with a CSRF token?

2. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,38 @@
   +    $request = \Drupal::request();
   +    if ($request->server->get('HTTP_REFERER')) {
   +      return $request->server->get('HTTP_REFERER');
   +    }
   

   Shouldn't we use something like a destination parameter for this?

A small UX improvement would be if the link text was something like Update from <source>. "Update from YouTube" seems much more straightforward than "metadata". Possibly even "Refresh"?

Wondering/hopeful whether there are any updates here?

I've just updated to 8.7 and these patches don't appear to apply (via composer), and the features do not seem to be integrated OOTB. I do realize it's a moving target with moving parts, etc.

Alternatively, is there an effective workaround to manually update the media thumbnails?

Based on #2878119-135: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction, it sounds like the media maintainers no longer think #2878119 is a good idea, that issue should be won't fix'ed, and we should plow ahead here with just the updateMetadata() API change for media entities along with all the nice UI additions.

This needs work for:

• Re-rolling to apply against 8.9.x
• To not bring anything else from #2878119 beyond updateMetadata()
• Rename media_update_8601()
• ...

Trying to re-roll #2878119 manually really sucked. ;) I'd rather not have to try to do that again. If someone's already got this on a branch that would be "easy" to rebase, that'd be swell. I'd be happy to try to move it forward from there.

Thanks!
-Derek

Here is a good starting point. This is based on 2983456-7-this-patch-only-do-not-test.patch in #7.
I think all we need is some tests (also for the update path to install the new action) and a UX review.

Also decided to not disable the new action in the media/media_library views by default, even though the batch API is not used. In theory updating metadata could be slower than other action, but I don't think this is necessarily a reason not to expose the option to users. I've tried updating 50 youtube video's (since that is all we show on 1 page), and that seemed to work fine, took about 2,5 seconds on my local machine.

Hopefully this helps!

Some more thoughts for further iterations.

1. +++ b/core/modules/media/src/Entity/Media.php
   @@ -391,6 +391,30 @@ public function prepareSave() {
   +    foreach ($this->translations as $langcode => $data) {
   +      if (!$this->hasTranslation($langcode)) {
   

   Use foreach (array_keys($this->getTranslationLanguages()) as $langcode)?
   $this->hasTranslation($langcode) shouldn't be needed.

2. +++ b/core/modules/media/src/Entity/Media.php
   @@ -391,6 +391,30 @@ public function prepareSave() {
   +      foreach ($translation->bundle->entity->getFieldMap() as $metadata_attribute_name => $entity_field_name) {
   +        if ($translation->hasField($entity_field_name)) {
   +          $translation->set($entity_field_name, $media_source->getMetadata($translation, $metadata_attribute_name));
   +        }
   +      }
   

   Move this to a new protected updateMappedMetadata method, it is duplicate code (also in prepareSave()).

3. +++ b/core/modules/media/src/Entity/Media.php
   @@ -391,6 +391,30 @@ public function prepareSave() {
   +      $translation->updateThumbnail();
   

   We need to check what to do if the source does not have a thumbnail. The updateThumbnail method uses a default thumbnail when that happens, but not sure if we need additional logic to skip a thumbnail update in that case?

Thanks, @seanB!

A huge # of those test failures are this:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.action.media_update_metadata with the following errors: system.action.media_update_metadata:configuration missing schema
 

I'm going to do a quick re-roll to include that and see how much closer to green that gets us. Stay tuned...

Ugh, having troubles with my local test environment again. :( Uploading here so the bot can verify the results. Hopefully this should help.

Amazing, that's green. ;)

So, back to NW for:
- Feedback in #16 (which all seems legit and worth fixing)
- Tests

1. +++ b/core/modules/media/media.install
   @@ -5,6 +5,8 @@
    
   +use Drupal\Core\Config\FileStorage;
   +use Drupal\Core\Config\InstallStorage;
    use Drupal\Core\Entity\Sql\SqlEntityStorageInterface;
    use Drupal\Core\File\Exception\FileException;
    use Drupal\Core\File\FileSystemInterface;
   @@ -12,6 +14,7 @@
   
   @@ -12,6 +14,7 @@
    use Drupal\media\Entity\MediaType;
    use Drupal\media\MediaTypeInterface;
    use Drupal\media\Plugin\media\Source\OEmbedInterface;
   +use Drupal\system\Entity\Action;
   

   These don't seem to be used?

2. +++ b/core/modules/media/media.install
   @@ -257,3 +260,21 @@ function media_update_8700() {
   +function media_update_8900() {
   +  if (!Action::load('media_update_metadata')) {
   +    \Drupal::entityTypeManager()
   +      ->getStorage('action')
   +      ->create([
   +        'id' => 'media_update_metadata',
   +        'label' => 'Update metadata',
   +        'type' => 'media',
   +        'plugin' => 'media_update_metadata',
   +      ])
   +      ->trustData()
   +      ->save();
   +  }
   +}
   

   I could be wrong, but I think the fact that this is using the entity API means that it should be a post-update function.

3. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,42 @@
   +/**
   + * Class UpdateMetadataController.
   + */
   +class UpdateMetadataController extends ControllerBase {
   

   This probably needs a better doc comment. :) Also, maybe we should tag it explicitly @internal like so:

   @internal
     Controllers are internal.
   

4. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,42 @@
   +    $media->updateMetadata()->save();
   

   I know it's more verbose, but for DI reasons, can we use $this->entityTypeManager()->getStorage()->save()?

5. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,42 @@
   +    \Drupal::messenger()->addStatus($this->t('Updated metadata on media item %label', [
   +      '%label' => $media->label(),
   +    ]));
   

   We should be using $this->messenger().

6. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,42 @@
   +    if (\Drupal::request()->query->has('destination')) {
   +      return \Drupal::destination()->get();
   +    }
   

   I think we can just use $this->getRedirectDestination() here.

7. +++ b/core/modules/media/src/Entity/Media.php
   @@ -394,6 +394,30 @@ public function prepareSave() {
   +  public function updateMetaData() {
   

   I think we usually treat "metadata" as one word, so this should be updateMetadata().

8. +++ b/core/modules/media/src/Entity/Media.php
   @@ -394,6 +394,30 @@ public function prepareSave() {
   +    foreach ($this->translations as $langcode => $data) {
   

   I don't think we should be using $this->translations directly. Classes should use their own APIs. So $this->getTranslations() is preferable.

9. +++ b/core/modules/media/src/Plugin/Action/UpdateMetadataAction.php
   @@ -0,0 +1,35 @@
   +  public function execute($entity = NULL) {
   +    /** @var \Drupal\media\MediaInterface $entity */
   +    $entity->updateMetadata()->save();
   +  }
   

   Can we inject the entity storage handler instead?

1. +++ b/core/modules/media/src/Entity/Media.php
   @@ -364,29 +364,62 @@ public function prepareSave() {
   +      if ($override_existing) {
   +        $should_map_field = TRUE;
   +      }
   +      else {
   +        // If not explicitly requested by the caller, the field will be mapped
   +        // if it's empty, or if the source field changed.
   +        $should_map_field = $translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged();
   +      }
   +      if ($translation->hasField($entity_field_name) && $should_map_field) {
   +        $translation->set($entity_field_name, $media_source->getMetadata($translation, $metadata_attribute_name));
          }
   

   $should_map_field = $override_existing || ($translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged())?

   Also, we do the $translation->hasField($entity_field_name) check after $translation->get($entity_field_name)->isEmpty()?

   Maybe this is even better:

   if (!$translation->hasField($entity_field_name)) {
     continue;
   }
   
   if ($override_existing || $translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged()) {
     $translation->set($entity_field_name, $media_source->getMetadata($translation, $metadata_attribute_name));
   }
   

1. +++ b/core/modules/media/media.install
   @@ -8,14 +8,14 @@
   +use Drupal\Core\StringTranslation\TranslatableMarkup;
    use Drupal\Core\Url;
   +use Drupal\image\Plugin\Field\FieldType\ImageItem;
    use Drupal\media\Entity\MediaType;
    use Drupal\media\MediaTypeInterface;
    use Drupal\media\Plugin\media\Source\OEmbedInterface;
    use Drupal\user\Entity\Role;
    use Drupal\user\RoleInterface;
   -use Drupal\image\Plugin\Field\FieldType\ImageItem;
   -use Drupal\Core\StringTranslation\TranslatableMarkup;
   

   Probably an automatic IDE fix; technically out of scope, but eh...I don't care much.

2. +++ b/core/modules/media/media.post_update.php
   @@ -97,3 +98,21 @@ function media_post_update_add_status_extra_filter() {
   +    \Drupal::entityTypeManager()
   +      ->getStorage('action')
   +      ->create([
   +        'id' => 'media_update_metadata',
   +        'label' => 'Update metadata',
   +        'type' => 'media',
   +        'plugin' => 'media_update_metadata',
   +      ])
   

   Nit: Why not simply use Action::create() here?

3. +++ b/core/modules/media/media.routing.yml
   @@ -25,6 +25,17 @@ entity.media.revision:
   +  options:
   +    parameters:
   +      media:
   +        type: entity:media
   

   Is this necessary? For some reason, I was under the impression that the routing system would reflect on the controller method and determine automatically that it expects a media entity. No big deal, just not sure if my info is correct.

4. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,45 @@
   + * @internal
   + *   Controllers are internal.
   

   🙏

5. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,45 @@
   +   * @param \Drupal\media\MediaInterface $media
   +   *   The media item it update.
   

   Should probably be "The media item for which to update metadata"?

6. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,45 @@
   +    $this->entityTypeManager()->getStorage('media')->save($media);
   

   Hmmm...isn't the whole idea of this issue to add an explicit updateMetadata() method? Wouldn't we call that instead, then save?

7. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,45 @@
   +    if (\Drupal::request()->query->has('destination')) {
   +      return $this->getRedirectDestination()->get();
   +    }
   

   Shouldn't we just plain use $this->getRedirectDestination(), rather than checking if a specific URL query parameter is set?

8. +++ b/core/modules/media/src/Entity/Media.php
   @@ -364,29 +364,60 @@ public function prepareSave() {
   +  protected function updateMappedMetadata(MediaInterface $translation, $override_existing = FALSE) {
   

   Not a big deal, but I'm not entirely clear on why we would pass $translation into this method? Couldn't we just call $translation->updateMappedMetadata() and have it operate on itself?

   Also, I know there's already been some bikeshedding on this, but I'd use "overwrite" instead of "override". Override implies that the previous value is remembered somewhere and can be reverted, but that's not case -- AFAICT, if $override_existing is TRUE, previous values are thrown away forever. That is an overwrite.

9. +++ b/core/modules/media/src/Entity/Media.php
   @@ -364,29 +364,60 @@ public function prepareSave() {
   +      if (!$translation->hasField($entity_field_name)) {
   +        continue;
   +      }
   

   Under what circumstances would the field not exist?

10. +++ b/core/modules/media/src/MediaInterface.php
    @@ -64,4 +64,12 @@ public function setCreatedTime($timestamp);
    +  /**
    +   * Update the media entity's field values from the source's metadata.
    +   *
    +   * @return \Drupal\media\MediaInterface
    +   *   The updated media item.
    +   */
    +  public function updateMetadata();
    

    This documentation is vague. We should document if, and how, existing values are overwritten here. We should also document how this interacts with translations, and we should say that this does NOT save the media item afterwards.

+1 to this. I need a public API to trigger metadata updates when the source field value has changed. Media module currently handles this automatically but the logic for determining if source value has changed is not quite sophisticated enough to detect certain situations. Specifically in my case, if the file entity for a file-based media entity is updated.

Having this committed would allow my module, Media Entity File Replace, to manually trigger a metadata update.

One use case I have on a site and haven't seen mentioned here is having the thumbnail be redefined on the video hosting platform (Vimeo in that specific case), and having to trigger a refresh on site while the URL of the media is constant but the actual thumbnail content changes (or I don't understand what's going on): this would mean not being able to rely on a thumbnail URL to compare whether the managed file association needs to be redone.

Re-rolling patch against 9.1.x-dev, kindly review a patch.

Novice to fix the trivial test fail

Fixing failed tests! Please review!

Looks good, thanks @ridhimaabrol24

Fixing Fail test, Please have a look.

This is a major feature request, but also an important one, so I'm bumping priority.

Here I have tried to fix failed tests and fixed some CS violations.

Fixing test.

Fixing test, Please review.

Usability review

We looked at this issue at the #3173646: Drupal Usability Meeting 2020-09-29.

Thanks for the screenshots in the issue summary. I guess the main question is whether we can come up with something better than "update metadata" to describe the action.

I am setting the status to NW for a reroll (even though the current patch is less than two weeks old) and an issue summary update.

• Add a release notes snippet to the issue summary.
• Where will this be documented? This is especially important because of comments like "it wouldn't hurt to let them figure out what that means" (from #36).
• The issue summary refers to #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction, which is closed as "won't fix". It is OK to leave that link for historical interest, but do not ask readers (like me) to go to that issue for context. Give examples of what "update metadata" means, say for a local PDF or a remote video. The examples should help decide whether "refresh" or "update" is clearer.
• Is there a confirmation page? If so, please include a screenshot under "user interface changes".

Not in scope for the usability review, but doesn't an API addition require a change record? I am adding the tag for that.

Great work - are there plans for a patch for 8.9.x?

@benjifisher going back to the wording. I see a few notes in #10 and #36 and while 36 raises some technical challenges, I think 10 still has some good thoughts. I disagree with the sentiment in 36 that it wouldn't hurt to let them figure out what [metadata] means. IMHO it's our responsibility to meet users where they are.

From 36 is sounds like getting the source (e.g. "YouTube") is hard, but getting the type (e.g. "Video") is possible. I polled/brainstormed with a bunch of end users and content-focused folk on one of my teams with the question:

If there was a button in Drupal that pulled information from a video provider (YouTube, Vimeo, or otherwise), what would you expect it to say?

"Refresh video information" was their consensus. Given the options above I think we could abstract this to "Refresh <type> information".

Given the base media types, this would produce:

Refresh video information
Refresh image information
Refresh local video information
Refresh document information

which all seem to sound reasonable (to me).

I tried this patch on my D8 project with media_entity_file_replace and found that the width and height of the images wasn't updated after using the new action on my media item.

Steps to reproduce:
1. Upload 400x400 image
2. See that the sizes in the media__field_media_image and media_field_data table are correct.
3. Replace the image with a 1024x1024 image
4. See that the sizes in those tables are still stored as 400x400
5. Run the action on the media item
6. See that the sizes are still 400x400

This causes issues with Focal Point, which uses those dimensions to figure out what the image should do. For now, I did the dumb thing and just reached into the database and updated the values manually when an image is replaced, but that's definitely not the "right" way to do it. I'm not sure if updated width and height attributes were expected as a result of this issue, but if they were, it's definitely not working.

Re-rolled for 8.9.x

There's no need to reroll this for 8.9.x -- because it's a new feature, it will not be committed to that branch anyway. :) This is for 9.2.x or later.

Reviewing #55

1. +++ b/core/modules/media/media.post_update.php
   @@ -16,3 +18,19 @@ function media_removed_post_updates() {
   +/**
   + * Install the 'Update metadata' action.
   + */
   +function media_post_update_install_update_metadata_action() {
   +  if (!Action::load('media_update_metadata')) {
   +    Action::create([
   +      'id' => 'media_update_metadata',
   +      'label' => 'Update metadata',
   +      'type' => 'media',
   +      'plugin' => 'media_update_metadata',
   +    ])
   +      ->save();
   +  }
   

   This needs a test.

2. +++ b/core/modules/media/src/Entity/Media.php
   @@ -364,29 +364,60 @@ public function prepareSave() {
   +      // Populate the field value in one of these scenarios: 1) The caller of
   +      // this function asked for it explicitly, 2) The entity field is empty,
   +      // or 3) The media source field has changed.
   

   Let's use the list format - https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

3. +++ b/core/modules/media/src/MediaInterface.php
   @@ -64,4 +64,18 @@ public function setCreatedTime($timestamp);
   +  /**
   +   * Update the media entity's field values from the source's metadata.
   +   *
   +   * This will fetch metadata from the source field, and update all values that
   +   * are mapped to entity fields. This will overwrite non-empty existing values,
   +   * and will update all translations of the entity. This will also try to
   +   * update the entity's thumbnail with a newer version, if available.
   +   *
   +   * @return \Drupal\media\MediaInterface
   +   *   An unsaved version of this media item, after updating the mapped field
   +   *   items and the thumbnail.
   +   */
   +  public function updateMetadata();
   

   It's great that this documents that the documentation says the entity thats returned needs saving.

   At some point it'd be lovely to know if the entity has changed and therefore if a save is actually necessary. But that's out of scope.

   One question I have is are we concerned about data loss through translations being unexpectedly overwritten?

4. +++ b/core/modules/media/tests/src/Functional/MediaUpdateMetadataControllerTest.php
   @@ -0,0 +1,68 @@
   +    // Initially the "name" metadata is the filename.
   +    $file = \Drupal\file\Entity\File::create([
   

   This fails a coding standards check. Need to add use Drupal\file\Entity\File as FileEntity and then $file = FileEntity::create([

5. One concern I have is whether we do this differently. At the moment we call $media->updateMetadata() and then $media->save(). Whilst we call $media->save() we will call $media->prepareSave() which does some of the same things as $media->updateMetadata(). I wonder if it'd be better if we did something like. $media->enforceMetadataUpdate()->save() where enforceMetadataUpdate() sets a protected property on the Media entity that is then checked in prepareSave().

+++ b/core/modules/media/tests/src/Functional/MediaUpdateMetadataControllerTest.php
@@ -0,0 +1,68 @@
+/**
+ * Tests the media "Update Metadata" controller.
+ *
+ * @group media
+ */
+class MediaUpdateMetadataControllerTest extends MediaFunctionalTestBase {

Given the amount of multilingual logic in the code I think we should be testing multilingual media items here.

This patch fixes #66.2 and 66.4.

Fixed test failures in #68, thanks!

Replying to #61 - @cweagans, if I understood you correctly issue of image dimensions not being updated once file is updated/changed is not related to this. Can you please create a new issue for it.

NW for #66.1, #66.3, #66.5 and #67.

This addresses #66.5 + some code style issues.

Still outstanding: #66.1, #66.3 and #67.

It feels like it should be two separate items/actions: "Update metadata" & "Update thumbnail".

Some very minor tweaks from looking at the #72 interdiff:

1. Instead of:

   protected function reloadPage() {
     if ($this->getRedirectDestination()->get()) {
       return $this->getRedirectDestination()->get();
     }

I suggest:

   protected function reloadPage() {
     if ($destination = $this->getRedirectDestination()->get()) {
       return $destination;
     }

2. And instead of:

   protected function shouldUpdateThumbnail($is_new = FALSE) {
     // Update thumbnail if we don't have a thumbnail yet or when the source
     // field value changed or when it was enforced.
     return !$this->get('thumbnail')->entity || $is_new || $this->hasSourceFieldChanged() || $this->forceMetadataUpdate;
   }

I suggest generally doing the trivial/cheapest tests first:

   protected function shouldUpdateThumbnail($is_new = FALSE) {
     // Update thumbnail if we don't have a thumbnail yet or when the source
     // field value changed or when it was enforced.
     return $is_new || $this->forceMetadataUpdate || !$this->get('thumbnail')->entity || $this->hasSourceFieldChanged();
   }

Hey, I have tried to cover the points mentioned in #73
Please review

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

Coming here from #3080666: oEmbed system doesn't work if thumbnail url does not have a file extension where Vimeo thumbnails is not correctly getting generated at all. Having an official way of re-generating them is pretty crucial to us.

1. +++ b/core/modules/media/media.module
   @@ -528,3 +528,20 @@ function media_views_query_substitutions(ViewExecutable $view) {
   +function media_entity_operation(EntityInterface $entity) {
   +  $operations = [];
   +  if ($entity->getEntityTypeId() === 'media' && $entity->access('update')) {
   +    $operations['update_metadata'] = [
   +      'title' => t('Update metadata'),
   

   this should go in the list builder, no need to implement a hook for an entity which the module defines already

2. +++ b/core/modules/media/src/Controller/UpdateMetadataController.php
   @@ -0,0 +1,46 @@
   +  public function updateMetadata(MediaInterface $media) {
   +    $media->updateMetadata();
   +    $this->entityTypeManager()->getStorage('media')->save($media);
   +    $this->messenger()->addStatus($this->t('Updated metadata on media item %label', [
   

   this is a CSRF vulnerability.

   Either we need a CSRF token on the route, or we need this to be a confirm form.

   I can do this anywhere on the internet

   <img src="https://yoursite.com/media/3/update-metadata" />

   And if I can trick you into visiting that page, it will force regeneration without your knowledge.

   This could lead to server load.

We can't put this in with a security issue in it

trying to cover #80.2 adding csrf token to the "metadata-update" route.

Fixing the tests and addressing 80.1

I do not understand why Drupal\Tests\media\Functional\MediaUpdateMetadataControllerTest is failing; when run locally, it passes.

This patch is intended to force the test runner to dump out what the page output looks like when it fails, since Drupal Core does not save its browser output on Jenkins.

In all other respects, this is the same as #88 -- just trying to understand what the test runner sees that we do not see locally since the XPath matches locally.

Trying again, this time to get passed the code sniffer to get the debug output I need.

Test runner... just run the damned tests! I don't care about style right now...

I've created a reroll of #82 that applies on 9.4.x.

Thanks, @jeroenT! Hadn't considered just commenting out the inspections for debugging... that makes a lot of sense.

Ok, test runner... let's see if base-64-encoding the HTML gives us the context we crave.

Interesting... on Drupal CI the paths for the site have a subdirectory prefix, which explains the test failures:

This does not happen locally. Mystery solved; this should be a quick fix for the tests. Working on that now.

Ok, added-in the site base path. This is the same as #89 with tests (hopefully) working now.

Also, I am attaching an inter-diff from 88 to this change since one wasn't previously attached. Cross-branch inter-diffs pose an interesting challenge; I am diffing the patches themselves since the changes are minimal. If anyone prefers a better way to get a cross-branch inter-diff, I am all ears!

Setting to NR.

I have used the patch in #95 with Drupal 9.3.5 and it's working well. Incidentally I am also using the patch from https://www.drupal.org/project/drupal/issues/3042423#comment-14357576 so a hook can pull hires thumbnails as well. The patch on this issue insures that if a video is edited so a higher resolution thumbnail is pulled the queue cam be run immediately and the new thumbnail is hooked up in the filesystem and used for the thumbnail display. Working like a charm.

If the video was deleted from YouTube, update metadata does not let you update the metadata, it just deletes the entity.

I wanna keep the video so it just show the usual YouTube box telling you the video was deleted.

Also if the video is not available in your region but is available in others it will remove the entity.

If anyone's looking for a fix to the issue reported by @cweagans in #61 it's here: #3088168: Media thumbnail dimensions are wrong for YouTube videos.

I used the patch #95. It worked once or twice, now the option does not appear in the bulk menu. I tried removing the patch(via composer), updating the db(nothing to change), reapplying the patch, updating the db (nothing to change).

Additionally, sometimes when I run it on the individual media item, I get something like looks like JSON in an expanding text area.

Edit: I got the bulk option back by running this command:

drush ev "include 'core/modules/media/media.post_update.php'; media_post_update_install_update_metadata_action();"

Not sure if this is isolated to me, but hope it helps someone else!

@GuyPaddock your patch no longer applies:

Patch: 2983456-95-9.4.x.patch no longer applies but after upgrading to Drupal 9.4.4 the following error occurs:

PHP Fatal error:  Class Drupal\media\Entity\Media contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\media\MediaInterface::enforceMetadataUpdate) in /app/web/core/modules/media/src/Entity/Media.php on line 86

Here's a reroll for 9.4.4.

... and a fix for the style linter.

Thanks @ahebrank applies smoothly again!

Latest patch works perfectly

Still the points from #98 need to be addressed to put this back to "Needs review".

What can we do to make progress with the usability review? (Needs usability review tag) Is it still needed for this bulk operation?

The best way to get a usability review is to join the weekly meeting and present the issue.See the #ux channel in Drupal Slack for the schedule.

If you cannot make it to the meeting, then either ping the same channel or add a comment to the issue for the weekly meeting, currently #3310096: Drupal Usability Meeting 2022-09-23 and ask for a review. Either way, make the issue easy to review: make sure the issue summary is up to date, so that people at the meeting do not have to read through the comments to understand the usability questions.

• The "Proposed resolution" section should describe all the changes made in the issue.
• The "User interface changes" should show the existing UI and the proposed UI.

Most of the time, I prefer to have plain text in the "Proposed resolution" section and screenshots in the "User interface changes" section.

Also, review #56 and make sure that all the points raised there have been addressed.

Patch applies but 2 extra folder structures are added as well with the files included in the patch. Is anyone else having this problem? Guessing this is not something that should be happening?

Eg. the following added file
core/modules/media/config/install/system.action.media_update_metadata.yml

Is also added to the following locations
core/b/core/modules/media/config/install/system.action.media_update_metadata.yml
core/core/modules/media/config/install/system.action.media_update_metadata.yml

Using patch from #104
https://www.drupal.org/files/issues/2022-07-30/drupal-add_update_metadat...

Using drupal/core (9.4.7)

I've been testing this patch on a 9.x live install and I had issues with the thumbnails update.

If a thumbnail already exists on the Media and we trigger the new update metadata function, the thumbnail is not updated. To make it work we have to:

1. Physically delete the file
2. Run the update metadata function
3. Run the cron (or the queue)
4. Clear image styles

From an usability point of view, I think that the user should only have to use the update metadata function then see the result immediately.

In the MR:

• Rerolled #104 on 10.1.x
• Changed condition in \Drupal\media\Entity\Media::getThumbnailUri() for force thumbnail update instead of using existing one

In the attached patch:

• 9.x version of the above

In the MR:

• Rebased the branch on 10.1.x
• Added the \Drupal\media\MediaInterface::isMetadataUpdateEnforced() method
• Updated \Drupal\media\Plugin\media\Source\OEmbed::getLocalThumbnailUri() to be able to force the thumbnail download
• Updated \Drupal\media\Entity\Media::updateThumbnail() to flush thumbnail's images styles after update

In the attached patch:

• 9.x version of the above

#115 is missing the following use statement:
use Drupal\system\Entity\Action;
which throws an error:
Error: Class 'Action' not found in media_post_update_install_update_metadata_action() (line 42 of /app/web/core/modules/media/media.post_update.php) #0 /app/vendor/drush/drush/src/Commands/core/UpdateDBCommands.php(321): media_post_update_install_update_metadata_action(Array)

@No Sssweat The patch is only here for devs convenience but the real review has to be made on the !3037 Merge Request.
I checked in the MR and the appropriate use properly is included so the change should be effective on D10.

As told in #116, the attached patch was faulty. Here is a new one for Drupal 9.5 users.

My apologies @DuaelFr, I was not aware.

Thank you for enlightening me on how the drupal.org review process flows with GitLab these days.

Also, while I am at it, thank you for your contributions!

@DuaelFr Thanks for the 9.5.x port of the patch in #118. Successfully using this with a custom MediaSource similar to OEmbed. There is one major issues I experience:

In function `GetLocalThumbnailUri` -`$files = $this->fileSystem->scanDirectory($directory, "/^$hash\..*/");`. scanDirectory will get unusably slow for very large number of files in a directory. The default media table use calls the GetLocalThumbnail function for every media. In my case there are 25'000 media and the page is taking a very long time to load. It would be better to use `file_exists` (maybe with fallback for image extensions). What do you think?

Otherwise working really nice.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. 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.

Patch for 9.5.3 users.

Patch from #122 doesn't apply for me using composer. I could get it to partly apply using the patch utility but then I had fatal errors.
Here is a new reroll for Drupal 9.5.3

Is there an update hook to install this file? core/modules/media/config/install/system.action.media_update_metadata.yml

For now, I just went through the patch in #123 to identify the reason why the patch doesn't apply.
I found some mix-up in there, namely with Media.php and the media.post_update.php parts. I have taken the gitlab pull request as reference and re-rolled the patch over 9.5.9 version of Drupal core.

Fixed the spelling error in the #126 patch.

Fixed assertion for "Update metadata" operation in test and reroll it for D10.0.x.

Uploading the patch for D9.5.x as well, since #128 fails to apply to that version.

#3271688: Remove source from media mappings may be related as I found when using metadata and trading out an image on a media deletes the files.

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Re-roll of #129 against Drupal 10.1.x.

Re-roll of #129 against Drupal 10.1.x.

(#133 accidentally included an empty file at a deleted file path.)

#120 is maybe a going concern?

(If that's pre-existing behaviour though, it could be a new issue.)

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

I came across this issue and applied the patch in #134. In general the functionality looks real good and useful. While testing and playing around I've noticed a few details:

1. The status message when running Update metadata seems solely inform the user that the process started and ran, but not about its potential outcomes:

a) You are able to update a media item several times in a row but each time you get the same status message returned: Updated metadata on media item x. Just based on that status message, I would assume that the media item is getting updated with new changes on every run. But if you take a look at the Updated-column on /admin/content/media the timestamp for the media item remains the same and shows an update event in the past. A discrepancy and disconnect between the status message and the Updated-column.

b) After that I was curious and got inspired by #98 what would be the status message if you update the metadata for a remote video media type's media item where the URL is not reachable because the server is unavailable/down. Not sure if it is a legitimate way to simulate that scenario, but I've simply switched off the WLAN connection while the site was still running locally in DDEV.
In a Safari window I got You are not connected to the internet for the URL of the remote video's media item. While when I ran Update metadata I've received the same status message "Updated metadata on media item x" and the media item's thumbnail got removed alongside.

c) Not sure how likely it is that a local source file is getting deleted and the Update metadata process is run, but I've manually removed the original image of an image media type's media item. Again I've received the status message Updated metadata on media item x while the thumbnail got removed as in b).

I wonder if instead of informing the user that the process updating the metadata got started, would it be more helpful to provide information about the outcome of that process? That way the user would feel more in control. How about the following rough draft, not perfect at all, just wanted to communicate the gist about each scenario:

• If everything runs through fine with changes in place on the media item's source the current status message could be used.
• For scenario a) how about something like "Nothing to update. Media item x remains unchanged."
• For scenario b) something like "The remote source for media item x is currently unavailable. Refreshing the metadata is not possible, try again later."
• For scenario c) something like "The source file for media item x is missing, unable to refresh the metadata."

2. About the main question if there is a better alternative for describing the action compared to Update metadata? I am uncertain if there is a one or two worded alternative that explains the process clear and unambiguous as a whole. But from my perspective it should be avoided to use loaded terms that might create assumptions, associations, and expectations on the user's end. I think more neutral terms should be used instead preferably. From my perspective Update as well as Metadata are such loaded terms.

Update sounds familiar in the context of updating for example Core or contrib modules but in the context of entities it feels sort of unusual - in particular for locally stored media items in contrast to the remote video ones. In addition to that the term sort of overlaps with Edit one of the other options in the operations drop button. Personally my initial association was either to add something new to a media item and or it might entail additional pro-active manual steps.
Metadata, on the other hand, generates certain expectations. For audio you have for example mp3tags for mp3s, for images and locally hosted videos there is EXIF data and documents like PDFs have extensive metadata too. But if you for example try to create a view based on any of the media items it turns out the metadata is mainly about the thumbnail (in Core out of the box only for remote video and image media types) and the metadata attributes (name, filesize, filemime, width, height, etc) as mentioned in the issue summary. That is totally fine but the term metadata is potentially creating certain assumptions and expectations on the user's end and "might" lead into confusion therefore.

The proposed alternative Refresh (in #10 and #60) for the term Update might be the more clear and neutral choice. From my perspective the term Refresh implies a more unsupervised and automated process.

But in both comments I disagree with the proposed alternative for the term metadata. #10 suggests Refresh from Youtube but as @marcoscano points out in #36 not every media item is from Youtube therefore that suggestion is not applicable. On the other hand going with the labels suggested in #60 I consider those too wordy and it also raises the question what kind information is updated?

Refresh video information
Refresh image information
Refresh local video information
Refresh document information

But I think @devianintegral indirectly provided another viable alternative in #10 from my point of view. Why not use the term Source instead of Metadata? The term Source is more neutral but at the same time would imply that you synchronize your media items based on the informations available in the media item's source file. So how about the following changes?

The drop button in the operations column
All options in the select field are single worded, I wonder if it would make sense to stay consistent here?

Edit 
Delete
Update metadata -> Refresh

Bulk action options
For bulk actions I would provide some more context like the other options do and add "from source":

Delete media
Publish media 
Save media
Unpublish media
Update metadata -> Refresh from source

3. As recommended by @benjifisher in #56 https://www.drupal.org/docs/8/core/modules/media/creating-and-configurin... was already added as noted in #57.
But I wonder if it would make sense to create a help topic for the Update metadata task alongside. That way the user wouldn't have to leave the admin ui, plus it would be an easy and convenient way to explain the underlaying functionality as well what it entails aka what data is being updated/refreshed for the different media types.

Those were just my personal observations. I think it would be still good to discuss the issue, in particular point 2 about the microcopy, in a larger group in the weekly usability meeting (as suggested by @benjifisher in #108).

We've ran into this bug from a slightly different angle with Acquia DAM. We have implemented our own Metadata Sync controller which will attempt to resave the media entity. However the following code in the media entity ensures that if you change the metadata on the external source, it'll never update Drupal because the actual source asset id hasn't changed.

   // Only save value in the entity if the field is empty or if the
   // source field changed.
   if ($translation->hasField($entity_field_name) && ($translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged())) {
      $translation->set($entity_field_name, $media_source->getMetadata($translation, $metadata_attribute_name));
          }

In the meantime we've copied code from core and manually fixed it in our module. Once this issue is fixed, it'll make that code and our metadata sync controller redundant. So +1 to prioritizing a fix here!

Applied #134 succesfully
I can "update" single items but the "update metadata" is not avaliable in the batch selection

Edit: My bad, did not see the db updates. Ran drush updb. Everything works fine now #134.

The patch in #134 failed for Drupal 10.2 on the media.routing.yml file, so I've adapted the patch slightly.

We had a problema applying #141 following the change of getMetada() into getRawMedata in #3042423: Add new hook to Media module to modify OEmbed metadata #59

So here is a patch to apply those changes.

Patch from #142 was not applying because it has been created inside the core folder instead of at the root of the project.
This patch solves that issue and applies on 11.x (and 10.2)

I rerolled MR !3037 on 11.x based on the code from #134 and #141 (next patches were missing some code).

The attached patch applies on Drupal 10.2

Thank you for this feature. Using it on D10.2.2.

For us, one issue remains: browser cache. E.g. updating YouTube video media, new thumbnails will be created but with the same hash (file name). If the browser cache is enabled, the new thumbnail will not be refreshed in the browser.

I'm attaching a patch to fix this issue, using timestamp as a suffix for thumbnail files, and care about removing the old ones on the force update. Moved image_path_flush().

The patch was created from MR3037 diff.

FYI: as @klausi noted, the scanDirectory() can become unusably slow if the directory contains many files.

@szato, we faced the same issue, however we felt changing the logic around the thumbnail name generation might be an overkill and we refrained from solving front-end cache issue by changing the back-end logic.

We have varnish configured on our sites, which further complicates the issue.
So we applied the changedTime of the thumbnail entity to the URI of the image style served, as a get parameter, in an md5 encoded format.
Browser cache relies on the URI too, like varnish does, so cache-busting by get parameter is an acceptable solution which is well separated from the back-end logic of thumbnail generation.
However, we found that the changedTime on the thumbnail entity is not update when we trigger this action, so that might be something to address here!

@joevagyok, if you use hook_preprocess_image_style(), then you can use filemtime($variables['uri']) (the original image file modification time) for changedTime. I think it's better because the entity update time changes every time it's saved again, not just when the file is changed.

Rebased against 11.x -- hopefully the test bot doesn't freak out 😅.

Additionally, the concern brought up in #98 is still valid. Upon manual testing, the following procedure results in data loss:

1. Create a media entity that implements oembed
2. Disconnect from the public internet
3. Update the metadata
4. Notice that the original thumbnail is gone and is not replaced with a new one

+1

Related to @japerry's comment in #139, I would note that this approach only handles base field values (string, integer, boolean), and there are cases where it would be important to have source data mapped to, say, a taxonomy term reference field.

The current code can't do that, but I would propose a follow-up issue to make it possible using an event handler.

I can work on that over in #3443757: Mapping metadata to reference field as a proof-of-concept. Posting here to see if there is an existing issue or indeed, a desire for that kind of feature.

Note: the latest MR 3037 does not have the browser cache busting that was added in #145.

Rebased on 11.x, I added most of #145 except for removing image_path_flush() in Media, that will affect every type not just OEmbed.

Edit: actually I removed #145, it looks like there was some disagreement.

@KarlShea
Regarding your comment "I added most of #145 except for removing image_path_flush() in Media, that will affect every type not just OEmbed."
- That seems like an edge case. You would need to have another media type that is using the thumbnail file of an oEmbed resource. The normal usage would be a 1:1 of media to thumbnail file.

Regarding your comment "Edit: actually I removed #145, it looks like there was some disagreement."
There still needs to be a solution for busting cache - browser, edge cache, reverse proxy.

Options so far for cache busting:
1. The thumbnail generation approach as used in patch #145
Pros:
- Busts through all layers of caching - browser, edge cache reverse proxy.
- Automatically deletes the old file and any image styles for the old file.
- ???
Cons:
- A call to "getMetadata" for the "thumbnail_uri" assumes that the thumbnail is being set on the media entity. If it does not update the media, then this would cause deletion of the old file.
- ???

2. Add a modified timestamp URL query parameter as described in #146 and #147
Pros:
- Does not alter the thumbnail generation.
- ???
Cons:
- URL query parameters are not always respected by all layers of caching. For example, edge caching in Akamai could ignore the query parameter for some assets such images depending on the configuration in Akamai.
- Depending on the approach to get the modified time, you could be incurring a file system operation ("filemtime") on every image_style theme rendering.
- ???

Rerolled !3037 MR against 11.x
Patch attached for people needing this on 10.3.1

Hey @DuaelFr applying that patch in 10.3.1 causes update.php to fail with [error] The installed version of the /Media/ module is too old to update. Update to a
version prior to 11.0.0 first (missing updates:
media_post_update_remove_mappings_targeting_source_field).
.
Specifically: removing the following lines allows the database update to pass:

+    'media_post_update_oembed_loading_attribute' => '11.0.0',
+    'media_post_update_set_blank_iframe_domain_to_null' => '11.0.0',
+    'media_post_update_remove_mappings_targeting_source_field' => '11.0.0',

@queenvictoria Thank you for letting me know! You're right, I did a mistake rerolling this and making the patch. Here is a new one.

Thank you, @DuaelFr for patch #157 for D10.3.

It applied cleanly and works well, but it introduces the following error when drush updb is run:

"The following update is specified as removed in hook_removed_post_updates() but still exists in the code base: media_post_update_oembed_loading_attribute"

I was getting the same error as banoodle but patch 159 seems to have resolved it.

Oh, @jphelan, you are correct! I don't know why, but I didn't realize 159 is for 10.3 also. Thanks for pointing this out.

Patch for #159 works perfectly for me.

Thanks to everyone for getting this working and to RTBC!

I started triage and immediately saw that the remaining tasks state that a usability review is needed. So, I went through the comments and I do not see a review. Setting back to needs work. I read through nearly all of the comments and noticed some that have not yet been addressed. I have listed those in the 'remaining tasks'.

So, this just needs to work through those items to finish this off.