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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matthieuscarset created an issue. See original summary.

matthieuscarset’s picture

The 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 :)

cilefen’s picture

Status: Active » Needs review

Status: Needs review » Needs work
rwohleb’s picture

Reroll of patch #2 for formatting so that it applies cleanly with composer-patches.

matthieuscarset’s picture

Status: Needs work » Needs review

Thanks you for the reroll @rwohleb.
Marking this thread as Needs review then.

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lhangea’s picture

Status: Needs review » Needs work
matthieuscarset’s picture

Reroll 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?

Status: Needs review » Needs work
lhangea’s picture

lhangea’s picture

Status: Needs work » Needs review
matthieuscarset’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the clean up @lhangea.
I've tested the patch and it rolls correctly on 8.7.

Marking this as RTBC.

matthieuscarset’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

In 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:

  1. https://www.drupal.org/docs/8/testing
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x
a.milkovsky’s picture

I can confirm, that #12 fixes the issue. Drupal 8.6.2.

hilly510’s picture

Patch in #12 works for 8.6.3

thejacer87’s picture

first crack at the tests.

confirmed it failed before the fix in #12 and works after patch is applied

alexpott’s picture

Status: Needs review » Needs work

@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.

  1. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -64,7 +64,15 @@ protected function createMediaField($formatter, $file_extensions, array $formatt
    -    $display = entity_get_display('entity_test', 'entity_test', 'full');
    +    $values = [
    +      'targetEntityType' => $entity_type,
    +      'bundle' => 'entity_test',
    +      'mode' => 'full',
    +      'status' => TRUE,
    +    ];
    +    $display = \Drupal::entityTypeManager()
    +      ->getStorage('entity_view_display')
    +      ->create($values);
    

    Why are these changes necessary?

  2. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -89,4 +97,18 @@ public function dataProvider() {
    +  /**
    +   * Data provider for testAttributes.
    +   *
    +   * @return array
    +   *   An array of data arrays.
    +   *   The data array contains:
    +   *     - An array of settings for the field formatter.
    +   */
    +  public function attributesProvider() {
    

    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?

thejacer87’s picture

@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

thejacer87’s picture

thejacer87’s picture

thejacer87’s picture

Status: Needs work » Needs review

The last submitted patch, 22: drupal-fix-attributes-filemediaformatterbase-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

The test looks great.

+++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
@@ -64,7 +64,15 @@ protected function createMediaField($formatter, $file_extensions, array $formatt
-    $display = entity_get_display('entity_test', 'entity_test', 'full');
+    $values = [
+      'targetEntityType' => $entity_type,
+      'bundle' => 'entity_test',
+      'mode' => 'full',
+      'status' => TRUE,
+    ];
+    $display = \Drupal::entityTypeManager()
+      ->getStorage('entity_view_display')
+      ->create($values);

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.

thejacer87’s picture

heres the patch without the deprecation fix

wells’s picture

#27 applies cleanly on 8.6.7 and resolves the issue.

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.

ReViJa’s picture

For when will this patch enter the core?
Version 8.6.13 and same problem

Thanks !!

ericgsmith’s picture

Status: Needs review » Reviewed & tested by the community

Issue is still present in 8.7.0

#27 applies cleanly and fixes the issue.

pandaski’s picture

Applied patch #27 to 8.8.x-dev, looks good

alexpott’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed cbb7ef0 and pushed to 8.8.x. Thanks!

Will backport to 8.7.x once the branch is out of commit freeze.

diff --git a/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
index 2727e6911d..1b9982d551 100644
--- a/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
+++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
@@ -55,7 +55,6 @@ public function testRender($tag_count, $formatter_settings) {
     $assert_session->elementExists('css', "video > source[src='$file2_url'][type='video/mp4']");
   }
 
-
   /**
    * Tests that the attributes added to the formatter are applied on render.
    */
@@ -94,7 +93,6 @@ public function testAttributes() {
     $assert_session->elementExists('css', "video[autoplay='autoplay'] > source[src='$file_url'][type='video/mp4']");
     $assert_session->elementExists('css', "video[loop='loop'] > source[src='$file_url'][type='video/mp4']");
     $assert_session->elementExists('css', "video[muted='muted'] > source[src='$file_url'][type='video/mp4']");
-
   }
 
 }
diff --git a/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
index 1b9982d551..8517b3244d 100644
--- a/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
+++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
@@ -65,7 +65,7 @@ public function testAttributes() {
       [
         'autoplay' => TRUE,
         'loop' => TRUE,
-        'muted' => TRUE
+        'muted' => TRUE,
       ]
     );
 

Fixed a couple of coding standards.

  • alexpott committed cbb7ef0 on 8.8.x
    Issue #2969691 by thejacer87, matthieuscarset, lhangea, rwohleb,...
alexpott’s picture

Status: Patch (to be ported) » Fixed

  • alexpott committed f71da89 on 8.7.x
    Issue #2969691 by thejacer87, matthieuscarset, lhangea, rwohleb,...

Status: Fixed » Closed (fixed)

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

emb03’s picture

This is still a problem in 8.9.7 Was this included in core?

emb03’s picture

Priority: Normal » Major

This automagically started working after two days.

emb03’s picture

Priority: Major » Minor