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?
Comment | File | Size | Author |
---|
Issue fork drupal-3026184
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:
Comments
Comment #2
kyuubi CreditAttribution: kyuubi as a volunteer commentedComment #3
larowlan\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
Comment #4
kyuubi CreditAttribution: kyuubi as a volunteer commentedHi,
Submitting a patch for this that changes the OEmbed plugin to support tokens.
Comment #5
larowlanthis 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
Comment #6
kyuubi CreditAttribution: kyuubi as a volunteer commentedThanks @larowlan,
Updated patch with short hand syntax and field description.
Comment #7
dwwOut the door right now, so I can't do one myself, but this now needs review.
Thanks,
-Derek
Comment #11
kyuubi CreditAttribution: kyuubi commentedFrom what I can see this is present in the new Drupal version, but that's not reflected in this issue?
Can someone confirm?
Comment #12
kyuubi CreditAttribution: kyuubi commentedAh nevermind. Cache.
It will need a re-roll this patch.
Comment #13
kyuubi CreditAttribution: kyuubi commentedHere is an updated patch against 8.7.x.
Comment #14
yogeshmpawarSetting back to Needs Review & triggering bots.
Comment #18
carolpettirossi CreditAttribution: carolpettirossi at Prosple commentedJust adding here an updated patch for 9.1.x.
Comment #19
carolpettirossi CreditAttribution: carolpettirossi at Prosple commentedSorry, previous patch was generated incorrectly. Here is a new one.
Comment #20
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixing Test.
Comment #21
chr.fritschIn general, it looks good to me. We also still need a test for this.
We should use arrays short syntax here.
Comment #22
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented@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.
Comment #25
nmeegama CreditAttribution: nmeegama commentedPatch failed for Drupal 9.2.4
Comment #26
nmeegama CreditAttribution: nmeegama commentedNew patch for Drupal 9.2.x
Comment #27
nmeegama CreditAttribution: nmeegama commentedComment #28
nmeegama CreditAttribution: nmeegama commentedPatch with code-checks
Comment #30
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented@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.
Comment #31
nmeegama CreditAttribution: nmeegama commentedSpecifically because there is security update for 9.2.x, But yes makes sense to do 9.3.x as well
Comment #32
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedAdd test on #31 for 9.3.x, Moved to NR.
Comment #33
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedChange version back to 9.3.x.
Comment #34
danflanagan8I don't think the added testing in #31 actually tests anything.
^^ There aren't any tokens to replace here, so this doesn't do anything.
^^ 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.Comment #35
phenaproximaRemoving the defunct D8Media tag.
Comment #36
phenaproximaI'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.
Comment #38
phenaproximaI 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.
Comment #39
seanBChanges 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.
Comment #40
phenaproximaThat's the conclusion I came to as well. This doesn't seem worth an update path, IMHO.
Comment #41
phenaproximaI think there are merge conflicts in light of #3231731: Use Content-Type header from oEmbed thumbnail GET request when determining the file extension.
Comment #42
phenaproximaNah, apparently I'm wrong. Restoring RTBC!
Comment #43
alexpott@phenaproxima nope you were right. You need to merge 9.3.x into your branch.
Comment #44
phenaproximaDone. Restoring RTBC on the assumption that tests will pass.
Comment #45
alexpottComment #46
phenaproximaGood catch. I've added the call to
renderFromHtml()
and relevant test coverage.Comment #47
seanBTests are green and feedback seems addressed, back to RTBC!
Comment #48
alexpottCommitted adf864e and pushed to 9.3.x. Thanks!
Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedPublished the CR.
Comment #52
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedNote 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.