Fresh Drupal 8.5.x install. I've added a Media Video and I try to mute it in its display mode.
Unfortunately, the video is rendered without the attribute therefore, it's not muted.
How to reproduce
- Install Drupal 8.6.x
- Enable the Media module
- Create a Media Video type
- Edit the Display Mode and select the "Muted" attribute to YES.
- Create a new video and see it on front end.
Expected result
- On front, the video is muted.
Actual result
- On front, the video is NOT muted - muted attribute is not printed in the HTML markup.
I've attached a patch.
This is a quick fix.
Comments
Comment #2
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and at Appnovation for Appnovation commentedThe issue is actually caused by the array union in
FieldMediaFormatterBase::prepareAttributes()
function.Indeed, using array union like
['control', 'autoplay', 'loop'] + $additional_attributes
limit us to pass extra parameters.The
muted
attributes is actually not taken into account because the first array contains 3 items only.It is more flexible for future addition to the Media player's attributes to use
array_merge()
.And it's a quick fix for this issue :)
Comment #3
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #5
rwohlebReroll of patch #2 for formatting so that it applies cleanly with composer-patches.
Comment #6
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and at Appnovation for Appnovation commentedThanks you for the reroll @rwohleb.
Marking this thread as Needs review then.
Comment #8
lhangea CreditAttribution: lhangea as a volunteer commentedRerolled
Comment #10
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedReroll was missing a newline at the end. This is why the test failed in #8.
BTW, I do not see any significant differences in patches' code since mine sent in #2.
I guess this issue can be RTBC then, isn't it?
Comment #12
lhangea CreditAttribution: lhangea as a volunteer commentedThis should be fine
Comment #13
lhangea CreditAttribution: lhangea as a volunteer commentedComment #14
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedThanks for the clean up @lhangea.
I've tested the patch and it rolls correctly on 8.7.
Marking this as RTBC.
Comment #15
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedComment #16
alexpottIn order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:
Comment #17
a.milkovskyI can confirm, that #12 fixes the issue. Drupal 8.6.2.
Comment #18
hilly510 CreditAttribution: hilly510 commentedPatch in #12 works for 8.6.3
Comment #19
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedfirst crack at the tests.
confirmed it failed before the fix in #12 and works after patch is applied
Comment #20
alexpott@thejacer87 thanks for working on the tests. One thing we do is upload a test-only patch along with the fix patch - that way you can prove to the community the new tests fail as expected.
Why are these changes necessary?
The provider should probably be in FileVideoFormatterTest. Also using a provider when there is only one test seems unnecessary - or is there a need to expand this?
Comment #21
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commented@alexpott
1. i noticed that `entity_get_display` was deprecated, so i figured i would just take care of it now
2. no rhyme or reason, just copying the same pattern the other test does. but i see now the dataProvider in the testbase is used by both audio and video
ill fix it up and repost an updated failing patch and working patch
Comment #22
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedhere's just the test
Comment #23
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedand the full patch
Comment #24
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedComment #26
alexpottThe test looks great.
Whilst technically correct this change is still out-of-scope. Replacing all the usages of the deprecated entity_get_display() needs to be done a separate issue.
Comment #27
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedheres the patch without the deprecation fix
Comment #28
wells#27 applies cleanly on 8.6.7 and resolves the issue.
Comment #30
ReViJa CreditAttribution: ReViJa commentedFor when will this patch enter the core?
Version 8.6.13 and same problem
Thanks !!
Comment #31
ericgsmith CreditAttribution: ericgsmith as a volunteer commentedIssue is still present in 8.7.0
#27 applies cleanly and fixes the issue.
Comment #32
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedApplied patch #27 to 8.8.x-dev, looks good
Comment #33
alexpottCommitted cbb7ef0 and pushed to 8.8.x. Thanks!
Will backport to 8.7.x once the branch is out of commit freeze.
Fixed a couple of coding standards.
Comment #35
alexpottComment #38
emb03 CreditAttribution: emb03 commentedThis is still a problem in 8.9.7 Was this included in core?
Comment #39
emb03 CreditAttribution: emb03 commentedThis automagically started working after two days.
Comment #40
emb03 CreditAttribution: emb03 commented