Add support of "preload" attribute into FileMediaFormatterBase

Good job for your first issue! :)
I have a few comments, though.

1. +++ w/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
   @@ -66,6 +67,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +          'none' => $this->t('one : The author thinks that the browser should NOT load the video when the page loads'),
   

   There is a "N" missing at the beginning of the translatable string.

2. +++ w/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
   @@ -66,6 +67,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +        '#options' => [
   +          'none' => $this->t('one : The author thinks that the browser should NOT load the video when the page loads'),
   +          'auto' => $this->t('Auto : The author thinks that the browser should load the entire video when the page loads'),
   +          'metadata' => $this->t('Metadata : The author thinks that the browser should load only metadata when the page loads'),
   +        ],
   

   Options values should follow these rules:

   • no space before the colon symbol in english
   • no capitalized letter after the colon symbol
   • each string should end with a dot

Updated latest patch according latest comments.

The code looks good.
We should increase the test coverage to ensure that this is working properly and that we will not break it in the future :)

1. +++ w/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
   @@ -66,6 +67,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +        '#options' => [
   +          'none' => $this->t('None: the author thinks that the browser should NOT load the video when the page loads.'),
   +          'auto' => $this->t('Auto: the author thinks that the browser should load the entire video when the page loads.'),
   +          'metadata' => $this->t('Metadata: the author thinks that the browser should load only metadata when the page loads.'),
   +        ],
   

   I suggest using more objective text explaining what the option does. From the referenced page:

   preload
   This enumerated attribute is intended to provide a hint to the browser about what the author thinks will lead to the best user experience with regards to what content is loaded before the video is played. It may have one of the following values:

   • none: Indicates that the video should not be preloaded.
   • metadata: Indicates that only video metadata (e.g. length) is fetched.
   • auto: Indicates that the whole video file can be downloaded, even if the user is not expected to use it.

   https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video

2. Since the patch is changing existing configuration options, it should include an upgrade to add the new option to existing configuration.

A few thoughts on this issue:

1. Why include the super verbose option labels at all? It makes the select list super long (see screenshot) and doesn't seem to be the correct place to document things like that. IMO we should either leave it off (the docs aren't hard to find), or put a link to the Mozilla docs in the description text for the full field.
2. Should this be moved up a level to FileMediaFormatterBase, since this attribute applies to both video and audio tags? Other attributes like loop or autoplay live there.
3. In my testing, since the field formatter provides a default value (metadata), once it's applied the setting is used by existing files. Sequential config exports will then add the new setting to existing config. Is an update still necessary?

I've attached a patch with my suggested changes (minus the description, which I'd be more than happy to add).

Fixed failing test

What about existing configuration? We need to populate this with a default value - looks like the choice is "metadata". So we need a post update function to use the ConfigEntityUpdater class to add this configuration. We also need a test for it working.

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
@@ -56,6 +57,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+        '#type' => 'select',

Given there are only three options should this be radios? And maybe a #description is warranted that explains what this setting is about. I don't find the words that self explanatory.

@pclaitte there is existing configuration which does not have the preload configuration. We need a post update function to find this configuration and set it to the default value. If the configuration is optional then we need to handle it differently in the form. With the patch as it stands if you open an existing field formatter config form and, without making any changes, press save it'll change the configuration and that shouldn't happen.

@alexpott I'd love to get this in, I've added @pclaitte changes onto an MR and the following changes:

- Widget is now radios as suggested
- Added short descriptions for the 3 options.
- Added a post update with a config updater

I think the update needs a test but I'd appreciate some guidance on where to find something similar (this has been my first encounter with the config entity updater)

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This seems like it's going to need an upgrade path for existing settings.

Patch looks good to me, already pretty polished...

What I'm wondering about is: how "safe" is to force a default preload attribute value on sites that didn't have any?

I mean, if I had a page full of videos with no 'preload' attribute, maybe just an image placeholder, and suddenly I get that preload=metadata default.... will browsers just start preloading when they didn't before?

The point is: does it make sense to force a default value? or to force any value at all? Should we skip the update script until the user actually edits the formatter?

For background, I've found this pretty interesting post about how browsers behave, but note it is 10 years old, couldn't find and updated one... https://www.stevesouders.com/blog/2013/04/12/html5-video-preload/

Setting to NR for more input as per #35

See the upgrade path but it needs test coverage. Could be something simple as

Before this value was null
Run updates
This value is now false

Didn’t test the MR

@smustgrave please see #35 regarding update path.

Yes the update hook is there but there is no test coverage of it.

See BlockContentUpdateTest for example

@smustgrave the question in #35 refers to whether we should have the update hook in the first place.

So if I do a config export.
Go into a field or display using this formatterBase.
If I make 0 changes but save the field and do drush export again and it's showing this new setting then yes it will need an update path.

If no then it will not.

The update hook should set to disabled value though.

That help some?

@smustgrave "Should we skip the update script until the user actually edits the formatter?" That's exactly what #35 is explaining.

It's ok to assume changes can happen when I save the formatter, but NOT when I just upgrade my site. That could have unexpected consequences, and that's what #35 is asking for feedback from @alexpott.

Actually the opposite if I save a formatter without making a change and I get changes in my config now that should not be expected.

Same for how we handle other field changes but will let alexpott respond.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I only scanned the MR quickly, but I couldn't see the a presave hook to update shipped config (i.e. default config in modules), that should happen as well as the update function.

We need to update the configuration to something in both post update and presave, so that sites don't get unexpected config changes in runtime - better if they get them during a core update which is when they'll be looking for them.

So.. I think it comes down to what a non-intrusive update looks like - i.e. should it be 'metadata' or 'none', or do a need an additional option for 'unspecified' to preserve current behaviour and update sites to that?

Didn't look around for docs on browser behaviour yet so not sure what the answer is, but definitely we should update to something here.

Rebased 10.1.x into MR; providing patch for 9.5 in case anyone needs it.
Leaving in NW as per #45.

#3309016: Add image preload option to help boost actual and perceived performance is doing some similar things for images. It also has some of the bits around update hooks that are being asked about in #40 and below. For already committed code, look at what ResponsiveImageConfigUpdater is doing.

Can we update the IS to differentiate between what _this_ issue is doing vs #3309016?

Created MR against 11.x. Adding patch for 10.3.x.

Has this not been implemented? If so, how do I find it? I do not want to apply a patch for something that won't show up in production. But I am sorry for wasting anyone's time if I am missing something...

I have no preload options at /admin/structure/media/manage/audio/display

Not being able to prevent preloading of media in D10 is a deal-breaker for me, I cannot use Drupal media. I create sites with audio library pages using views, and when a page of multiple audio files is displayed, sometimes dozens, I cannot have them all downloading.

For this reason, I am compelled to retain the method I used in Drupal 7, which was to create fields with preconfigured HTML5 code using tokens. Doing that I need a file field and two text fields for every audio file (the easiest way without making a custom module, as I am not a developer).

After adding a poster image to video files in #2954834: Add poster image to HTML5 media videos with requireKey: false, the preload option do not need an updater and updater test coverage as well?