Problem/Motivation

Drupal currently has no support for local audio and video files. That means, you are able to upload such files in file fields, but Drupal has no possibilities to render these files as HTML 5 video or audio elements.

It would also be a great addition to the existing media implementation to support local video and audio files. With that functionality it would be possible to ship media types that support local video and audio handling.

Proposed resolution

The contrib file_entity module ships video and audio formatters since it's start. Move these two formatters in core.

Remaining tasks

  • Create a core patch, based on the video and audio field formatters from file_entity
  • Refactor those formatters
  • Commit

User interface changes

Users are able to select a video or audio formatter on "Manage display" for file fields, if the file extensions of the file field apply to the formatter.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#124 Screen Shot 2017-11-17 at 2.59.03 PM.png1.7 MBwebchick
#124 Screen Shot 2017-11-17 at 2.58.35 PM.png1.49 MBwebchick
#124 Screen Shot 2017-11-17 at 2.57.03 PM.png87.45 KBwebchick
#122 interdiff-1174892-120-122.txt6.41 KBchr.fritsch
#122 1174892-122.patch25.76 KBchr.fritsch
#120 interdiff-1174892-113-120.txt8.61 KBchr.fritsch
#120 1174892-120.patch26.01 KBchr.fritsch
#113 interdiff-1174892-110-113.txt3.68 KBchr.fritsch
#113 1174892-113.patch24.07 KBchr.fritsch
#110 interdiff-1174892-106-110.txt7.43 KBchr.fritsch
#110 1174892-110.patch24.04 KBchr.fritsch
#106 interdiff-1174892-101-106.txt1.81 KBchr.fritsch
#106 1174892-106.patch24.04 KBchr.fritsch
#101 interdiff-1174892-95-101.txt1.86 KBchr.fritsch
#101 1174892-101.patch24.18 KBchr.fritsch
#95 interdiff-1174892-93-95.txt4.43 KBchr.fritsch
#95 1174892-95.patch23.85 KBchr.fritsch
#93 interdiff-1174892-92-93.txt8.57 KBchr.fritsch
#93 1174892-93.patch23.33 KBchr.fritsch
#92 interdiff-1174892-90-92.txt11.33 KBchr.fritsch
#92 1174892-92.patch25.08 KBchr.fritsch
#90 interdiff-1174892-87-90.txt583 byteschr.fritsch
#90 1174892-90.patch25.13 KBchr.fritsch
#87 interdiff-1174892-84-87.txt3.44 KBchr.fritsch
#87 1174892-87.patch25.15 KBchr.fritsch
#84 interdiff-1174892-82-84.txt2.13 KBchr.fritsch
#84 1174892-84.patch24.88 KBchr.fritsch
#82 interdiff-1174892-80-82.txt14.03 KBchr.fritsch
#82 1174892-82.patch25.29 KBchr.fritsch
#80 interdiff-1174892-78-80.txt13.17 KBchr.fritsch
#80 file_field_formatters-1174892-80.patch26.55 KBchr.fritsch
#78 interdiff-1174892-72-78.txt21.53 KBchr.fritsch
#78 file_field_formatters-1174892-78.patch26.1 KBchr.fritsch
#73 file_field_formatters-1174892-72.patch28.7 KBkeithm
#71 interdiff-1174892-69-71.txt1.19 KBchr.fritsch
#71 file_field_formatters-1174892-71.patch28.67 KBchr.fritsch
#69 interdiff-1174892-67-69.txt11.89 KBchr.fritsch
#69 file_field_formatters-1174892-69.patch28.68 KBchr.fritsch
#67 interdiff-1174892-63-67.txt12.69 KBchr.fritsch
#67 file_field_formatters-1174892-67.patch28.78 KBchr.fritsch
#63 interdiff-1174892-61-63.txt10.3 KBchr.fritsch
#63 file_field_formatters-1174892-63.patch30.72 KBchr.fritsch
#61 file_field_formatters-1174892-61.patch30.89 KBchr.fritsch
#58 file_field_formatters-1174892-58.patch27.19 KBchr.fritsch
#55 file_field_formatters-1174892-55.patch13.11 KBchr.fritsch
#39 drupal-video_audio_support-1174892-39.patch368.66 KBtravist
#22 drupal-video_audio_support-1174892-22.patch347.06 KBtravist
#21 drupal-video_audio_support-1174892-21.patch1.42 MBtravist
#17 drupal-video_audio_support-1174892-17.patch1.4 MBtravist
#19 drupal-video_audio_support-1174892-19.patch1.39 MBtravist
#1 1174892-html5-audio-video-tags.patch1.61 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs work
FileSize
1.61 KB

Very basic initial patch.

Todos:
- Make actual theme functions for audio and video.
- Support the audio/video element in the formatter settings.

travist’s picture

Dave,

This is definitely a great start, but I do see one issue with this approach. I can see you are using the multiple file upload to print out a new player for each file uploaded to the file field, so that the patch above would produce the following output...

  <video src="file1.mp4"></video>
  <video src="file2.mp4"></video>

IMO, we should use the multiple upload capabilities from file field to provide the ability for the user to upload several formats of the same video ( OGG and MP4 ) so that it will work as HTML5 for most modern browsers. The code for this is actually something like this...

  <video>
     <source src="file1.mp4" type="video/mp4">
     <source src="file2.ogv" type="video/ogg">
  </video>

I already have this working in the HTML5_media module above so check that out and let me know what you think, and of course, please give me your thoughts on this approach.

Travis.

travist’s picture

I like your approach just using the formatters and not a widget for media display... I will be making those changes to the html5_media module, and then make some changes to include the source element if you agree with my comment above.

Thanks again.

Travis.

Jacine’s picture

Subscribe! :D

ericduran’s picture

Hmm, when supporting multiple src we're going to want to figure out how core is going to handle one element with multiple attributes of the same type.

We need to figure out how we're go to account for one video element having multiple srcs, examples:

<?php
 array(
 'type' => 'video',
 'src' => array(
  'video/mp4' => 'file1.mp4',
  'video/ogg' => 'file2.mp4',
  )
)
?>

The above example should print

<video>
     <source src="file1.mp4" type="video/mp4">
     <source src="file2.ogv" type="video/ogg">
  </video>

Yet

<?php
 array(
 'type' => 'video',
 'src' => array(
  'video/mp4' => 'file1.mp4',
  )
)
?>

should print

<video src="file1.mp4"></video>

This is just me subscribing to the issue ;)

Dave Reid’s picture

I believe that it should be up to contrib as long as it's possible to do. Multiple-source video fields could use Field collection and a small contrib module could be written (or put in html5_media) for the multi-source video field collection formatter.

I think the 80% of use case will just want to upload a video, not have multiple formats. We should be working to do that in core, but ensure the more complex use case is possible in contrib.

RobLoach’s picture

travist’s picture

Thinking more about this, we may have an issue we need to overcome...

Dave, I agree with your 80% as long as they have a Flash/Silverlight fallback in place to allow that single video to be playable in all browsers. Herein lies the problem. In order to do Flash fallback the right way, you still need to nest the object element within the video element as follows..

<video src="myfile.m4v">
  <object .....   />
</video>

If we are to go ahead and build this nesting to accomplish fallback, then my question is can we build this to work for the source element as well? What are your thoughts on that? Also, is there currently a way in Drupal infrastructure to get this type of element nesting on a per-field basis?

Your help and guidance is appreciated.

Thanks,

Travis.

Dave Reid’s picture

MediaElement does have Flash and Silverlight fallback included in the library.

Everett Zufelt’s picture

The problem with HTML5 video fallback is that it doesn't work if you are using a user agent that supports HTML5 video.

That is if you are using Firefox 8 and embed an H.264 video as the src of the video Firefox will not play the video, because it doesn't support H.264, but it also will not render the fallback content, because it supports the video element.

See https://developer.mozilla.org/en/Using_audio_and_video_in_Firefox

travist’s picture

I understand that MediaElement has Flash fallback, but the biggest problem I see is that it writes HTML output dynamically using JavaScript. This will completely undermine any theming efforts where theme developers would like to alter the markup generated by this library. The better option is to allow the themers decide how the markup of their player should look by using the Drupal theme layer (maybe a media_player.tpl.php?) which will allow them to create their own themes for their players. I envision future themes would have a media player that fits perfectly in with their theme.

