Video files using the FileVideoFormatter have a fixed dimension

Tested this on 9.3.x-dev site, went to /admin/structure/media/manage/video/display was able to reproduce problem (width and height required).
Applied #3 patch went back to same page .. width and height no longer mandatory. Emptied width and height values and saved changes, no error messages. Uploaded a test video and made a view for it, changed width and height all seems ok. Ran similar tests with a video on article page, all fine. Can see #required attribute has gone from settingsForm() in FileVideoFormatter.php.

I think we need to fix \Drupal\file\Plugin\Field\FieldFormatter\FileVideoFormatter::settingsSummary() to deal with this not being set and also \Drupal\file\Plugin\Field\FieldFormatter\FileVideoFormatter::prepareAttributes(). I think we should only set the attributes if they are set. It would be good to test the HTML outputted by the file formatter when only width or height is set and when neither is set.

I just ran into this for a client. I opened an MR, committed #3 as the first commit, and pushed another commit to address most of @alexpott's concerns in #7. Doesn't expand test coverage, so tagging for that.

Saving credit for authors (myself and @zrpnr) and reviewers (@schillerm and @alexpott).

I added an additional functional test, checking that the width and height attributes are missing when the formatter settings have a null value in the width and height attributes.

I am removing the Needs tests tag, and changing the status to needs review.

MR appears to have a failure.

The pipeline error seems to be related to this other issue ( https://www.drupal.org/project/drupal/issues/3401988 ) and not with the merge request itself. The branch itself is mergeable, though.

Should we still merge the code back to the project ? Or should we make the pipeline run correctly first ?

IMO, this is a really small change, and it also have associated tests, so it should be safe to just accept the merge request.

The second option might require to merge the changes back to the issue branch, so the pipeline is updated with the fix from the issue, and then, push so it runs without hitting the error ? I am not completely sure about this.

I have added one more test to validate the HTML outputted by the file formatter when only width or height is set.

Moving back to needs review

Sadly, there is a failed test. Rebasing..

All tests passed 🎉

Seems close.

Since this is changing the current field formatter believe we will need a CR with screenshots. Screenshots could be added to the issue summary here too.

Hi,
I Have tested the latest MR !5057 on Drupal version 11.0
MR Applied successfully...

Testing steps"

1. Install the Drupal version 11.x
2. Install the Media and Media Library core Modules
3. Go to media types ->video -> manage display ->video types field format -> check for dimension field, it's mandatory
4. Can check on content also, add video media for any content type, and create content by adding different size videos (video size will be fixed as in format)
5. Apply MR, and check for the same

Testing Result:
For the Media type -> video ->dimention field not mandatory
It can save black and videos displayed with actual size. Also able to set fixed dimensions as per requirement

Attaching screenshots and videos as #26

Thanks for testing @Kanchan Bhogade.

I have updated the issue summary and created a Draft change record: https://www.drupal.org/node/3418545

Screenshots are added in #27 so removing the tag also I reviewed the CR it seems fine. Leaving it to someone else to review.

CR seems good.

Cleaned up the issue summary as the remaining tasks aren't API changes.

Also screenshots belong in the User interface changes section.

Torn if this is a feature request but will leave as is.

Issue credits

Hiding patch

Left some MR comments

Hi @larowlan, thank you for your suggestions.

I changed the formatter summary text, so there is one line for each width / height attribute that has been set.

I also joined the tests for the different cases, only width, only height, both, and none, in the same test method, changing the field config and loading the page again in the same method.

Waiting to see if all the pipelines run smoothly.

I realized the functional test for testing the multiple width, height, both, none cases was wrong.
Fixed that in last commit.

Feedback appears to be addressed

@smustgrave sorry about that, i will do that in future ocasions. Thanks for pointing that out, i didn't know.

Committed to 11.x and backported to 10.3.x
Published CR

Let's also update the config schema to match this change: #3425318: Update `type: field.formatter.settings.file_video` to match the validation logic in FileVideoFormatter's settings form.