Hi,

We currently allow file fields to configure their upload paths to prevent the situation of thousands of files in the same folder. The image styls for these files mimic that behavior.

However, with the media library, respectively the Remote Videos, all thumbnails for videos get generated in the same folder.
This can lead to performance scalability problems.

Is there a chance to introduce pattern configurations for these as we have for all other files? Or at least introduce a sane default?

Issue fork drupal-3026184

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kyuubi created an issue. See original summary.

kyuubi’s picture

Version: 8.6.x-dev » 8.6.5
larowlan’s picture

Version: 8.6.5 » 8.7.x-dev
Category: Feature request » Bug report
Priority: Major » Normal

\Drupal\media\Plugin\media\Source\OEmbed::getLocalThumbnailUri is where the code is here.

I'd argue this is a bug because https://twitter.com/dries/status/397541550223155200

kyuubi’s picture

Hi,
Submitting a patch for this that changes the OEmbed plugin to support tokens.

larowlan’s picture

Issue tags: +Needs tests
+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -370,7 +382,8 @@ protected function getLocalThumbnailUri(Resource $resource) {
+    $directory = $this->tokenService->replace($configuration['thumbnails_directory'] ,array('media' => $media));

this will need to use short array syntax, but other than that looks pretty good.

I guess we should update the config form description to include 'You may use tokens' or similar

kyuubi’s picture

Thanks @larowlan,
Updated patch with short hand syntax and field description.

dww’s picture

Status: Active » Needs review

Out the door right now, so I can't do one myself, but this now needs review.

Thanks,
-Derek

The last submitted patch, 4: media-support_tokens_in_oembed_thumbnail-3026184.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kyuubi’s picture

From what I can see this is present in the new Drupal version, but that's not reflected in this issue?
Can someone confirm?

kyuubi’s picture

Ah nevermind. Cache.
It will need a re-roll this patch.

kyuubi’s picture

Here is an updated patch against 8.7.x.

yogeshmpawar’s picture

Status: Needs work » Needs review

Setting back to Needs Review & triggering bots.

Status: Needs review » Needs work

The last submitted patch, 13: media-support_tokens_in_oembed_thumbnail-3026184-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

carolpettirossi’s picture

Just adding here an updated patch for 9.1.x.

carolpettirossi’s picture

Sorry, previous patch was generated incorrectly. Here is a new one.

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
4.87 KB
3.31 KB

Fixing Test.

chr.fritsch’s picture

Status: Needs review » Needs work

In general, it looks good to me. We also still need a test for this.

+++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
@@ -164,6 +174,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $this->urlResolver = $url_resolver;

@@ -396,7 +410,8 @@ protected function getLocalThumbnailUri(Resource $resource) {
+    $directory = $this->token->replace($configuration['thumbnails_directory'] ,array('media' => $media));

We should use arrays short syntax here.

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
2.17 KB

@chr.fritsch I have updated the array short syntax mentioned in #21, Also try to write the test case, Please have a look and advise.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nmeegama’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Needs review » Needs work

Patch failed for Drupal 9.2.4

nmeegama’s picture

nmeegama’s picture

Status: Needs work » Needs review
nmeegama’s picture

Patch with code-checks

Status: Needs review » Needs work

The last submitted patch, 28: 3026184-24.patch, failed testing. View results

vsujeetkumar’s picture

@nmeegama there is any specific reason to change the version from 9.3.x to 9.2.x?, I think we should work on 9.3.x directly. Also there is no need to add "DIRECTORY_SEPARATOR" in OEmbedSourceTest file, because of that tests fails, Please have a look.

nmeegama’s picture

Specifically because there is security update for 9.2.x, But yes makes sense to do 9.3.x as well

vsujeetkumar’s picture

Status: Needs work » Needs review

Add test on #31 for 9.3.x, Moved to NR.

vsujeetkumar’s picture

Version: 9.2.x-dev » 9.3.x-dev

Change version back to 9.3.x.

danflanagan8’s picture

Status: Needs review » Needs work

I don't think the added testing in #31 actually tests anything.

+++ b/core/modules/media/tests/src/Kernel/OEmbedSourceTest.php
@@ -164,9 +179,14 @@ public function testThumbnailUri(string $remote_thumbnail_url, $thumbnail_header
+    $directory = $this->token->replace('public://oembed_thumbnails', ['media' => $media]);

^^ There aren't any tokens to replace here, so this doesn't do anything.

+++ b/core/modules/media/tests/src/Kernel/OEmbedSourceTest.php
@@ -164,9 +179,14 @@ public function testThumbnailUri(string $remote_thumbnail_url, $thumbnail_header
-
+    $hash = Crypt::hashBase64($thumbnail_url);
     // The thumbnail should have a file extension, even if it wasn't in the URL.
-    $expected_uri = 'public://oembed_thumbnails/' . Crypt::hashBase64($thumbnail_url) . ".$expected_extension";
+    $expected_uri = 'public://oembed_thumbnails/' . $hash . '.' . $expected_extension;

^^ This is just splitting one line into two lines.

It was a good effort to get started. What we need is a test with an Oembed source plugin where $configuration['thumbnails_directory'] includes tokens. Then we can test if they get replaced correctly.

phenaproxima’s picture

Issue tags: -D8Media

Removing the defunct D8Media tag.

phenaproxima’s picture

Category: Bug report » Feature request

I'm not against this idea overall, but I'd prefer if we didn't pass the $media context into the token system. That adds what feels like unnecessary complexity to me, and it means we need to add more deprecations (since the signature of a protected method would change). If we just use the default tokens, we should at least get things like the current year and month, which will allow the thumbnail directory to be split up nicely and thus get past the scalability/performance issues that this issue was opened for.

phenaproxima’s picture

I moved the patch to a merge request and added test coverage. Re-titling the issue for clarity too, and hiding patches in favor of the merge request.

I also dialed back the scope a bit, as described in my last comment. What we have now at least allows segmenting thumbnails by date, and is configurable per media type; if we want to pass the media entity in to the token system too, we can do that in a separate issue if needed. :)

This still needs a constructor deprecation for the new argument, but first let's see if this breaks any tests.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good to me. I'm a bit on the fence about updating the thumbnails_directory for existing sites that still use the default. Since it is relatively easy for users to change the config I'm leaning towards not updating this automatically. In that case this is probably RTBC assuming the tests will pass.

phenaproxima’s picture

I'm a bit on the fence about updating the thumbnails_directory for existing sites that still use the default. Since it is relatively easy for users to change the config I'm leaning towards not updating this automatically.

That's the conclusion I came to as well. This doesn't seem worth an update path, IMHO.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Nah, apparently I'm wrong. Restoring RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
git ac https://git.drupalcode.org/project/drupal/-/merge_requests/1275.diff
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7174    0  7174    0     0   1763      0 --:--:--  0:00:04 --:--:--  1763
error: patch failed: core/modules/media/tests/src/Kernel/OEmbedSourceTest.php:165
error: core/modules/media/tests/src/Kernel/OEmbedSourceTest.php: patch does not apply

@phenaproxima nope you were right. You need to merge 9.3.x into your branch.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Done. Restoring RTBC on the assumption that tests will pass.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
phenaproxima’s picture

Status: Needs work » Needs review

Good catch. I've added the call to renderFromHtml() and relevant test coverage.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Tests are green and feedback seems addressed, back to RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed adf864e and pushed to 9.3.x. Thanks!

  • alexpott committed adf864e on 9.3.x
    Issue #3026184 by phenaproxima, nmeegama, kyuubi, vsujeetkumar,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

Published the CR.

amateescu’s picture

Note that this change introduce a caching regression: any page displaying a thumbnail for an oEmbed source became uncacheable.

Opened #3432599: The default configuration of the oEmbed source plugin makes pages uncacheable to fix that problem.