Another concern I have with MediaElement is it is huge. Last time I checked the download was 23mb. Understand that we could remove some of the libs he includes, but it would still add a significant footprint to drupal core. My goal is to create the absolute minimalistic player with a plugin type architecture that would allow contrib modules to add functionality to the player. All of this is already working within the http://drupal.org/project/html5_media project.... I just need to go ahead and create a patch in this issue with that project ported to the latest D8 branch, but please take a look at that project files and "player" folder to see what I am working on. I am not saying that this is the absolute right way to go, but I would at least like to have people give it a test drive and see if it would work for Drupal core.

There are some things that need to be figured out with what I did, like uploading additional files like subtitles to be shown with the media, but I think a first run patch is needed. I will try to have a patch within the next two weeks.

travist’s picture

Component: file.module » file system
Status: Needs review » Needs work
Issue tags: -Media Initiative

Here is a github repo of what I am working on. It is easier to explore the code.

https://github.com/travist/drupal_multimedia

Dave Reid’s picture

Component: file system » file.module

Sure the actual source files of Mediaelement that would be used as a library are 430KB which includes the silverlight and flash files. I understand the concern about theming, but for most people that just want to upload a video and play it, they likely would be overriding a simple formatter anyway if they want to theme it.

I'll be sure to take a look at what's going in the Github repo for drupal-html5-player! My concern with rolling our own library is just frankly core code maintenance. Once we put it in we have to maintain it forever. That's not a decision to take lightly.

travist’s picture

Yeah, I am officially putting my foot in my mouth because looking at his library, I see that 20 of those MB's are included media. So, yeah that argument is invalid. :)

But I also want to mention another concern. That is that MediaElement.js cannot use some libraries that are already included in D7-D8... primarily jQuery UI. Take a look at this code... https://github.com/johndyer/mediaelement/blob/master/src/js/mep-feature-.... Here he is replicating code that jQuery UI already figured out with their slider control. Here is what that code would look like using jQuery UI ( https://github.com/travist/drupal_media_player/blob/8.x/core/modules/fil... ).

I think my point is that we have the ability to make a Drupal specific player that will completely cater to the themeing community to where they would not have to alter it, but rather build on top of it and also provide the markup they want in their theme. This would be very powerful. If you think that people will just override something, then we probably should keep it out of core all together. The goal here is to try to attempt to get this much needed functionality into core.

Your thoughts?

Everett Zufelt’s picture

If the module is using any jQuery UI components, and you would like it to be considered for Core, then one important step is to make sure that the implementation of the components used doesn't have any accessibility problems.

You can see a list of open accessibility bugs against jQuery UI at http://bugs.jqueryui.com/query?status=!closed&keywords=~a11y

Dave Reid’s picture

Issue tags: +Media Initiative
travist’s picture

Status: Needs work » Needs review
FileSize
1.4 MB

So, I know there will be some issue queue churn from this, but I want to get this out there so that everyone has time to digest before we meet in Denver to discuss this.

Here is my patch that introduces <video> and <audio> support into Drupal 8! Instead of going into detail of what this patch provides, I recorded a video where I walk through what this introduces into Drupal core.

http://www.youtube.com/watch?v=qY8ezHNiZPA

In addition, I have committed my changes to https://github.com/travist/drupal_multimedia, and you can view the changes made to core by looking at the following URL.

https://github.com/travist/drupal_multimedia/compare/8.x...8.x-media

If anything, this is a good solid start to get this functionality into Drupal 8.

Comments and Critiques welcome!

Travis.

The last submitted patch, drupal-video_audio_support-1174892-17.patch, failed testing.

travist’s picture

Status: Needs work » Needs review
FileSize
1.39 MB

Let's try this one...

Status: Needs review » Needs work

The last submitted patch, drupal-video_audio_support-1174892-19.patch, failed testing.

travist’s picture

Alright, this one applies cleanly on my sandbox. Didn't realize that you had to use git diff --full-index --binary

travist’s picture

Component: file system » file.module
Status: Needs work » Needs review
Issue tags: +Media Initiative
FileSize
347.06 KB

So, the previous patch was pretty huge because I included the automated documentation which isn't necessary. Here is a new patch with only the required additions necessary to get this working (without the documentation for the player).

MrMaksimize’s picture

I have to agree here - the idea of using a template file and being able to build and theme the player the way I'd want it is extremely powerful. Overriding formatters is hard :)

Jacine’s picture

Issue tags: +sprint

Some community reviews and feedback would be great to see here. Tagging for the current sprint.

aspilicious’s picture

Ok my feedback / own opinion

1) You should wrap all the video/audio stuff in a media field somehow. It will be confusing to use file field for this and text field for that.
2) Don't (as in rly don't) use a table in field formatter settings. Use plain text seperated with a
.
3) The media player is a seperate non drupal library, right? So we should place it inside misc (like farbtastic)

Could you split the player from the drupal code when you reroll. Makes it easier to review

travist’s picture

I am totally open for feedback here, and was actually thinking that the player could live in the misc folder. So, for that reason, I don't see any problem with putting the player within the misc folder, but would like to get other opinions before I do a re-roll with this. Also, I will be in Denver and would like to meetup with everyone to go over this and we can talk it through there.

cosmicdreams’s picture

I would be ok with having a default media player in the misc folder as long as it is easy to override that player with another one. Custom video players are a frequent request / requirement of clients. Often times the video content network they pick governs the kind of player that can be used as custom branding of the player was built in (typically flash based players)

travist’s picture

That is another cool thing about this player is that it is really easy to build shims around those content video networks custom players and have it plug right into this framework providing a common API between it and say regular HTML5 media control. I have already written one for Limelight CDN (who has their own player), but using their JavaScript API you can create the shim I am referring too. Regardless, this media player is only brought in if they select the "Media Player" field formatter, so it can easily be swapped out for something else if you just provide a different field formatter.

klonos’s picture

I just saw the video and this looks really awesome! I wish things were that simple in D7 too.

I agree with Bram's 3rd point in #25, it is somewhat of a UX WTF to have to figure out/remember that you need to paste the URL in a text field that is then converted to a media player on display. Users enter a URL, so they expect the field to be at the very least titled so (even if in fact it is nothing more than a text field). How about using a "special" kind of text field called "Media URL"? That would perhaps also allow us to have it have a media player widget assigned for its display by default. Don't you thing that this would be more streamlined?

cosmicdreams’s picture

We now have form api support for url fields. Why not use that?

Everett Zufelt’s picture

I haven't looked at this issue in a while, so forgive me if any of this has been addressed.

The UI of the media player will need to be assessed for accessibility. High level:
- Can it be used with keyboard alone
- Can it be used with a screen-reader
- Does it have resizable text and proper colour contrast

For video, what formats does the player support, if any of those formats support caption/subtitlinghow does the player handle that?

For audio / video, there should be a field available to add a URL to the transcript, or to paste the transcript.

travist’s picture

Everett,

Currently the player does respond to the typical keyboard inputs including Space for Pause/Play, Right and Left for seek forward and backward, and of course ESC to exit full screen mode.

As for subtitles, that could be added without to much hassle since HTML5 provides a <track> tag that provides subtitles within the HTML5 player. I plan on implementing that really soon and pushing up a fix for that so I don't think that will be an issue.

As for resizable text and proper color contrast, this is controlled via CSS so I am confident that this isn't an issue as well, and if it is can easily be fixed.

In the next day or so, I will be pushing up a new version of the patch with an updated player code in it (which provides true fullscreen support for supporting browsers), and when that happens I would love for you to perform an accessibility audit on the player to make sure it upholds all your team requires.

Thanks,

Travis.

Everett Zufelt’s picture

@travist

Thanks for the feedback, sounds good.

Can you please provide a demo URL when you push your changes, this makes it far easier for others to provide quick feedback.

travist’s picture

I just created a BOF in Denver for us to get together and go over this...

http://denver2012.drupal.org/bof/rich-media-handling-and-drupal-8

travist’s picture

So, I think we should embrace this as the "core" player for Drupal.

http://popcornjs.org

This is a well established "core" player as I described above, but is maintained by the Mozilla Foundation. I do think that we will need to add some stuff to it to get the theming support, but hopefully this should be a really good start. I will start working on integrating this instead of the minPlayer and try to update this pull request accordingly.

Exciting stuff!

Travis.

klonos’s picture

I'd love to see this as a module for D7 too! Even if it is not accepted for D8 core, it's worth having it in contrib.

travist’s picture

This is already in a D7 module, which is http://drupal.org/project/html5_media. However, I am going to make a patch to the Media module on Friday to basically move this into the media module instead, which would end up obsoleting the html5_media module.

mgifford’s picture

Absolutely on the exciting...

