- 🇩🇪Germany sleitner
@pbouchereau the patch needs to be based in the docroot, not within core
@sleitner
And without a missing file too... learning the hard way.
Thanks.The last submitted patch, 48: 2954834-48.patch, failed testing. View results →
- 🇩🇪Germany sleitner
phpunit has been updated in DrupalCI to 9.6.1 and throws new deprecation warnings. Not related with your patch as far as I can see.
- Status changed to RTBC
almost 2 years ago 5:31pm 7 February 2023 The last submitted patch, 48: 2954834-48.patch, failed testing. View results →
- 🇩🇪Germany sleitner
Branch testing only failed temporarily again.
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
updated the issue summary.
Wondering if this needs a review / sign off from a 'subsystem' media maintainer. Already seeing some of them commenting.
Added some tags so hopefully it will reach more people, searching the issue queue :)
----
btw really liking this new feature
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
making clear thee current patch is about mapping the fields at a media entity.
The last submitted patch, 48: 2954834-48.patch, failed testing. View results →
- 🇩🇪Germany sleitner
Branch testing only failed temporarily again.
The last submitted patch, 48: 2954834-48.patch, failed testing. View results →
- last update
over 1 year ago 30,321 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - last update
over 1 year ago 30,323 pass - Status changed to Needs work
over 1 year ago 8:53am 2 May 2023 - 🇬🇧United Kingdom catch
+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php @@ -66,6 +92,22 @@ class FileVideoFormatter extends FileMediaFormatterBase { + '#type' => 'select', + '#title' => $this->t('Poster field'), + '#description' => empty($image_fields) ? $this->t('Please add an image field to this media entity to use the poster feature.') : NULL, + '#default_value' => $this->getSetting('poster'), + '#options' => $image_fields,
The text here shouldn't use 'Please', I think we have an issue around to remove please from the code base entirely. Also not sure 'poster feature' helps much.
Maybe:
'An image field to use as the source of the poster attribute'?
--
Also what happens if the poster image uploaded is wildly different dimensions to the video - does this need to support image styles? Will add some complexity to the patch to do so, but was also mentioned in #30.
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
We have tested this patch for over a month now and I agree with catch. without an image style it is not usable for content-editors.
For site builders the option list is getting really long now and with adding image style options inside this screen I think we need some extra structure like fieldsets/details to group some settings. (also used an other patch to add some other video settings)
- 🇩🇪Germany Anybody Porta Westfalica
As we're now running into this requirement, I'll create a MR from the latest patch and will try to work on this. Really helpful addition.
- last update
over 1 year ago Custom Commands Failed - @anybody opened merge request.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - 🇩🇪Germany Anybody Porta Westfalica
Yay, it basically works!! :)
Give it a try on the tugboat preview.Now I'm wondering, if the implementation currently in
viewElements()
shouln't at least partially be moved intoprepareAttributes()
or what's the reason for the decision to put it intoviewElements()
.
Also I'm not totally sure about the Cache tags implementation.Would be nice to have another review, I think we're making progress here and I'm happy to learn details. Also we should have a look at the existing tests again, I didn't touch them yet.
- last update
over 1 year ago 29,382 pass, 8 fail - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - Status changed to Needs review
over 1 year ago 12:22pm 22 May 2023 - 🇩🇪Germany Anybody Porta Westfalica
@Grevil please note the 9 upstream test fails in core, that will also fail here, until it's resolved in core.
- last update
over 1 year ago Custom Commands Failed - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 29,388 pass, 2 fail - Status changed to Needs work
over 1 year ago 1:45pm 22 May 2023 - 🇩🇪Germany Grevil
Forgot to mention it. I debugged the cache tags and they also LGTM!
- Status changed to Needs review
over 1 year ago 3:00pm 22 May 2023 - 🇩🇪Germany Anybody Porta Westfalica
Thanks @Grevil, I fixed the image style test now, please review finally.
- Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - 🇩🇪Germany Anybody Porta Westfalica
Switching back to 10.1.x so we hopefully get this in before stable as minor fix.
- 🇬🇧United Kingdom catch
See https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... → (linked in the comment above) 11.x is the target branch for everything now and we backport from there.
- last update
over 1 year ago 29,399 pass - last update
over 1 year ago 29,399 pass - 🇩🇪Germany Anybody Porta Westfalica
Thanks @catch!
GitLab rebase didn't work so let's create a fresh new MR against 11.x instead and leave the 10.1.x one if someone uses / needs it in the meantime? @Grevil could you do that?
So the old MR might still be reviewed in the meantime to speed things up.
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
Testing the new image style feature.
A user has now 3 options within the poster field:
- None
- Thumbnail
- Image
As showed below the thumbnail option and the image option are equal. With both a user can select an image style. So I was wondering what is the difference?
- 🇩🇪Germany Anybody Porta Westfalica
Thank you for the feedback @Martijn de Wit. This shows possible confusion for end-users.
As showed below the thumbnail option and the image option are equal.
It's not the same. While in the "Poster field" you pick the image field to display as poster.
In the "Poster field image style" field, you select the image style (Imagecache was the name in Drupal 7) to apply on the image.You say the options are equal - but that's not the case and it doesn't look like this is true on your screenshots?
Perhaps the UX team should have a short look to improve the descriptions or field labels?
- Merge request !4046Issue #2954834: Add poster image and transcript to HTML5 media videos → (Open) created by Grevil
34:49 34:15 Running- 🇩🇪Germany Grevil
Added a new branch "2954834-add-poster-image-transcript-from-fields-11.x" and applied the current "MR !4010 " diff.
@Anybody, as you created the MR, you should (hopefully) be able to close it again. I get a 404 error, when trying to close the old MR.
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
A yeah, now I get it :)
I have 2 image fields on my video entity. one named 'image' and one named 'thumbnail' from previous tests.
Sorry for the confusing post.Then it works as advocated, so it was a good test :)
- Status changed to Needs work
over 1 year ago 1:51pm 1 June 2023 - 🇺🇸United States smustgrave
Took a look and believe we will need an upgrade path for the new formatter settings.
- 🇩🇪Germany Anybody Porta Westfalica
Thanks @smustgrave! That would mean, we need an update path to set all values to empty on existing formatters:
'poster' => '', 'poster_image_style' => '', 'transcript' => '',
Is that really needed? I guess if yes, it's because the keys have to be added?
Do you know any similar example we could use for the update hook?Would it perhaps make sense to add a helper for such cases to make field (formatter / storage) settings updates more simple (DX)? ✨ Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed) Needs work
At least to me such things always feel quite hard to do it right, having to copy existing examples etc... we shouldn't open that side topic here, I guess, but perhaps this is a good starting point to proceed the discussion in the linked issue or a new meta issue? - 🇺🇸United States smustgrave
May be a good example https://www.drupal.org/project/drupal/issues/3309016 📌 Add image preload option to help boost actual and perceived performance Needs work
- 🇬🇧United Kingdom catch
Is that really needed?
Yes we need to update existing configuration so that if you update and do a config export, the exported configuration matches how things would look if you'd resaved. If we don't do this, then when you save formatter settings later manually, these keys would be added which gets very inconsistent/confusing.
The pattern is to put the logic for adding the new keys into a presave hook, and then using the actual post update function to just load and re-save configuration. Views (both in 10.1 and 9.5) has the most examples of this, but also a grep for ConfigEntityUpdater will find more.
- last update
over 1 year ago 29,421 pass - last update
over 1 year ago 29,421 pass - last update
over 1 year ago Custom Commands Failed - Status changed to Needs review
over 1 year ago 2:40pm 6 June 2023 - 🇩🇪Germany Grevil
I added an update hook, but I couldn't figure out, how to specifically target field instances using the "file_video" formatter. Please review and adjust accordingly!
- Status changed to Needs work
over 1 year ago 4:45pm 6 June 2023 - 🇺🇸United States smustgrave
May be worth asking #contribute channel.
But the version number wouldd be 101021
- 🇬🇧United Kingdom catch
The update should use hook_post_update_NAME() rather than hook_update_N(), and the logic of actually updating the configuration should happen in hook_entity_presave(), not in the update function itself, so that it also runs when importing default config and similar.
- 🇩🇪Germany Grevil
Thank you, @catch!
I got the point on using the hook_post_update_NAME() hook rather than the hook_update_N() hook, but didn't understand the hook_entity_presave() part, as we are basically only updating a few "entity_form_display" setting values and do not change the config itself. So why exactly the "hook_entity_presave" hook? Am I missing something? Is there an example with a similar update in core?
- last update
over 1 year ago 29,446 pass - 🇬🇧United Kingdom catch
For an example of another update, see:
/** * Implements hook_ENTITY_TYPE_presave() for entity_view_display. */ function media_entity_view_display_presave(EntityViewDisplayInterface $view_display): void { $config_updater = \Drupal::classResolver(MediaConfigUpdater::class); assert($config_updater instanceof MediaConfigUpdater); $config_updater->processOembedEagerLoadField($view_display); }
Which is related to:
media_post_update_oembed_loading_attribute()
Grepping for ConfigEntityUpdater will find more post updates which will usually be tied to a presave hook in the respective module.
Looking at this again, I think we might need to do more with the transcript support though:
HTML allows multiple
<track>
elements, to support subtitles in different languages as well as captions, see the examples on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/trackSo... should we think about supporting a multiple value field for 'track'? But then this would ideally support the 'kind' attribute as well as language etc., it is starting to look like another media type with its own fields at that point though and then a multi-value media field on the video entity.
- 🇬🇧United Kingdom catch
Tagging for needs subsystem maintainer review to hopefully get a second opinion on that last bit.
- 🇩🇪Germany Grevil
Thanks for the clear-up @catch! I'll have a look again, once everything is discussed with the relevant sub system maintainer! :)
- 🇬🇧United Kingdom catch
I think this might be worth splitting into two issues - one for poster image for which I assume there should just be one.
And one for transcript/
<track>
where there are more possibilities - if we do split, that should probably be the new issue since there'll be more to figure out on there. Might be worth waiting to see what media maintainers say though. - 🇳🇱Netherlands seanB Netherlands
HTML allows multiple
elements, to support subtitles in different languages as well as captions, see the examples on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/trackSo... should we think about supporting a multiple value field for 'track'? But then this would ideally support the 'kind' attribute as well as language etc., it is starting to look like another media type with its own fields at that point though and then a multi-value media field on the video entity.
Since media entities are translatable, I don't think we need to support different tracks for different languages. You probably only want the track for the current language (like how you would handle translations for other entities).
The
kind
attribute seems interesting, although I'm not completely sure if we can properly explain the different use-cases to site editors. I think accessibility experts might have more insights on when / how to use thekind
attribute properly. It would already help a lot to configure thekind
attribute on the formatter. Site builders can add a field description on the field where editors can upload the.vtt
files, to make sure their editors understand what kind of input is expected. - 🇬🇧United Kingdom catch
Since media entities are translatable, I don't think we need to support different tracks for different languages. You probably only want the track for the current language (like how you would handle translations for other entities).
I don't think this is right for subtitles. You can have a video in a single language, with subtitles in 15 languages, and as someone watching the video, you would want those subtitles options to be available via the video player, not by having to view different language versions of the entity. i.e. you can watch an English language film with English, English captions, French, or Arabic subtitles. Even if someone is browsing a site in French, and there is a video that only has English audio, they still might want to choose English subtitles instead of French (i.e. if they're trying to learn English and want to pause and read sometimes when it's hard to hear).
- 🇳🇱Netherlands seanB Netherlands
I guess you are right we probably need to support multiple subtitle files in each translation, since the source field could technically be different for each translation. So you could have a different video for each language, while still wanting subtitles in multiple languages for each translation.
Then we only need to decide how to deal with the
kind
attribute. I don't feel I'm the best person to make a decision here. At the moment, I'm thinking it is already helpful to be able to configure a singlekind
value on the formatter for all the files uploaded in the selected file field. That should also be relatively easy to implement. If we need to support multiplekind
attributes, I think we need to be able to configure a different file field for eachkind
. This is a bit more complex, but if we need to support this I would probably vote to do it like this. - 🇩🇪Germany Anybody Porta Westfalica
Thanks for the discussion! I think we should split this then to not block the poster image implementation, which Google Search Console complains about. This is the key feature most users need.
Implementing transcript is also helpful, but not that widely used, I think. So I'd suggest
- We change the title of this issue to "Add poster image to HTML5 media videos"
- We create a sibling issue: "Add transcript to HTML5 media videos" as follow-up and postpone it on this one
- We split the code so that this can be reviewed and merged and work on the transcript feature can be proceeded in detail later on.
Would have been better we did this earlier, sorry.
- 🇬🇧United Kingdom catch
Yes splitting is good, but I think we should use
track
for the new issue rather than transcript, since that covers the transcript vs. subtitle use-cases and the HTML element is the same for both. - 🇩🇪Germany Grevil
Created the child issue here: ✨ Add track to HTML5 media videos Postponed .
- last update
over 1 year ago 29,805 pass - last update
over 1 year ago Custom Commands Failed - Status changed to Needs review
over 1 year ago 3:06pm 5 July 2023 - 🇩🇪Germany Grevil
Alright, I removed all the transcript parts. Please review, in case I missed anything!
Also, do we still need to implement the hook_ENTITY_TYPE_presave(), as discussed in #90 and #91? Still unsure about that bit.
- last update
over 1 year ago 29,803 pass - last update
over 1 year ago 29,799 pass - last update
over 1 year ago 29,804 pass - Status changed to Needs work
over 1 year ago 8:17am 8 August 2023 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.
- First commit to issue fork.
- last update
over 1 year ago 30,148 pass - Status changed to Needs review
over 1 year ago 6:04am 11 September 2023 - last update
over 1 year ago 30,148 pass - Status changed to Needs work
over 1 year ago 2:30pm 11 September 2023 - 🇺🇸United States smustgrave
Seems we are missing upgrade test coverage. Should be simple though to add.
- 🇮🇳India shree.yesare
Updated the patch from #48 ✨ Add poster image to HTML5 media videos Needs work to include media entity reference field for thumbnail image on Drupal 10.x
- 🇮🇳India shree.yesare
Updated the patch from #48 ✨ Add poster image to HTML5 media videos Needs work to display default image from selected poster image field, if no thumbnail is added to video media.
- 🇮🇳India shree.yesare
Updating the patch in #109 →
Fix issue if no default image is added to field. - 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
Why using the patch from #48?
There is a merge request that is far more recent? containing solution after several discussions with good points ..
- 🇮🇳India Priya gupta
This patch only for poster attribute.
1. Once the patch applies, then add existing image field
2. Change the poster option as an image in Manage display in Video Media type - 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
Resolved merge conflicts after 📌 Convert FieldFormatter plugin discovery to attributes Active was committed.
- 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
Hiding all old patches
- Status changed to Needs review
7 months ago 10:27pm 21 June 2024 - Status changed to Needs work
7 months ago 4:06pm 22 June 2024 - 🇮🇳India renukakulkarni
Re-rolled the patch added in #119 ✨ Add poster image to HTML5 media videos Needs work as this was failing to apply for 10.2.x until this issue is merged into a stable release.
- 🇮🇳India rahulrasgon
Rerolling https://www.drupal.org/project/drupal/issues/2954834#comment-15374122 ✨ Add poster image to HTML5 media videos Needs work for Drupal 10.3.1
- 🇮🇳India rahulrasgon
Above patch https://www.drupal.org/project/drupal/issues/2954834#comment-15718821 ✨ Add poster image to HTML5 media videos Needs work having some issue. Uploading the new patch for Drupal 10.3.x
Thanks!! - 🇳🇱Netherlands Martijn de Wit 🇳🇱 The Netherlands
Did you try the merge request ?
- 🇩🇪Germany VISIOS
Could not apply patch #122 for Drupal 10.3.2. Can someone post a working patch for composer? Thanks!
- 🇩🇪Germany larskleiner
I was able to apply patch #122 to Drupal 10.3.5 but it's missing the feature to configure an image style for the thumbnail image. I guess that feature is available in the merge request(s) but I can't apply either 4010 or 4046.
However, patch #122 includes the transcript feature which rather should be addressed in https://www.drupal.org/project/drupal/issues/3372592 ✨ Add track to HTML5 media videos Postponed .
Attached a patch that allows for image style config but without any transcript fields. Tests are also missing. Works with Drupal 10.3.5.
- 🇩🇪Germany Anybody Porta Westfalica
@smustgrave re #117
Still believe it needs upgrade tests
Are we sure this is needed? (As you wrote "believe").
I'm asking because such old issues are terrible for Drupal (but of course broken upgrades, if relevant and realistic, are also). I think I'm not the only one who's finally in the state of thinking that opening a core issue won't make sense if the functionality should be seen in this decade... but let's not have this frustrating discussion here... I totally understand the needs for tests! Just had to cry a bit looking at the age of this issue... so let's get things done. Other persons will have to decide on these topics and possible improvements.So what's expected for these tests? Seems like nobody is able to write them, including me.
You wrote
Should be simple though to add.
could you tell how or give an example from somewhere else?
- 🇺🇸United States smustgrave
Left comments on the MR.
Will say we have 1000s of tickets that go back 15+ years, it's not that they're ignored it's just how much effort is being put into it. Just briefly looking seems this was re-rolled a bunch but not pushed forward.
✨ Add "Disable image resize" setting to image fields Needs review may be a good example of test.