Would really like to know what the ramifications would be of adding this.

travist’s picture

Here is the latest patch for this. I have still not had time to look into popcorn.js as the "core" for this media integration, but this is at least a good foundation to experiment with the integration with Drupal.

RobW’s picture

Couple points:

In response to #6 and #8 (which are a year old, so maybe opinions have changed): I can't think of a reason why anyone would be interested in adding a single format. Even if they do want to, it's the equivalent of (ok, it just is) coding an i.e. only site, and we certainly shouldn't be encouraging it. From a ux perspective, most end users will probably only want to upload one file, but that's what projects like Derivatives API and Media Derivatives are for.

JS libraries: Popcorn seems like overkill just for playing files -- from what I see its main use case is tying time based events together inside and outside an HTML media element. One of the major benefits of media in HTML is that the browsers supply controls natively. It seems to me there's no need for any js (even for fallback if we use Video for Everybody, below).

Structure: The most robust video tag structure seems to be Video for Everybody. It's well tested and as a recommendation on http://html5please.us has high visibility and consistent refinement.

We could use VfE syntax with no js whatsoever. If we want to go crazy with validation, we could make things extra light for modern browsers by removing the flash fallback in html and feature detecting html5 media element support (with modernizr and modernizr load aka yep/nope, or whatever feature detection and conditional loading is available in core), doing nothing if it passes and loading mediaelement.js only if it fails. Light and fast HTML5 media for supporting browsers, polyfill to add support for those that don't.

One of the important themes I see throughout initiatives for Drupal 8 is keeping the front end simple, allowing developers build up as opposed to forcing them to strip down. Simple, functional support for the elements should be the goal for core. If theming the player's controls requires an extra js file (mediaelement.js) let's make it optional, or better yet a contrib module.

[edited and expanded]

travist’s picture

Hello Rob,

I appreciate your input on this and am more than intrigued at this approach. My only concern is with regards to consistency, which is something that Video for Everybody does not address at all. It primarily rely's on the native controls which is inconsistent across ALL browsers. I am not necessarily saying that this is a problem and some people probably don't care, but there are plenty that do care about this issue. Considering that Drupal aims to provide a consistent user interface, I would think that this is something that should be addressed within core. But you are right... to provide consistency requires, and is heavily dependent on Javascript. There are goods and bads either way, and I would certainly love to hear what everyone else thinks about this.

I am pretty slammed at the moment, but once things calm down, I may prototype this up in code and just see how it compares to the previous solution.

Thanks again for your input,

Travis.

RobW’s picture

Hey Travis, thanks for the reply.

I spent some time looking through the HTML5 Media code today, and I think I understand more what you're looking to accomplish. It seems like the project's main goal is a consistent theming api for HTML5 audio and video tags, which I can completely get behind. In order for that to work there needs to be a way of adding, formatting, and theming the tags themselves, which you've provided, but only coupled with the first use case.

I think this could be a complete core solution if it moved in two steps:

  1. Provide formatters that produce the naked HTML5 audio and video elements.
  2. Extend those with the minPlayer for complete, Drupal style theming control.

One of the points brought up in the convert Drupal 8 theming to twig discussion was that Drupal can benefit from being less proprietary and more accommodating to front end developers who have their own toolsets and techniques that they apply across any number of projects. A two step system would cover developers who want the lightness of native code, those who have a HTML5 media player that they prefer from use across all of their projects, and those who like minPlayer because it works out of the box and does things the Drupal way.

Speaking to consistency, native players are all different, but they're also more performant and offer a consistant experience in browser. Think check boxes and radio buttons -- it's not considered bad form (HA groan) to let browsers implement native styling there, but if you want to put the extra effort into images, css, and some wrapper tricks, a custom style can better compliment design. I think video and audio are pretty much the same. There's clear benefits to both approaches.

Anyways, thanks for all the work you've put into this module so far. Testing and experimenting with it in a side project as we speak.

pschuelke’s picture

Status: Needs review » Needs work

I applied the patch from #39. It installed correctly and when I created a file field with an .mp4 video file uploaded, the manage display setting on the field was 'media player'; the video did not display. In the 'recent log messages it had 'Theme key "media_player" not found.'

Dave Reid’s picture

Version: 8.x-dev » 9.x-dev

Drupal 9 at this point. We did add support for HTML5 audio and video in the File entity module.

catch’s picture

Version: 9.x-dev » 8.1.x-dev
Issue summary: View changes
Dave Reid’s picture

Issue tags: +D8Media
slashrsm’s picture

Dave Reid’s picture

I'm curious if anyone has thoughts about where these field formatters and theme functions should live for Drupal core 8.0.x. Should we merge the effort with the HTML5 Tools module? Elements module? A new contrib module for audio and video formatters?

hass’s picture

Elements sounds best to me.

slashrsm’s picture

Elements module seems to be focusing on form elements. We are talking formatters here. What about file entity?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

spacereactor’s picture

Look like media as formatter isn't happening anything soon. There is audiofield port for drupal 8 https://github.com/joshirohit100/audiofield which work for audio formatted in drupal 8. And video.js for video formatted but video.js doesn't work well with views.

slashrsm’s picture

File entity module provides audio and video formatters: http://cgit.drupalcode.org/file_entity/tree/src/Plugin/Field/FieldFormatter

It would probably make a lot of sense to base our work on top of that (considering the age of the patch in this issue).

xjm’s picture

Title: Rich media display with <video> and <audio> HTML5 elements. » File field formatters for rich media display with <video> and <audio> HTML5 elements.
Assigned: travist » Unassigned
Status: Needs work » Active
Parent issue: » #2755061: [meta] Local audio and video media in entity content

We discussed this issue and the related problem space at length at Drupal Dev Days Milan. I've summarized that discussion in a parent issue: #2755061: [meta] Local audio and video media in entity content

For this issue, we decided to look into adding HTML5 field formatters for file fields as an intermediate step, probably in an experimental module. This would not completely solve the problem of playing media in content nor necessarily be a final architecture (see #2755061: [meta] Local audio and video media in entity content for a list of concerns and limitations), but it would provide an incremental improvement for site builders to use and experiment with.

Setting this issue back to "Active" since the previous patch is no longer applicable following all the changes to the entity system in Drupal 8 development. The code in #53 is probably a good place to start.

chr.fritsch’s picture

Status: Active » Needs work
FileSize
13.11 KB

Here is a first draft to support the audio files. Most of the functionality is moved from the file_entity module. The attached test is currently not working, but i have no idea why.

We should also discuss, if its ok to bring that directly to the file module, or should we open an experimental module to support all the new formatters.

Dave Reid’s picture

I would prefer to keep it as a field formatter in the file module.

xjm’s picture

Status: Needs work » Needs review

Thanks @chr.fritsch! Setting NR for the testbot to expose the test issue and run other tests as well.

Regarding whether to put it in File module or not, I agree with @Dave Reid that we should probably move it there eventually regardless. However, the limitation with that is that File is stable code, so we would have much stricter requirements and not be able to iterate on it. Whereas we can add it as experimental code in 8.2.x with the goal of migrating it back into File module in the future. (See: https://www.drupal.org/core/experimental. This is somewhat comparable to #2739075: [plan] Make Place Blocks module functionality part of the Block module (etc.) -- that functionality should obviously not be a standalone module in the long term.) :) So we would have a followup issue to move it before we consider it stable. However if there's limitations I've overlooked for that approach, let's discuss.

chr.fritsch’s picture

Ok, here is a new patch which includes the video formatter and 2 working tests

The last submitted patch, 55: file_field_formatters-1174892-55.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: file_field_formatters-1174892-58.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
30.89 KB

All tests should be fixed now

Berdir’s picture

Status: Needs review » Needs work

While I see how making something experimental is interesting from a maintenance point of view, but I think there are a few good arguments against that here.

This is more or less a direct port of the functionality that was in file_entity 7.x for years. It's not a new feature like big_pipe and I also wouldn't compare it to block_place. It's just two plugins, adding more features or making them available for more field types in a BC way should be possible and we always have the option to deprecate them and remove in 9.x.

Making it experimental would mean that users can't use it yet for at least another minor release (we haven't moved anything out of experimental yet and as @catch said in another issue, have no process for that yet). We'll also have to figure out what to do with the existing feature in file_entity (remove + upgrade path?) and making it experimental would I think mean that we couldn't remove it yet.

Quick review, mostly docs and coding standard things.

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,194 @@
    +/**
    + * @file
    + *  Contains Drupal\file\Plugin\Field\FieldFormatter\FileAudioFormatter
    + */
    

    no longer needed.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,194 @@
    +
    +  const applicableMimeType = 'audio';
    +
    

    const should be all caps afaik? and needs docs.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,194 @@
    +   *   The rendered service
    +   */
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, RendererInterface $renderer) {
    +    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
    +    $this->renderer = $renderer;
    +  }
    

    this is the same for both classes, lets move that into the base class?

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,194 @@
    +    /**
    +     * @var integer $delta
    +     * @var File $file
    +     */
    +    foreach ($this->getEntitiesToView($items, $langcode) as $delta => $file) {
    

    we don't do type hints for scalar and the second should then be a one-line comment with fully-qualified namespace.

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,50 @@
    + * @file
    + * Contains
    + */
    +
    +namespace Drupal\file\Plugin\Field\FieldFormatter;
    +
    +
    +use Drupal\Core\Field\FieldDefinitionInterface;
    +use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
    +
    +abstract class FileMediaFormatter extends FileFormatterBase {
    +
    +  const applicableMimeType = FALSE;
    

    more missing and incorrect docs.

    default value of FALSE seems a bit weird, espcially as it is not checked below. Might be better to make an abstract static function that subclasses are enforced to implement?

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,50 @@
    +  /**
    +   * Returns the first part of the mimetype of the file.
    +   *
    +   * @return string
    +   *   The mimetype.
    +   */
    +  public static function getMimeTypeType($mimeType) {
    

    missing docs for the param

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
30.72 KB
10.3 KB

Fixed some coding style issues

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative
Dave Reid’s picture

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
@@ -0,0 +1,186 @@
+    foreach ($this->getEntitiesToView($items, $langcode) as $delta => $file) {
+      $mimeTypeType = static::getMimeTypeType($file->getMimeType());
+
+      if ($mimeTypeType == 'audio') {

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
@@ -0,0 +1,225 @@
+    foreach ($this->getEntitiesToView($items, $langcode) as $delta => $file) {
+      $mimeTypeType = static::getMimeTypeType($file->getMimeType());
+      if ($mimeTypeType == 'video') {

We should abstract this code into an overridden FileMediaFormatter::getEntitiesToView() method.

chr.fritsch’s picture

I moved the complete block in a new method FileMediaFormatter::getSourceFiles. Code there was totally the same in audio and video. Additionally i moved __construct and create in the FileMediaFormatter too. Now audio and video formatter are much clearer.

webflo’s picture

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +    $summary[] = t('Controls: %controls', array('%controls' => $this->getSetting('controls') ? 'visible' : 'hidden'));
    

    Missing call to t()?

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,154 @@
    +
    +
    

    Double new line.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,154 @@
    +   * Constructs a FileAudioFormatter instance.
    

    Docs does not match the class name.

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,157 @@
    +
    +
    

    Double new line

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,157 @@
    +    $summary[] = t('Controls: %controls', array('%controls' => $this->getSetting('controls') ? 'visible' : 'hidden'));
    

    Missing call to t() for visible and hidden

  6. +++ b/core/modules/file/templates/file-audio.html.twig
    @@ -0,0 +1,22 @@
    +#}
    +<audio {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</audio>
    \ No newline at end of file
    

    Missing newline at end of file. Indentation in the template is not correct.

  7. +++ b/core/modules/file/templates/file-video.html.twig
    @@ -0,0 +1,22 @@
    +<video {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</video>
    \ No newline at end of file
    

    Indentation is not correct.

  8. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,145 @@
    + * @group field
    

    I think this test has the wrong @group. Should be file.

  9. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileVideoFormatterTest.php
    @@ -0,0 +1,145 @@
    + * @group field
    

    @group file

  10. +++ b/core/themes/classy/templates/field/file-audio.html.twig
    @@ -0,0 +1,23 @@
    +<audio {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</audio>
    

    Indentation

  11. +++ b/core/themes/classy/templates/field/file-video.html.twig
    @@ -0,0 +1,23 @@
    +{{ attach_library('classy/file') }}
    +<video {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</video>
    

    Indentation

  12. +++ b/core/themes/stable/templates/field/file-audio.html.twig
    @@ -0,0 +1,22 @@
    +<audio {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</audio>
    

    Indentation

  13. +++ b/core/themes/stable/templates/field/file-video.html.twig
    @@ -0,0 +1,22 @@
    +<video {{ attributes }}>
    +    {% for file in files %}
    +        <source {{ file.source_attributes }} />
    +    {% endfor %}
    +</video>
    

    Indentation

+++ b/core/modules/file/file.module
copy from core/modules/file/file.module
copy to core/modules/file/file.module.orig

Please remove file.module.orig

chr.fritsch’s picture

Here we go. All the comments are fixed. Switched from t() to this->t inside the Formatters.

Status: Needs review » Needs work

The last submitted patch, 69: file_field_formatters-1174892-69.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
28.67 KB
1.19 KB

Fix tests

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

keithm’s picture

#71 does not apply to 8.4.x, or 8.3.x at this time. This rerolled patch only changes a couple of lines to short-form array syntax.

I don't know how to create an interdiff when the original patch doesn't apply, so none supplied.

Version: 8.4.x-dev » 8.5.x-dev

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

chr.fritsch’s picture

Issue tags: +Media Initiative

Patch still applies to 8.5.x

phenaproxima’s picture

Issue tags: -D8Media

Re-tagging.

phenaproxima’s picture

Status: Needs review » Needs work

I really like what this patch is accomplishing. Super useful for supporting locally hosted video and audio, and a great boon to the Media Initiative. However, I found a not-insignificant number of issues. Nothing is architecturally wrong with this patch (yay!), but it needs a great deal of cleanup before it will be ready to commit.

  1. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -67,6 +67,49 @@ field.field_settings.file:
    +  label: 'File audio display format settings'
    

    Can this be "Audio file display format settings"? I think that will read more clearly.

  2. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -67,6 +67,49 @@ field.field_settings.file:
    +    controls:
    +      type: boolean
    +      label: 'Show audio controls'
    +    autoplay:
    +      type: boolean
    +      label: 'Autoplay'
    +    loop:
    +      type: boolean
    +      label: 'Loop'
    +    multiple_file_behavior:
    +      type: string
    +      label: 'Display of multiple files'
    

    Both the audio and video formatters share these controls. Can we create a base mapping for file_audio and file_video to descend from? Maybe we could call it file.formatter.rich_media.

  3. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -67,6 +67,49 @@ field.field_settings.file:
    +  label: 'File video display format settings'
    

    Likewise, can this be rephrased as "Video file display format settings"?

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +class FileAudioFormatter extends FileMediaFormatter implements ContainerFactoryPluginInterface {
    

    Why is this implementing ContainerFactoryPluginInterface? It is not being used.

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  const APPLICABLE_MIME_TYPE = 'audio';
    

    Constant inheritance makes me nervous, because it's easy to forget. Can this be an abstract static method on the base class instead?

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +    $element['controls'] = array(
    

    Coding standards dictate that we use [] syntax, not array().

  7. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +        'tags' => $this->t('Use multiple @tag tags, each with a single source.', array('@tag' => '<audio>')),
    

    Why is @tag a replaced variable? Can't we just put it directly in the string?

  8. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +    if (!empty($source_files)) {
    

    To keep this method a little "flatter", can we change this to:

    if (empty($source_files)) {
      return $elements; 
    }

    ...and then we can move the logic out a level?

  9. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,118 @@
    +      foreach ($source_files as $delta => $files) {
    

    $files is confusingly named. Won't each delta be associated with a single file?

  10. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +abstract class FileMediaFormatter extends FileFormatterBase {
    

    This should be implementing ContainerFactoryPluginInterface.

  11. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +   *   The rendered service.
    

    Should be "The renderer service" :)

  12. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +
    +    /** @var MimeTypeGuesserInterface $extensionMimeTypeGuesser */
    

    Nit: There is an extra blank line.

  13. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +    $extensionMimeTypeGuesser = \Drupal::service('file.mime_type.guesser.extension');
    

    For consistency, let's use $snake_case for variables, not $camelCase.

  14. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +    $extensionList = array_filter(explode(' ', $field_definition->getSetting('file_extensions')));
    

    It's preferable to use preg_split('/\s+/') instead of explode(), to account for variations in the white space.

  15. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +    foreach ($extensionList as $extension) {
    +
    +      $mimeType = $extensionMimeTypeGuesser->guess('fakedFile.' . $extension);
    

    Nit: There should not be an extra blank line here.

  16. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +    return parent::isApplicable($field_definition) && $applicable;
    

    It would be preferable to run parent::isApplicable() first, then return early if that comes back FALSE.

  17. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +  public static function getMimeTypeType($mimeType) {
    

    Does this need to be public?

  18. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +  protected function getSourceFiles(EntityReferenceFieldItemListInterface $items, $langcode) {
    +
    +    $source_files = [];
    

    Nit: Superfluous blank line.

  19. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,153 @@
    +    foreach ($this->getEntitiesToView($items, $langcode) as $file) {
    +
    +      if (static::getMimeTypeType($file->getMimeType()) == static::APPLICABLE_MIME_TYPE) {
    +
    +        $source_attributes = new Attribute();
    

    Nit: There are extra blank lines here that shouldn't be :)

  20. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +  public static function defaultSettings() {
    +
    +    return array(
    

    Nit: Extra blank line.

  21. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +    $element['controls'] = array(
    +      '#title' => $this->t('Show video controls'),
    +      '#type' => 'checkbox',
    +      '#default_value' => $this->getSetting('controls'),
    +    );
    +    $element['autoplay'] = array(
    +      '#title' => $this->t('Autoplay'),
    +      '#type' => 'checkbox',
    +      '#default_value' => $this->getSetting('autoplay'),
    +    );
    +    $element['loop'] = array(
    +      '#title' => $this->t('Loop'),
    +      '#type' => 'checkbox',
    +      '#default_value' => $this->getSetting('loop'),
    +    );
    

    Since this is shared with the audio formatter, these options should be part of the base class.

  22. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +    $element['multiple_file_behavior'] = array(
    +      '#title' => $this->t('Display of multiple files'),
    +      '#type' => 'radios',
    +      '#options' => array(
    +        'tags' => $this->t('Use multiple @tag tags, each with a single source.', array('@tag' => '<video>')),
    +        'sources' => $this->t('Use multiple sources within a single @tag tag.', array('@tag' => '<video>')),
    +      ),
    +      '#default_value' => $this->getSetting('multiple_file_behavior'),
    +    );
    

    Same with this one.

  23. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +    $summary[] = $this->t('Controls: %controls', array('%controls' => $this->getSetting('controls') ? $this->t('visible') : $this->t('hidden')));
    +    $summary[] = $this->t('Autoplay: %autoplay', array('%autoplay' => $this->getSetting('autoplay') ? $this->t('yes') : $this->t('no')));
    +    $summary[] = $this->t('Loop: %loop', array('%loop' => $this->getSetting('loop') ? $this->t('yes') : $this->t('no')));
    

    As with the settingsForm() method, these options are shared with the audio formatter and should therefore be part of the base class.

  24. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +    if (!empty($source_files)) {
    

    As with the audio formatter, let's switch to an early return here so we can flatten the method.

  25. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +      foreach (array('controls', 'autoplay', 'loop', 'muted') as $attribute) {
    +        if ($this->getSetting($attribute)) {
    +          $video_attributes->setAttribute($attribute, $attribute);
    +        }
    +      }
    

    Can this be put into the base class, maybe as a protected helper method, so we're not repeating this stuff for both the audio and video formatters?

  26. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,156 @@
    +        $elements[$delta] = array(
    +          '#theme' => 'file_video',
    +          '#attributes' => $video_attributes,
    +          '#files' => $files,
    +        );
    

    Some of this could probably also be moved into the base class.

  27. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,146 @@
    +  /**
    +   * The entity type.
    +   *
    +   * @var string
    +   */
    +  protected $entityType;
    +
    +  /**
    +   * The bundle type.
    +   *
    +   * @var string
    +   */
    +  protected $bundle;
    +
    +  /**
    +   * The field name.
    +   *
    +   * @var string
    +   */
    +  protected $fieldName;
    

    It's not clear why these need to be properties of the test class; they're only used in setUp() as far as I can tell.

  28. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,146 @@
    +  public function testRender() {
    +
    +    file_put_contents('public://file.mp3', str_repeat('t', 10));
    

    Nit: Extra blank line :)

  29. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,146 @@
    +    $expected_output = <<<EXPECTED
    +
    +  <div>
    +    <div>$this->fieldName</div>
    +          <div>
    +              <div><audio  controls="controls">
    +      <source  src="$src" type="audio/mpeg" />
    +  </audio>
    +</div>
    +              </div>
    +      </div>
    +
    +EXPECTED;
    +    $this->assertEquals($expected_output, $build[$this->fieldName]['#markup']);
    

    I don't think we should be asserting things this way; we're taking a lot of white space into account and such. It's kinda brittle.

    I would prefer if we turned this into a functional (browser) test. Then we could assert the presence of the <audio> element, and its attributes, in a much more stable way.

  30. +++ b/core/modules/file/tests/src/Kernel/Formatter/FileVideoFormatterTest.php
    @@ -0,0 +1,146 @@
    +class FileVideoFormatterTest extends KernelTestBase {
    

    Let's make the same modifications to this test as to the one for the audio formatter. Ideally the tests, like the formatters, can share a base class to do the boring set-up work, and differ only on the assertions.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
26.1 KB
21.53 KB

Thanks for the great review @phenaproxima.

I think i fixed everything beside 5, 29 and 30. I will take a look later to that, if no one else picks it up before.

Lets see what testbot says so far.

phenaproxima’s picture

Status: Needs review » Needs work

Thanks, @chr.fritsch. Substantially improved already!

  1. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -70,6 +70,41 @@ field.field_settings.file:
    +    controls:
    +      type: boolean
    +      label: 'Show audio controls'
    

    The label should probably be "Show controls", since this is no longer specific to audio. :)

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +
    +    if (!parent::isApplicable($field_definition)) {
    

    Extra blank line! My nemesis.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +    /** @var MimeTypeGuesserInterface $extension_mime_type_guesser */
    +    $extension_mime_type_guesser = \Drupal::service('file.mime_type.guesser.extension');
    +
    

    Seeing as how we're using ContainerFactoryPluginInterface now, can this be injected?

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +    foreach ($extensionList as $extension) {
    +      $mimeType = $extension_mime_type_guesser->guess('fakedFile.' . $extension);
    

    $extensionList and $mimeType should be $snake_case.

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +      if (static::getMimeTypeType($mimeType) == static::APPLICABLE_MIME_TYPE) {
    

    This logic is repeated a few times in this class. How about making it a helper method. Something like this:

    if (static::mimeTypeApplies($mime_type))

    mimeTypeApplies() could be an abstract protected static method, and then we can get rid of the APPLICABLE_MIME_TYPE constant.

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +      $elements[$delta] = array(
    

    Should use [], not array().

  7. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +   *   Additional attributes for preparing.
    

    This should be expanded upon a bit.

  8. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +   * @return Attribute
    

    Attribute needs to be fully namespaced, and this is missing a description.

  9. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +    foreach (array('controls', 'autoplay', 'loop') + $additional_attributes as $attribute) {
    

    array() --> []

  10. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatter.php
    @@ -0,0 +1,255 @@
    +          $source_files[] = array(
    

    Oops! Still some instances of array() syntax kicking around.

  11. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,103 @@
    +  protected function prepareAttributes(array $additional_attributes = []) {
    +
    +    $video_attributes = parent::prepareAttributes(['muted']);
    

    Extra blank line :)

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
26.55 KB
13.17 KB

Ok, next round of modifications.

Last missing thing is converting the tests to functional (browser) test. But TBH i'm not really sure of that. The current tests are actually checking the output. The functional tests wouldn't do more. And the KernelTests are super fast.

phenaproxima’s picture

Status: Needs review » Needs work

But TBH i'm not really sure of that. The current tests are actually checking the output. The functional tests wouldn't do more. And the KernelTests are super fast.

The current tests are asserting raw output, which is unreliable. If even one character of whitespace changes, the assertion will fail (and such a failure will be hard to debug). We are much better off asserting that actual elements and attributes exist, and that's something we can only do with a functional test.

It's true that functional tests are slower, but they're not THAT much slower. The Testing profile used by BrowserTestBase speeds things up *a lot*. A huge swath of core Media's tests are functional tests anyway. One possible way we could reduce the time is to add assertions for our new formatters to existing tests in the File module, if any suitable tests exist. But I've pretty much got my heart set on converting the tests to browser tests.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
25.29 KB
14.03 KB

Ok, fair enough. I converted the tests into BTB tests.

phenaproxima’s picture

Status: Needs review » Needs work

Looking fantastic. Very, very close now. Thanks, @chr.fritsch!

  1. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,46 @@
    +    $this->assertSession()->elementExists('css', 'audio');
    +    $this->assertSession()->elementAttributeContains('css', 'audio', 'controls', 'controls');
    

    We can assert this in one line:

    $this->assertSession()->elementExists('css', 'audio[controls="controls"]')

  2. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,46 @@
    +    $this->assertSession()->elementExists('css', 'audio > source');
    +    $this->assertSession()->elementAttributeContains('css', 'audio > source', 'src', file_create_url($file->getFileUri()));
    +    $this->assertSession()->elementAttributeContains('css', 'audio > source', 'type', 'audio/mpeg');
    

    Same here:

    $this->assertSession()->elementExists('css', "audio > source[src='$file_url'][type='audio/mpeg']")

  3. +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -0,0 +1,46 @@
    +    $this->assertSession()->elementExists('css', 'video');
    +    $this->assertSession()->elementAttributeContains('css', 'video', 'controls', 'controls');
    +
    +    $this->assertSession()->elementExists('css', 'video > source');
    +    $this->assertSession()->elementAttributeContains('css', 'video > source', 'src', file_create_url($file->getFileUri()));
    +    $this->assertSession()->elementAttributeContains('css', 'video > source', 'type', 'video/mp4');
    

    We could also streamline these assertions quite a bit, in a similar way to the audio ones.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
24.88 KB
2.13 KB

Makes totally sense. Thanks for all your reviews @phenaproxima

phenaproxima’s picture

+++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
@@ -35,12 +35,9 @@
+    $this->assertSession()->elementExists('css', 'audio[controls="controls"]');
+    $this->assertSession()->elementExists('css', "audio > source[src='$file_url'][type='audio/mpeg']");

Hehe, if we wanted to collapse even further, we could use selectors like this:

audio[controls='controls'] > source[src='$file_url'][type='audio/mpeg']

However, no need to go crazy-pants. I have taken enough of your time with my nitpicking! :)

I'll do a final, in-depth review of the patch later, once it passes Drupal CI, but in all honesty I think this is probably either RTBC, or very close to it.

The last submitted patch, 82: 1174892-82.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

Ok, the file url is weird on testbot. Let's try this

The last submitted patch, 84: 1174892-84.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 87: 1174892-87.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
25.13 KB
583 bytes

Damn it...

phenaproxima’s picture

Status: Needs review » Needs work

Fan. Damn. Tastic.

Just about everything I found is a nitpick. Several supernits. I have only a couple of truly legit concerns, both about the video formatter, and both should be pretty easy to resolve.

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,26 @@
    +  public static function getMimeType() {
    

    Does this need to be public? I can't really think of any circumstances under which external code might want to call it. Maybe we can remove FileMediaFormatterInterface and simply have this be an abstract protected method of FileMediaFormatterBase. Not sure what the best course of action here is; I don't have a strong opinion either way.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +      'autoplay' => FALSE,
    

    This makes me happy. I greatly dislike autoplay! :)

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +        'tags' => $this->t('Use multiple @tag tags, each with a single source.', ['@tag' => '<' . static::getMimeType() . '>']),
    

    I'm not sure how the markup system handles this, so I'll ask here: do we need to use the HTML entities for < and > in the replaced variable? Should this be '&amp;lt;' . static::getMimeType() . '&amp;gt;'?

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   *   Additional attributes for preparing the html tag attributes.
    

    Nit: "HTML" is an acronym, so it should be all caps.

    Also, can this sentence be rephrased? Maybe "Additional attributes to be applied to the HTML element." Additionally, we need to document the fact that these are boolean attributes -- you either have them on the tag, or you don't. So the type hint should probably be string[].

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   *   Container with all the attributes for the html tag.
    

    Nit: html --> HTML

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   * Check if given mimetype applies to formatter one.
    

    Nit: I think that mimetype is really supposed to be "MIME type". Not a big deal, though. But even the naming of getMimeType() suggests that "MIME type" is two distinct words.

  7. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   * @param string $mimeType
    

    Nit: We usually use snake_case in parameter names, so can this be $mime_type?

  8. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   *   Mimetype applies or not.
    

    Can this be rephrased? "TRUE if the MIME type applies, FALSE otherwise."

  9. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +    return ($type == static::getMimeType());
    

    Let us use === here.

  10. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +   * @return array
    +   *   List of files.
    

    Let's expand the description of this return value. Can it describe what each sub-array looks like (what keys they contain, and what types of values are there)?

  11. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +        $source_attributes = new Attribute();
    +        $source_attributes->setAttribute('src', file_create_url($file->getFileUri()));
    +        $source_attributes->setAttribute('type', $file->getMimeType());
    

    Nit: setAttribute() is chainable, so let's do that for readability.

  12. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,248 @@
    +        if ($multiple_file_behavior == 'tags') {
    

    Let's use === here.

  13. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,17 @@
    + * Defines getter and setter methods for FileMediaFormatters.
    

    There are no setters in this interface. Additionally, as per a previous point, I'm not certain we need this interface at all. If we do, then we should probably expand the doc comment on this interface to explain what it is used for and why you would want to implement it.

  14. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,17 @@
    + * @package Drupal\file\Plugin\Field\FieldFormatter
    

    I don't think we need this annotation.

  15. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,17 @@
    +   * The applicable mimetype for a formatter.
    

    Nit: "mimetype" should be "MIME type".

  16. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,102 @@
    +      'width' => NULL,
    +      'height' => NULL,
    

    Config schema defines these both as integers, so we should probably set them to a sane default. Maybe 640x480?

  17. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,102 @@
    +    $element['width'] = [
    +      '#type' => 'textfield',
    +      '#title' => $this->t('Width'),
    +      '#default_value' => $this->getSetting('width'),
    +      '#size' => 5,
    +      '#maxlength' => 5,
    +      '#field_suffix' => $this->t('pixels'),
    +    ];
    +    $element['height'] = [
    +      '#type' => 'textfield',
    +      '#title' => $this->t('Height'),
    +      '#default_value' => $this->getSetting('height'),
    +      '#size' => 5,
    +      '#maxlength' => 5,
    +      '#field_suffix' => $this->t('pixels'),
    +    ];
    

    We should probably add a validation method to ensure that width and height cannot be set to 0 or a negative number. In fact...if possible, can these use number fields, not text fields, to prevent people from entering garbage like "foobar" or "66.7%"?

  18. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,102 @@
    +    if ($width && $height) {
    +      $summary[] = $this->t('Size: %width x %height pixels', [
    +        '%width' => $this->getSetting('width'),
    +        '%height' => $this->getSetting('height'),
    +      ]);
    +    }
    

    If we set sane defaults in defaultSettings(), and add a validation method as previously suggested, we don't need the if statement anymore.

  19. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,102 @@
    +    if ($width && $height) {
    +      $video_attributes->setAttribute('width', $width);
    +      $video_attributes->setAttribute('height', $height);
    +    }
    

    Same here.

  20. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,43 @@
    +    $this->drupalGet($entity->url());
    

    Supernit: Isn't $entity->url() deprecated? Pretty sure the correct one is $entity->toUrl()...

  21. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +  /**
    +   * The field name.
    +   *
    +   * @var string
    +   */
    +  protected $fieldName;
    

    I'm not sure we need to store this as a class property. Maybe createMediaField() could simply return the field definition instead.

  22. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +  /**
    +   * The view display.
    +   *
    +   * @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface
    +   */
    +  protected $display;
    

    Does this need to be a class property? It doesn't appear to be used by any of the subclasses.

  23. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +  public static $modules = [
    

    I could be wrong, but I believe $modules is protected in the base class, so it should be protected here too.

  24. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +  protected function createMediaField($formatter, $file_extionsions) {
    +
    +    $entityType = $bundle = 'entity_test';
    

    $file_extensions is misspelled. Plus, a nit: $entityType should be $entity_type. And a supernit: there shouldn't be an extra blank line here.

  25. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +        'file_extensions' => $file_extionsions,
    

    Just to be safe, let's call trim() on $file_extensions.

  26. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,85 @@
    +    $this->display = entity_get_display('entity_test', 'entity_test', 'full');
    +    $this->display->setComponent($this->fieldName, [
    +      'type' => $formatter,
    +      'settings' => [],
    +    ]);
    +    $this->display->save();
    

    Nit: All the method calls after entity_get_display() can be chained.

  27. +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -0,0 +1,43 @@
    +    $this->drupalGet($entity->url());
    

    I think this should be $entity->toUrl().

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
25.08 KB
11.33 KB

#91.1: The method is used in isApplicable which is a static method. So it must be static as well and protected static functions are not allowed.
#91.3: No, it shouldn't. It's handled correctly in the current way. &lt; wouldn't be encoded.
#91.4-13: Fixed
#91.14-27: Fixed

chr.fritsch’s picture

Some minor improvements and i could get rid of the renderer service by adding the cachetags by myself.

phenaproxima’s picture

Status: Needs review » Needs work

Nice work, @chr.fritsch! Always lovely to dump a bunch of DI code.

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
@@ -213,7 +162,8 @@
+   *   Numeric based array, which again contains an associative array with the

"Numeric based" is strange terminology, can it say "Numerically indexed" instead?

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,202 @@
    +        $cache_tags = array_merge($cache_tags, $file['file']->getCacheTags());
    

    We should probably use \Drupal\Core\Cache\Cache::mergeTags() here.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,202 @@
    +    foreach (['controls', 'autoplay', 'loop'] + $additional_attributes as $attribute) {
    +      if ($this->getSetting($attribute)) {
    +        $attributes->setAttribute($attribute, $attribute);
    +      }
    +    }
    

    The way this is handled doesn't square with the documentation. $additional_attributes should be arbitrary key/value pairs, and 'controls', 'autoplay', and 'loop' should be defaults that get merged into it. I'm envisioning something like this:

    $default_attributes = [];
    foreach (['controls', 'autoplay', 'loop'] as $attribute) {
      if ($this->getSetting($attribute)) {
        $default_attributes[$attribute] = $attribute;
      }
    }
    $additional_attributes += $default_attributes;
    // Now call $attributes->setAttribute()...
    
  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,202 @@
    +   * @param string $mime_type
    +   *   The full mimeType.
    

    Nit: Description should be "The complete MIME type."

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,15 @@
    +/**
    + * Defines getter methods for FileMediaFormatterBase.
    + */
    +interface FileMediaFormatterInterface {
    

    We need to expand this doc comment and explain what this interface is for, and why a formatter would want to implement it.

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,15 @@
    +   * The applicable MIME type for a formatter.
    +   */
    +  public static function getMimeType();
    

    Needs @return documentation, and the description should start with "Gets the applicable MIME type..."

  6. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -0,0 +1,75 @@
    +    $fieldName = Unicode::strtolower($this->randomMachineName());
    

    Nit: $fieldName should be $field_name.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
23.85 KB
4.43 KB

Ok, fixed all the remaining comments form #94.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

All my concerns are addressed. Fasten seatbelts for landing.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

@Gábor Hojtsy pointed out in IRC that the issue summary here is very out of date. Let's fix that before going back to RTBC.

Gábor Hojtsy’s picture

Issue tags: +Needs change notice, +8.5.0 highlights

Tagging needs issue summary update because that is far from up to date. Also would be good to add a change notice on the new base class + functionality. Also adding to 8.5.0 highlights list (anticipating it would land). Only found these relatively minor things:

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,204 @@
    +    $summary[] = $this->t('Multiple files: %multiple', ['%multiple' => $this->getSetting('multiple_file_behavior')]);
    

    Great care was taken with the other options to display as human readable and translated. This would display the bit cryptic "tags" and "sources" strings always in English. Can this be made to be more human / translation friendly as well?

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,204 @@
    +  /**
    +   * Check if given MIME type applies to formatter one.
    +   *
    

    And formatter two and three? :) This may be correct English but it tripped me up a bit reading it. I guess it is the MIME type supported by this formatter or something.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileVideoFormatter.php
    @@ -0,0 +1,94 @@
    +  public function settingsForm(array $form, FormStateInterface $form_state) {
    +    return parent::settingsForm($form, $form_state) + [
    

    Would video controls be audio controls only? That is the label inherited for the controls setting.

phenaproxima’s picture

Great care was taken with the other options to display as human readable and translated. This would display the bit cryptic "tags" and "sources" strings always in English. Can this be made to be more human / translation friendly as well?

My recommendation for this: "tags" should be displayed as something like "Multiple <audio> tags" and "sources" should be "One <audio> tag with multiple sources".

chr.fritsch’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the issue summary.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
24.18 KB
1.86 KB

Adjusted a few things, based on the comments from #98 and #99

chr.fritsch’s picture

I started writing a change record in https://www.drupal.org/node/2922096. I am not fully satisfied with that. Maybe someone could help.

phenaproxima’s picture

Status: Needs review » Needs work

I'll tackle the change record.

Meanwhile, I think we should probably change a couple of things that I can see causing trouble in the future:

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -32,7 +32,7 @@
    +        '#title' => $this->t('Show @type controls', ['@type' => static::getMimeType()]),
    

    I'd prefer if we didn't rely on the MIME type when generating user-facing text. IMHO, we should leave the #title empty in the base class, and set it explicitly in both subclasses.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -87,7 +87,12 @@
    +    if ($this->getSetting('multiple_file_behavior') === 'tags') {
    +      $summary[] = $this->t('Multiple files: Multiple %tag tags', ['%tag' => '<' . static::getMimeType() . '>']);
    +    }
    +    elseif ($this->getSetting('multiple_file_behavior') === 'sources') {
    +      $summary[] = $this->t('Multiple files: One %tag tag with multiple sources', ['%tag' => '<' . static::getMimeType() . '>']);
    +    }
    

    Can this be a switch-case, so we don't need to call getSetting('multiple_file_behavior') twice? Also, I think this is another case where we can't necessarily rely on the MIME type being the same as the tag, so I think the best option is to have the subclasses create this summary line themselves, rather than try to generalize it here. We'll just have to copy and paste this logic into both the audio and video formatters.

chr.fritsch’s picture

What about just removing the type from the strings? Then we would have just "Show controls" and "Multiple files: Multiple HTML tags" / "Multiple files: One HTML tag with multiple sources"

IMHO thats totally fine and it prevents the complexity of overwriting.

phenaproxima’s picture

I love it. Let's do that!

One request -- can "Show controls" be "Show playback controls", for clarity?

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
24.04 KB
1.81 KB

Changing to proposal of #104, #105

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change notice

Updated the change record with a bit more info.

All of @Gábor Hojtsy's concerns in #98 are addressed, so I think we are done here. RTBC once Drupal CI passes it.

woprrr’s picture

Nice !! LGTM +1 :) Awesome works guys !

seanB’s picture

Just found some time to take a look at this. Mostly nits, but some of the things should probably be fixed.

  1. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -70,6 +70,41 @@ field.field_settings.file:
    +file.formatter.rich_media:
    ...
    +  label: 'Rich media display format settings'
    

    Is there a reason to add the word 'Rich' here? This doesn't seem to be used anywhere else?

  2. +++ b/core/modules/file/config/schema/file.schema.yml
    @@ -70,6 +70,41 @@ field.field_settings.file:
    +    multiple_file_behavior:
    

    The var is named 'multiple_file_behavior' while it's about display. Maybe just call it 'multiple_file_display' or even better 'multiple_file_display_type'?

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,26 @@
    +    return 'audio';
    

    The interface states this should return a mime type, but just 'audio' is not a mime type right?

  4. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,212 @@
    +    $summary[] = $this->t('Controls: %controls', ['%controls' => $this->getSetting('controls') ? $this->t('visible') : $this->t('hidden')]);
    

    Should this be 'Playback controls' as well?

  5. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,212 @@
    +        $summary[] = $this->t('Multiple files: Multiple HTML tags');
    ...
    +        $summary[] = $this->t('Multiple files: One HTML tag with multiple sources');
    

    'Multiple files:' > 'Multiple file display:'

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,212 @@
    +   * Check if given MIME type applies to the one of the formatter.
    ...
    +    return ($type === static::getMimeType());
    

    This is a little confusing to me. Maybe change it to: Check if given MIME type applies to the media type of the formatter.

    We should also change getMimeType() to getMediaType() since it doesn't actually return a mime type as mentioned before.

    This also seems to be in line with:
    http://www.iana.org/assignments/media-types/media-types.xhtml
    https://tools.ietf.org/html/rfc6838#page-10

  7. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterInterface.php
    @@ -0,0 +1,26 @@
    + * This interface is used on the FileMediaFormatterBase class to ensure that
    + * each file media formatter will be based on a MIME type.
    ...
    +   * Gets the applicable MIME type for a formatter.
    ...
    +   *   The MIME type of this formatter.
    ...
    +  public static function getMimeType();
    

    Again, it's actually a media type. Let's change that.

  8. +++ b/core/themes/classy/templates/field/file-video.html.twig
    --- /dev/null
    +++ b/core/themes/stable/templates/field/file-audio.html.twig
    
    +++ b/core/themes/stable/templates/field/file-audio.html.twig
    --- /dev/null
    +++ b/core/themes/stable/templates/field/file-video.html.twig
    

    These files are exactly the same as the templates in the module. Do we really need to place them in the stable theme as well? I might be missing something?

chr.fritsch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
24.04 KB
7.43 KB

Thanks for reviewing @seanB

I adjusted the patch based on the comments 1-7.

Regarding 8: I know we have a test that ensures that new template files are placed in stable and classy.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks! Didn't know about the template file test for the stable theme. Back to RTBC!

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -0,0 +1,26 @@
    + *   description = @Translation("Render the file using an HTML5 audio tag."),
    

    s/Render/Display/
    ?

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,210 @@
    +    return ($type === static::getMediaType());
    

    Nit: the parenteticals can be removed.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,210 @@
    +          ->setAttribute('src', file_create_url($file->getFileUri()))
    

    This should use file_url_transform_relative().

  4. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -0,0 +1,43 @@
    +    $file_url = file_url_transform_relative(file_create_url($file->getFileUri()));
    
    +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -0,0 +1,43 @@
    +    $file_url = file_url_transform_relative(file_create_url($file->getFileUri()));
    

    Ok so here we do have file_url_transform_relative()! Because this is by definition a subset of the full URL, this is passing tests.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
24.07 KB
3.68 KB

Fixed the comments from #112

Wim Leers’s picture

+++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
@@ -37,7 +37,7 @@
-    $this->assertSession()->elementExists('css', "audio > source[src*='$file_url'][type='audio/mpeg']");
+    $this->assertSession()->elementExists('css', "audio > source[src='$file_url'][type='audio/mpeg']");

+++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
@@ -37,7 +37,7 @@
-    $this->assertSession()->elementExists('css', "video > source[src*='$file_url'][type='video/mp4']");
+    $this->assertSession()->elementExists('css', "video > source[src='$file_url'][type='video/mp4']");

I don't understand why these had to be changed?

chr.fritsch’s picture

Since i added file_url_transform_relative in getSourceFiles, the file urls should be same now. Or am i wrong?

Didn't ran test, because i am fighting with my dev environment currently...

phenaproxima’s picture

I don't understand why these had to be changed?

For precision. Now that we are actually using file_url_transform_relative() when setting the src attribute(s), we can assert the src attribute exactly, rather than partially.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I somehow read those diffs in reverse.

That is obviously exactly what I hoped for :)

Sorry for my poor reading comprehension!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,210 @@
    +    list($type) = explode('/', $mime_type, 2);
    +    return $type === static::getMediaType();
    

    Are there potential uses where the mime type doesn't match the tag?

    I.e. we're using the same method getMediaType to check both the mime type and the tag.

    Should they be separate methods to allow for a mismatch?

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -0,0 +1,210 @@
    +        if ($this->getSetting('multiple_file_display_type') === 'tags') {
    ...
    +          $source_files[0][] = [
    +            'file' => $file,
    

    I don't see any tests for the multiple files/multiple or single tags.

    Can we get extra tests for this.

Looking good

phenaproxima’s picture

Issue tags: +Needs tests

Okay, tagging for additional tests.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
26.01 KB
8.61 KB

Regarding #118.1: Thanks for asking this question. I think potentially it's possible to implement a formatter for picture elements. The mime type for this formatter would then be 'image'. So i think it's a good idea to seperate the tag from the mime type.

#118.2: Added some more tests

phenaproxima’s picture

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

+1 for what @larowlan suggested in #118. I think it is a good idea, and thank you @chr.fritsch for implementing it. I found minor things, mostly nits! :)

  1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileAudioFormatter.php
    @@ -25,2 +25,9 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getHtmlTag() {
    +    return static::getMediaType();
    +  }
    

    Couldn't this just be moved into FileMediaFormatterBase, as is?

  2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -15,6 +15,14 @@
    +   * Gets the HTML tag for a formatter.
    

    Nit: "a formatter" should be "the formatter".

  3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php
    @@ -113,7 +121,7 @@
    -        '#theme' => 'file_' . static::getMediaType(),
    +        '#theme' => 'file_' . $this->getHtmlTag(),
    

    It seems to me that we should stick with static::getMediaType() here, rather than $this->getHtmlTag(). The reason I say this is because which HTML tag is used could actually be overridden by the theme template (although I can't imagine why you'd want to do that), whereas the media type is a lot more stable.

    Or, better yet, I think we should use something even less likely to change -- maybe the formatter's plugin ID? '#theme' => $this->getPluginId()

  4. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -13,21 +13,33 @@
    +  public function testRender($tag_count, $formatter_settings) {
    +
    +    $field_config = $this->createMediaField('file_audio', 'mp3', $formatter_settings);
    

    Nit: An extra blank line! =P

  5. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -35,9 +47,25 @@
    +    $this->assertSession()->elementsCount('css', 'audio[controls="controls"]', $tag_count);
    +    $this->assertSession()->elementExists('css', "audio > source[src='$file1_url'][type='audio/mpeg']");
    +    $this->assertSession()->elementExists('css', "audio > source[src='$file2_url'][type='audio/mpeg']");
    

    Nit: We can call $this->assertSession() once and re-use its return value.

  6. +++ b/core/modules/file/tests/src/Functional/Formatter/FileAudioFormatterTest.php
    @@ -35,9 +47,25 @@
    +  /**
    +   * Data provider for testRender.
    +   *
    +   * @return array
    +   *   Provided data.
    +   */
    +  public function dataProvider() {
    

    We should expand the @return documentation to describe what is actually returned in the array.

  7. +++ b/core/modules/file/tests/src/Functional/Formatter/FileMediaFormatterTestBase.php
    @@ -39,11 +39,13 @@
    +   * @param string[] $formatter_settings
    +   *   Settings for the formatter.
    

    string[] should be 'array', as we cannot be sure that all the settings are, in fact, strings.

  8. +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -13,21 +13,33 @@
    +  public function testRender($tag_count, $formatter_settings) {
    +
    +    $field_config = $this->createMediaField('file_video', 'mp4', $formatter_settings);
    

    Nit: Extra blank line.

  9. +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -35,9 +47,25 @@
    +    $this->assertSession()->elementsCount('css', 'video[controls="controls"]', $tag_count);
    +    $this->assertSession()->elementExists('css', "video > source[src='$file1_url'][type='video/mp4']");
    +    $this->assertSession()->elementExists('css', "video > source[src='$file2_url'][type='video/mp4']");
    

    Nit: Can we collapse the assertSession() calls here by reusing its return value?

  10. +++ b/core/modules/file/tests/src/Functional/Formatter/FileVideoFormatterTest.php
    @@ -35,9 +47,25 @@
    +   * @return array
    +   *   Provided data.
    +   */
    +  public function dataProvider() {
    

    As above, we should document the returned array more thoroughly.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
25.76 KB
6.41 KB

Thanks for reviewing continously, @phenaproxima :)

I like the idea to use the pluginId for the template name.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think both my and @larowlan's concerns are addressed by the patch in #122. Back to RTBC once Drupal CI passes it.

webchick’s picture

Ok, ran this patch through its paces:

1) Added a new field to Article called "Videoz," allowing m4v and mov files.
2) Added a second field called "Audioz," allowing mp3 and m4a files.
3) Changed the fields under Manage display to display Video and Audio respectively:

Manage display

4) Created a piece of content, and tested both multivalue and single value fields.

Works! Here's Chrome:

Chrome

Here's Safari:

Safari

In the case of an valid but non-sensical extension ('txt'), just no tag is displayed, which is nice. Doesn't appear to be test coverage for this, so maybe a follow-up to add it.

Otherwise, nothing to complain about here. Looks like @larowlan's feedback was addressed. This is a great user-facing addition to Drupal so LET'S DO IT!!

Committed and pushed to 8.5.x. WOOHOO! :D

  • webchick committed 8f300f5 on 8.5.x
    Issue #1174892 by chr.fritsch, travist, Dave Reid, keithm, webchick,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
chr.fritsch’s picture

Thank you very much everyone, I am very happy that this is in now.

One question: The CR is still unpublished. Should I publish it or will one of the committer do it?

larowlan’s picture

Thanks, published the change record

Status: Fixed » Closed (fixed)

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

Liliplanet’s picture

Was wondering, please how do I change width and height of video pixels to % percentage? Needs videos to display responsive.