Add credit for this issue to the following contributors just prior to commit: https://www.drupal.org/files/issues/contributors.txt
(Not yet since we are trying to keep this issue on one page. There are 66 names, so this probably does not require Infrastructure intervention.)
In past few months (since Dublin) we reached the agreement to bring the contributed Media Entity module to core (under the name Media), which will allow us to implement better and more powerful media handling.
For more info see #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core, #2825215: Media initiative: Roadmap and #2786785: Media in Drupal 8 Initiative.
This is blocker for all issues that we'll be focusing on at Media sprint in Berlin (starts on December 12th 2016). Marking as major for that reason.
Bring Media entity module in core mostly as it is. Any changes made will need upgrade paths for the existing module users.
Remove integrations with few contrib modules: Inline entity form, Devel Generate and Plugin. We will open follow-up issues in those modules after this lands.
Media entity is stable and extensively used and there are no urgent changes planned for it. Other steps, which will happen after and not directly change Media entity are explained in #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core.
There are two mapping things for followups:
The UI. kind of confusing right now. Roy proposed a few solutions and there is a solution for it. https://www.drupal.org/node/2836153 Needs IS update
https://www.drupal.org/node/2862467
media_entity_copy_icons() (see #366 and #367).Adds new fieldable and revisionable entity type.
Moved to the summary of #2825215: Media initiative: Roadmap.
| Comment | File | Size | Author |
|---|---|---|---|
| #391 | interdiff-2831274-389-381.txt | 27.83 KB | phenaproxima |
| #391 | 2831274-391.patch | 234.57 KB | phenaproxima |
| #389 | Screen Shot 2017-04-24 at 3.52.44 PM.png | 45.98 KB | naveenvalecha |
| #389 | interdiff-2831274-388-389.txt | 2.62 KB | naveenvalecha |
| #389 | 2831274-389.patch | 255.64 KB | naveenvalecha |
Comments
Comment #2
slashrsm commentedAttached patch adds Media entity 8.x-1.6 with the changes described in the issue summary. It currently re-implements own revision form logic.
Comment #11
berdirthe config entity type defines third party settings, id and label, so you don't need those.
this can go :)
this too then
not sure about update functions, but I think we shouldn't take them over? Especially since this is actually mysql specific and something that would be allowed in core.
We could include a hook_update_last_removed() maybe to ensure that people have updated to the latest version before updating to 8.3.x?
I fear the use of bundle in the UI is going to be a problem for core, which tries to avoid that *very hard*.
we'll probably need to update the help?
@throws needs a description.
The names are a bit strange, only got the non-all permissions after seeing the code below. at least the labels should probably be changed to * own media?
I think we have that in core now.. entity-list-add. highly unlikely that someone customized this, so might be ok to drop this.. especially since it is backend and backend UI is excluded from BC afaik.
sanitize doesn't exist anymore, token should always return the unsanitzed version.
https://www.drupal.org/node/2789315 is close and we should be able to switch that when it lands.
this is provided by EntityChangedTrait
Using published means this isn't compatible with EntityOwnerInterface, maybe deprecate these methods or at least add the others too?
we should implement RevisionLogInterface if we don't already and uset setRevisionLogUid() like Node does.
do we have a getter for field_map?
clearly that did't happen so it will also not happen after a stable release, I'd remove the @todo, otherwise you'll have to create an issue for it :)
should we register a usage for the thumbnail?
Todo needs proper formatting and ilikely an issue.. or get removed :)
we should call the parent and let it generate the standard fields.
awww.. using vid is a sad copy & paste from node that we can't change now :)
as mentioned earlier, we should be able to replace this with the new trait+interface.
ah, you actually have th methods, just don't use the trait yet..
this is the same as the parent implementation.
I think the interface in the other issue documents this as shouldCreateNewRevision() or so.
hidden node.module dependency?
Yet another thing I'd rather not see copied from node module :)
See #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button and #1901216: Modules cannot reliably enhance or alter the node form anymore
another node.module thing. if saving fails, we throw an exception. this else case will *never* run. if you would want to handle that it would need to be in a try/catch. Or drop it.
docblock > 80 chars.
what's the custom class for if it doesn't do anything?
.. and the interface?
why extend from PluginBase in Component and not Core which includes StringTranslationTrait and DependencySerializationTrait?
this means that any new createInstance() call needs to load that config, but we probably only need it on very few cases' It is in general better to inject factories and only load things that you need when you actually do and avoid logic/code during create()/construct()
isn't the bulk form generic now, what's this for exactly, just copied from NOde?
what's the point of this operation exactly? Node has it too and obviously copied from there but for what exactly? :)
wouldn't this fail if you select "original" ?
not sure why we do this like this and don't just call a method that does the logic and then save?
not really that useful to override but OK :)
that seems copied from node and not really adjusted?
Comment #14
slashrsm commentedThank you!
Done: 2, 3, 4, 7, 8, 9, 10, 12, 13, 14, 15, 16, 19, 23, 24, 25, 27, 28, 29, 30, 31, 35, 36, 38
1: Removed third_party_settings. Was looking for id and label on config_object but couldn't find them. Are you sure they are provided there?
5: I understand the concern. We can't use "type" as we use this for something else already. The idea when we were originally designing this was that the type better relates to the type of asset (local image, YT video, Tweet, ...), while bundle is a bunch of fields and some configuration attached to a given type. "Sub-type" was another suggestion back then. Would love to hear more ideas. I am also concerned how much confusion would changing this in UI and not in the backend cause.
6: Didn't touch this. Still needs work.
17: Image field registers it already.
18: I would really like to remove the @TODO, but I really think that "Thumbnail" for alt isn't the best solution. I am not sure which solution is best here. Probably, as comment already directs, some token-based approach which is configurable somewhere and falls back to "Thumbnail" if empty. Ideas?
20: Yes :(
14., 21., 22.: We do implement the RevisionLogInterface since we have different field names. We already tried to convert to RevisionableContentEntityBase in #2712135: Update media entity to 8.1.x functionality but miserably failed. One of the blockers seems to be #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table.
26: That issue if far from straightforward. Seems that there is no consensus there yet. Should we go with the checkbox or wait to see what is decided there and be consistent?
32. Done, but literary every plugin implementation in contrib uses this. We will need to update all of them. Could we add magic getter to keep existing plugins working until they catch up?
33 and 37. Yes :) Node and User do this exactly like that. I removed the override and now we're depending on generic implementation.
34. Yes. Another node copy-paste. Someone could technically use it and removing it would break that. But that is also very unlikely. Not sure.
Comment #15
gábor hojtsyReviewed and discussed the UI of this module on the the Drupal Usability meeting today, see https://twitter.com/drupalux/status/803710630729486336 for recording.
Comment #16
berdir1. You're right, id/label isn't there. Always get confused about which things are defined there by default vs. having some default logic in config entity. Would be too easy if that were in sync :)
5. Yeah, I don't have a suggestion either. I think sub-type isn't better. We'll see what others have to say. And yes, I think content (type) vs node (type) is at least as confusing as using "bundle".
14 and so on: Ah yes, that issue. Hopefully it will be resolved soon ;)
26. Not sure. Personally, I'd say yes, we actually use that patch in our projects as HEAD is a huge pain for allowing non-admins to control status. But that's definitely not my decision to make, so lets wait for more feedback.
32. Fine with keeping it for BC, we could also inject both the factory and the config and deprecate the config? But that would also be a constructor change which will break all plugins that have their own injection. that pattern is so annoying for base classes :)
34. Nope, not unlikely at all. Every existing installation has the config, so the least we'd have to do is write an update function. And assuming that views actually defines config dependencies on actions (not sure about that), we might end up deleting views too, which wouldn't be cool. So, lets keep it.
Just one more nitpick from the interdiff:
Comment #18
slashrsm commentedWatched recording that @Gábor Hojtsy linked in #15. My main comments.
Thumbnail
Thumbnail is set first time and then not touched again. There are requests to change that, but in case of remote media it might introduce a huge performance regression if we would fetch thumbnail every time an entity is saved.
One idea was to add a checkbox that would force thumbnail to be re-calculated on save. Having input about from UX people about this would definitely help achieve a decision.
Source field
The fact that you need to create bundle, create field, return to bundle and set source field is obviously annoying. We are working on a patch that would auto-generate necessary field: #2625854: Provide default source_field when creating new media entity bundles. Would that improve the situation?
Default media listing
We should completely remove that and replace it with the media library that is planned for core in my opinion.
Missing media type plugins
It is definitely plan to ship with more type plugins (as proposed in the planned issue: #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core). I would think at least image, document and YouTube. The reason why they are not included in this patch is to not make it even bigger.
Media bundle form
It would be great if UX team would propose how to improve issues there (things not being refreshed, etc.). This is first time anyone checked this form from the UX perspective, which is extremely valuable.
Comment #19
slashrsm commentedFixed nit and two other nits that should pass more tests.
Comment #21
slashrsm commented#2796901: Convert all web tests to BTB where possible, increase test coverage and split existing tests into more granular classes. landed and I think that it makes more sense to include that now than wait. Huge interdiff, but main patch stays more or less the same. And all web tests are gone so we don't have to deal with them later.
Comment #24
chr.fritschThumbnail
We definitely need to update the thumbnail. A checkbox to force this is one option. What about checking of the source_field? If the value changed there, than fetching a new thumbnail.
Or we add an action somewhere to re-fetch the thumbnail.
Source field
IMHO, integration of #2625854: Provide default source_field when creating new media entity bundles would improve the situation.
Default media listing
I agree and will provide a patch later this day for that.
Missing media type plugins
After media entity is into core, we should open follow-ups to get media_entity_image into core, too. For video support we should think about if we can grab something from video_embed_field module. It ships integration with lots of remote video providers.
Comment #25
chr.fritschHere is a new patch
Renaming media_entity into media
One think i would like to discuss is, should we rename media_entity into media. I would say, yes, to follow core pattern. The problem is we already have in contrib a module called media. But the D8 usage of that is really low https://www.drupal.org/project/usage/media
Comment #27
phenaproximaI've always thought that the term "Media" was a bit too generic and overloaded. If I had my druthers, I would prefer that we rename Media Entity to one of Multimedia, Multimedia API, or Media API.
Comment #28
pixelmord commentedHaving watched the review of the current state in the UX meeting and having used the module(s) in contrib I have to say that the mentioned requirement that media providers shall take care of creating the needed field(s) on the bundle makes sense #2625854: Provide default source_field when creating new media entity bundles.
However I think then still the process/user journey is backwards, because you kind of create a generic bundle and then select a (possibly generic) provider and then the provider might bring configuration options into the form (after reloading the form). From a UX perspective a user wants to create a media of type XXXXX right away, e.g. a youtube or image media and does not care that this concept is divided in a provider and a bundle by the API.
If we had the option to use a multi-step form I would suggest beginning with the selection of the provider. Since we do not have that in core, I would suggest starting the form with a label (for the bundle) and the selection of the provider and not set the generic provider by default. After the selection of the desired provider, even if it is the generic one the form can show the fields specific to that provider and then eventually the form can be submitted.
That way the user performs the necessary actions in an order that feels a bit more natural. At the moment the provider selection is not visually promoted to be important because it is somewhere down the list of fields and fieldsets...
Comment #29
gábor hojtsy@phenaproxima: the problem with calling it "X API" is it is not an API module, it manages the UI too. Like Views has a UI module and an API module and there is no UI if you don't enable the UI module, but in this case, the module both provides a base API and a UI with place for plugins. That is why also the current contrib module is confusingly named in the human readable name as "Media Entity API".
Comment #30
phenaproximaWhy don't we split them apart, then? I can definitely see use cases where you might want to support media entities, but provide your own UI from top to bottom.
Comment #31
slashrsm commentedMedia bundle form (re #28)
Yes! I think that multi-step form or at least multiple screens could solve a lot of UX problems there if done right. Field mapping is definitely a good candidate for a separate screen in my opinion. But I also don't want to be too smart here and let UX experts have their say.
The question is how to approach this. Do the total overhaul as part of this patch? Try to fix lowest hanging fruit here and then do bigger changes in a follow-up? I would slightly prefer second option as it would allow us to keep moving faster.
Name of the module
I understand the concerns and I am aware that there are better names than media_entity. I would only suggest to consider amount of effort needed for a serious renaming of the module and potential update paths for existing sites that would be required. Compare this to the benefits that we get from that and make sure we get best ROI for the resources that we're investing into this.
Comment #32
dawehnerThis is not a proper review, I was just bored and read a bit of the patch.
This one misses a
cachePerPermissionsIt is weird that we add deprecated method right from the start
This test should use assertJsCondition instead of just waiting, see #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly
Comment #33
slashrsm commentedThis should fix the last failing test.
EDIT: Cross-post. Items from #32 are not addressed in this patch.
Comment #34
chr.fritschI fixed everything @dawehner addressed in #32
Comment #35
slashrsm commented#32-2 is true, but we have to be aware that this entity type exists and there are thousands of sites using it already. Those sites might have custom code relying on this functions. I don't think that we should remove them. This was done based on #11-13.
Comment #36
dawehner@slashrsm
Fair point, and yes, I actually agree.
Comment #37
chr.fritschOk, the deprecated functions are back.
Made the interdiff from comment #33
Maybe if there comes a decision to rename the module, then we can think about removing those functions again.
Comment #38
chr.fritschIs there a reason why all these properties are public?
Comment #39
phenaproximaStrange wording. Can this be something like "URI where media type icons will be installed"?
This looks quite similar to the core image formatter. Can we re-use its config schema?
Should be using $context.find().
This too.
...and this.
$types should be type hinted.
drupal.form depends on drupal and jquery, so we can probably remove those dependencies.
If this is going into core, we probably should not be linking to a contrib page :)
Should we mark this function @internal? I don't see why it would ever be used under normal circumstances...
$variables should be type hinted.
This should also be able to handle FieldItemListInterface.
It'd be nice to have singular_label and plural_label defined. I suggest "media item" and "media items", respectively.
Node type forms? Not so much...
Might be preferable to use $this->getEntity() here.
It might be more prudent to use $this->getEntity() here.
Same here.
Comment #40
tim.plunkett$form['#entity'] = $bundle = $this->entity;Please remove the $form['#entity'] part.
Comment #41
tstoecklerWow, this is great to see being pushed into core. Maybe we will have Media management in core in 2017. That's still ahead of the curve, right? No one else is doing that yet, right? Right? Right? (Sorry, couldn't resist. Really awesome that you are pursuing this so vigorously, though, this is genuinely awesome! Thank you! <3)
Reviewed the entire patch but for the test coverage, however just from the names of the test classes, it seems the test coverage is quite extensive, nice work!
I like the architecture, and it's good to see that a lof of other issues have now fallen into that we do not have to duplicate so much work. Although of course at the same time it reveals a lot of places where we do need a lot more generalization. One issue in particular from looking at the patch is that I think we should open a parallel issue (or I guess there must be one already) for generic entity actions. There really is nothing media-specific in those action plugins. It would be unfair to block this on that, though.
The one larger problem I have architecturally is the field mappings. I think it would be nicer if the media type plugins could provide field definitions and then those fields would be added automatically to the entity. It seems A) like a waste of time and B) error-prone to make people go and configure the same fields over and over again in the UI, when the plugin knows exactly what it wants already. This is possible in Entity API in contrib, and I think is a pattern we should look at here as well. That's the only major quibble with the patch, the architecture with the plugins and bundles looks solid although it does show that we should really have better integration between Entity API and Plugin API to make this easier (again: not the fault of this patch).
Yay!
Some notes on the actual patch:
I know this is nitpicking, but it would be nice to have these directly one after the other
This is fairly problematic IMHO, I think there is a lot of valid use-cases of building drupal sites without Views including media ones.
Is config/optional for all the views not an option?
Let's remove those. See #2808481: Introduce generic entity template with standard preprocess and theme suggestions for more background on the different suggestions for suggestions (;-)) but removing the ID-specific ones is pretty uncontroversial I think.
This should now no longer be necessary if you add a collection link and an admin permission to the entity definition
Sorry for the nitpick: Single quotes please.
The (optional) is not a thing for @var declarations as far as I'm aware. I think we can just drop it.
Should be
\Drupal\Core\Field\FieldItemInterface|stringAdding a trait with only a single method that excepts either a string or a field item is very suspicious to me, but OK...
This should be
Let's add a bunch of labels (plural / singular / count)
Per the above, let's add a "collection" link and then we can drop the collection route.
This should use
EntityPublishedTraitinstead.I think, if we're using EntityOwnerInterface, this should be using $this->getOwner() internally. Or the other way around. But we should be duplicating the actual field name and so forth here, IMO.
Same for getPublisherId() vs. getOwnerId() and setPublisherId() vs. setOwnerId()
"... if no label is provided." would sound better IMO. Also the comma is not necessary as far as I remember.
Minor, but you can just replace
empty($this->label())with!$this->label()I think this is worth a comment.
Also I think this makes it clear the
getQueueThumbnailDownloads()is a bad method name. It should be something likequeueThumbnailDownloads()or similar.Also what happens if the entity is saved for the first time and then very quickly saved again, before the queue has been processed? Or do we not need to care about that?
Let's put $file_storage into a separate variable and the we can do $file_storage->getQuery() instead of \Drupal::entityQuery('file')
If we're calling this $publisher then let's also call the getPublisher() method.
Above there is a fallback in case $this->label() is not set. But this is called before that, so that seems problematic.
I think this should be removed here and discussed for ContentEntityBase in a separate issue. It totally makes sense and is actually arguably a bug report that you can "delete" the revision log for existing nodes, for example.
Shouldn't the constraints be attached in getConstraints()? Maybe someone wants to use the constraints for something other than the validate() method.
I know it's pretty nitpicky but the order of these (form vs. view display options) is inconsistent among a bunch of these fields.
Are you sure this is a good idea? This runs into the whole page title vs. entity label issue which we really haven't solved properly yet. See #2353867: [META] Expose Title and other base fields in Manage Display for some background on this. I think for now it would be better to leave this out.
Can we use a larger difference here (like 5) so people have a better chance of fitting in between.
This should use
RevisionLogTraitinstead.It seems like something is broken if a media's bundle points to a non-existing bundle entity. So instead of
MediaBundle::getLabel($media)you should always be able to do$media->bundle->entity->label(). If you think it's annoying to do$media->bundle-entitysee you in #2699835: Add a method to ContentEntityBase for getting its Bundle entity.Or even in #969180: Add a convenience method for getting the label of a bundle which would obsolete this label method entirely.
So I think this method should be dropped.
Why is this needed? If it is for an 'exists' callback of machine name element, you can just use load() directly.
Minor, but it's nice to use @count instead of 1 even in the singular case, because in some languages singular != 1
I think this should use ->set() consistently
Should this not happen in buildEntity()?
Why is this is needed, this seems pretty non-standard?
This seems to have nothing to do with media, can we add something like this to \Drupal\Core\Form.
Why "edit"? (and not "update")
I think we should have this. I think instead ConfigurablePluginInterface should be optional and then MediaBundleForm should check for that and only conditionally call buildConfigurationForm() that's also the pattern used for e.g. Image toolkits or search pages in core.
Typo in the filename: Should be uppercase "t".
This probably needs some handling for the label/title for the full-page/non-full-page scenario. See node module or #2808481: Introduce generic entity template with standard preprocess and theme suggestions for more information.
As said above, I think this should be in config/optional
Comment #42
tstoecklerSome more notes.
I looked into 21. of #41 and I guess the reason you are not adding the constraints generically is because you need the entity object? But what is that use-case? That seems suspect to me.
It seems this is the only place that function is being used. If so, let's inline it please.
Seems this could actually use the generic thumbnail by default and then
Generic::thumbnail()could internally call$this->getDefaultThumbnail(), which seems like a reasonable bas implementation anyway, right?Also a get* Prefix would be nice here.
These should consistently use the get* prefix. Also I personally would find it more self-documenting to call them get*ThumbnailUri(), but I can see how that would make the function name pretty long, so feel free to ignore this.
Comment #43
gábor hojtsyThe mapping UI came up as a UX problem too. First that you need to add all those fields and second to do the mappings. @slashrsm explained that the idea was that solution/feature modules would ship with the entity bundles with respective fields, so in practice installing the media module or lightning or np8 distro, etc. would get you a preconfigured set. But with this being in core that situation is different because then its not just a fundamental lego block but it should actually make users' life simpler too :)
Comment #44
berdirWe could find a way to define that as an optional, non-BC breaking step? There could be a button "Create default fields" above the mapping that a plugin can support with an additional new interface? or even in the same, with an empty default method (then we can't conditionally show the button, though)
Comment #45
tstoecklerBut why even make it an empty step? Why not just do it in the first place?
Comment #46
berdirTo give you a chance to do it different?
If each plugin defines its own field, then each needs to provide unique, prefixed field storages, so that they don't conflict. Otherwise twitter and youtube might define a field with the same name that has a different type.
However, the whole point of having the field mapping is that you can re-use the same field across different bundles, so that it helps you simplify theming/views configuration.
Comment #47
chr.fritschWorked on the comments in #39
1. Fixed
2. We can open a follow up to generalize that
3., 4., 5. Fixed
6. Fixed
7. Fixed
8. We can fix it later, when we have a project page
9. Not sure, other media modules can use that in their install function, too.
10. Fixed
11. Needs work
12. Fixed
13. Fixed
14. Fixed
15. Fixed
Comment #49
slashrsm commentedThank you all for all the hard work, nice words and valuable input!
I feel that the metadata mapping part causes a bit of a confusion. Let me try to explain the reasoning behind it and its architecture. In order to do that we have to go back into the distant (D7 :)) past....
Media solutions in D7 were problematic in many ways, but they were useful. We were able to achieve many different things with them, but there was at least one feature that was nonexistent. Metadata handling (specially useful and important when you are dealing with remote media). There were a lot of modules that were providing support for various providers. Some of them even provided some metadata, but the APIs were not consistent. Every module would have its way of doing things. For example, if your site hosted YouTube and Vimeo videos and you wanted to display their titles and captions with them you most likely needed to implement two totally separate ways of getting that information in your theme. One per provider.
Idea of the metadata handling in Media entity was to standardize that. And it works on two levels. First level is API on media type plugins that allows for standardized discovery and retrieval of metadata information that a given type plugin provides. This API can be used by developers to access and use that information. This API was there way before mapping thing even existed. You can use this information to fill data into the themeing layer completely bypassing Entity API fields. Sometimes this can be a good idea and sometimes not. It depends....
Problems are with fields that we fetch from remote APIs since their retrieval is a slow process. Those you generally want to store locally to save many slow API calls. Cache is one option. If you aggressively cache rendered representation of the media it might be enough in some cases. And the other options are entity fields.
Only at this point we've reached 2nd level. We have this immensely powerful data storage layer in Drupal and it would be shame not to offer out-of the box, site-builder ready integration with our metadata api. Mapping is born. Tada! There are two main reasons why mapping is not enabled/defined by default: a) Types can provide *many* fields and it is unlikely we will need all of them all the time and b) we would need to auto-create many fields for them, which would be huge overhead IMO.
Solution to this problem is quite simple in my opinion:
1. UX team gives input on mapping form and we improve it. Maybe we move it on a separate screen. We make it smarter so it detects if there are no fields to map to, dynamically update when things change, ... This is an easy task assuming someone with UX/interaction design experience leads the way (we would do that in contrib already, but it seems that relevant people became interested in this only now).
2. Default bundles () that we create provide limited level of mapping that makes sense for 80% use case. This means 2 things: a) 80% of people won't need to touch this at all b) default configuration will act as an example. Relevant issues from parent meta: #2831936: Add "File" MediaSource plugin, #2831937: Add "Image" MediaSource plugin and #2831944: Implement media source plugin for remote video via oEmbed.
Comment #51
ekes commentedIt was suggested I put this use case in here, I didn't quite think it was the issue, but now I see the metadata discussion I understand:
I'm presently looking at the metadata extraction for indexing (via search api). First instance is the obvious pdf text, but then there's audio track information (even lyrics), exif data location. These are made available depending on the extractors installed (tika is in solr, but there could be any number of other external programs, services, as well as things like exif_* commands being available in php). They are exposed as additional properties that are not actually on the entity that search api can index.
It seemed to make sense to make these available to the site elsewhere. A tagged service was suggested.
So media could offer up the additional potential fields for mapping if there is a service installed for the media type(s). I was thinking that media would just call all tagged services for the potential mime type supported by the field; but it's not defined anywhere what mime types each media entity could support (*, audio/* etc). Or maybe it is only the work of the individual providers?
Comment #52
amateescu commented#2789315: Create EntityPublishedInterface and use for Node and Comment got committed recently so here's an updated patch which makes proper use of it :)
Comment #54
jibranSome minor observations.
Do we need this in core?
Why can't we use setDefaultValueCallback for r_uid basefields?
We can use class constant here.
Comment #55
marcoscanoI'm not sure whether this saves anyone some time or only adds confusion (sorry if it's the case), but I have advanced some quickfixes from review #41:
41-1) Fixed
41-2) Needs work
41-3) Fixed
41-4) Fixed
41-5) Fixed
41-6) Fixed
41-7) Fixed
41-8) Needs work
41-9) Fixed
41-10) Fixed
41-11) Was this refering to MediaBundle.php instead of Media.php? The collection link is present in the first case, and there is no route 41-for the second (it's a view)
41-12) Fixed in #52
41-13) Needs work
41-14) Fixed
41-15) Fixed
41-16) Needs work
41-17) Fixed
41-18) Needs work
41-19) Needs work
41-20) Needs work
41-21) Needs work
41-22) Fixed
41-23) Needs work
41-24) Fixed
41-25) Needs work (might lead to some BC issues?)
41-26) Needs work
41-27) Needs work
41-28) Fixed
41-29) Fixed
41-30) Needs work
41-31) Needs work
41-32) Needs work
41-33) Needs work
41-34) Needs work
41-35) Fixed
41-36) Needs work. (the solution implemented here will probably also solve #2754277)
41-37) Needs work
Comment #58
chr.fritsch41-2) Since we remove the media view because it will be replaced with the library, i think its fine when we remove the dependency
41-8) Needs still work
41-13) Fixed
41-16) Added a comment, not sure about renaming the function, because we want to try not to change the APIs
41-18) Renamed variable to owner, we want to use methods from EntityOwnerInterface
41-19) Setting the thumbnail is now the last action
41-20) Needs follow up
41-21) Needs still work
41-23) As suggested, lets leave it out
41-30) Needs work
41-31) Needs work
41-32) Needs work
41-33) Needs work
41-34) Needs work
41-36) Needs work
41-37) This is just for testing purpose and test will not work without. So i think in config/install is fine
39-11, #42 and #54 still needs to be addressed
Comment #59
slashrsm commented#41-8: This trait is used just by two contrib plugins (Twitter and Instagram AFAIK). Since it wouldn't have any usage in core we should remove it IMO.
#41-16: Improved the comment a bit. I agree that we should try not to break things.
#41-21: Is there
getConstraints()on the entity level? I remember looking for this when initially implementing this but wasn't able to find better way. Note that I am not super familiar with this part of D8 and would appreciate any hints.#41-30: This is how #2669802: Add a content entity form which allows to set revisions is solving this. Main reason for this approach is the fact that you can't decide not to save as new revision once you've set it (which happens in
prepareEntity()).#41-31: Removed.
#41-32: This exception is used only in contrib modules. We could move it to some other namespace inside core, but since there would be no core usages of it that doesn't make much sense in my opinion.
#41-33: Fixed.
#41-34: Removed the message. Didn't change bundle form part yet as I am not sure how to approach this? Should we still display plugin configuration fieldset with a message or entirely remove it it?
#41-36: Fixed.
#39-11: Not relevant after #41-8 anymore.
#42-1: A lot of contrib modules also use this function. I'd prefer if we can leave it like this.
#42-2: Done.
#42-3 and #42-4: We also need to consider BC aspect. I'd be totally for standardization of function names, but would prefer to leave existing functions in and declare them deprecated. Thoughts?
#54-1: Yes. It was added in #14 based on #11-4
#54-2: Done. While working on this I also notice that we're not updatng r_uid and r_timestamp correctly. Also fixed that.
#54-3: Fixed.
Comment #60
jibranThanks @slashrsm for fixing the review.
Can we add docs around
media_entity_update_last_removed()and magic 8003?Comment #61
slashrsm commentedGood idea. Here we go.
Comment #62
dawehnerJust some additional feedback/questions.
It is sad that
action.configuration.*doesn't exist.For me "adding" implies "new", so "Add media" would mean the same
Isn't this added on the template level only now these days? So aka. move this to classy templates.
I don't see any use of this annotation in core
Wait, shouldn't this be 'status' = 'published', given that
\Drupal\node\Entity\Node::isPublishedgets the value from the 'status' key.Its sad we cannot use
RevisionableContentEntityBaseI was wondering whether this should be fetched from the media type instead.
This seems logic which could live on its own service outside of the main media content entity.
Note: You can use
$this->currentUser()directly here.Can't we redirect to the media library?
Wait, is that the same as using
Url::fromRoute('<front>')which seems a little bit more semantic.At least these two services seems not needed by default, nor by the generic plugin. Are you sure this will be useful in most of the cases?
Comment #63
seanbRegarding #31. I understand that it is probably a lot of work to write a upgrade path when renaming the module, but having the inconsistency between the media entity and the node/user/taxonomy entities right from the start doesn't feel right. Just naming the module 'media' should be the way to go imho.
How about providing a upgrade path in contrib so this could be out of scope for the initial development (in the media_entity module for example)? The media entity bundles people defined might not be compatible with the defaults in core so there might be trouble when upgrading anyway?
Comment #64
Bojhan commentedI need up-to-date screenshots to review. Also likely better to split the UI review into a seperate issue.
Comment #65
yoroy commentedI collected screenshots in here: https://docs.google.com/document/d/1BnkA4fXZWG1nC4ipyl90BSvnE6Buqyjuz4gT...
Separate issue for the ui parts is a good idea.
Comment #67
phenaproximaThis looks pretty similar to the config schema for
field.formatter.settings.image. Can we simply re-use that here?Supernit: should be "its".
We are probably going to need something a little more descriptive here.
I've never seen this pattern before.
I don't care what we call these internally, but the users should never see the word "bundles" -- it's too techy. Can we expose these as "media types"? I'm told that the UX people are in agreement about this...
Can this be "Add media"?
We probably should not be linking to the contrib module page :)
Discussed with @seanB in Berlin -- I really don't like that we're relying on a library of non-vector icons existing in the file system. It's beyond the scope of this patch, but I think we should supply a font with all the media type icons, and a style sheet providing classes that use those glyphs. That way people can still override the icons if they need to, and we completely avoid any file system-related jankiness. We should create a follow-up issue to discuss and implement this approach.
s/bundles/types
I'm not too concerned, but why did we override these in the first place?
I think I'd prefer it if we first tried
$this->getType()->thumbnail($this), and if it returns a falsy value, fall back to getDefaultThumbnail().The last bit of this can be
empty($record->revision_log), for simplicity's sake..Can this be
static::class . '::getCurrentUserId'?Let's get rid of the "item(s)" here. It reads strangely. Also, s/bundle(s)?/type$1
Can we use $this->get()?
Can we do
return $this->set('description', $description)?For consistency, can this be changed to $this->get()?
Can $configuration be type hinted?
I'd prefer using $this->set().
$this->get()
Can we use $this->set() here?
$this->get()
So it looks like this code was copied more or less directly from Node's DeleteMultiple class. If we're going to re-use this logic, I suggest that we open a follow-up issue to move Node's form into a generic core namespace for re-use.
Again, this is adapted from Node. We should move the code out of Node so the rest of core can reuse it.
...in a followup, of course.
I'm not sure why we're doing this. Can we simply use core's ContentEntityDeleteForm instead?
Should be using getOwnerId(), not the deprecated getPublisherId().
Can we use methods of EntityPublishedInterface instead of $entity->status->value?
Missing documentation for $entity_field_manager.
s/bundle/type
Should be "...logic related to this media type."
Not sure we need this.
Nit: No need for the switching expression to be in parentheses.
I'd prefer to do an array_diff_key against $entityFieldManager->getBaseFieldDefinitions(), but this is OK for now.
$this->moduleHandler is not explicitly injected; if there's a moduleHandler() helper method, let's use it.
Either s/bundle/type, or (my preference) just say "Save" and "Delete".
s/bundle/type
s/bundle/type
This is only used by the bundle form; IMHO it does not belong on the entity interface.
Why not simply use EntityDescriptionInterface?
I'd like $configuration to be type hinted.
s/bundle(s)?/type$1
Let's use $this->currentUser()->id().
Why are we calling the label %info? Can it be called %label?
For clarity, can we say "type plugin" here instead of just "type"? Ideally we would rename the method as well, but I understand that we can't for BC reasons.
Needs more explanation. Returns the display label of what, exactly?
What if FALSE is a valid value for a field? <evil grin>
Nit: "functions" should be "function".
Needs FQCN and a better description.
Nit: s/Uri/URI
I think we could just {@inheritdoc} here...?
Needs FQCN.
We can't assume $media is an object, given the logic in execute().
It doesn't look like anything else in this class uses the tempstore...what is this needed for?
I only quick-skimmed this, but I don't understand why we can't just use the core image formatter. I see no dramatic differences between this one and that one.
Tests? This does not appear to be a test :)
Not a test.
Given some of the code in this wizard, it looks like it should extend the Media one rather than WizardPluginBase.
Comment #69
yoroy commentedRe: 8 in #67 There's an issue in place for creating SVG file type icons: https://www.drupal.org/node/2833768
Comment #70
slashrsm commentedThis is addressing #67:
#1 Done
#2 Done
#3 Tried to improve it. More ideas welcome.
#4 Contrib uses this notation a lot. Core does not though. Removed.
#5 This is being discussed as part of UX review. It seems that it will be changed to "types". Let's wait for final decision and address this together with other UX things.
#6 Done
#7 Needs work (whole hook_help() needs to be rewritten)
#8 Great to see that this is already in progress. If icons issue is done when this is ready we can change the icons as part of this patch. If not we can totally do this in a follow-up in my opinion.
#9 See #5
#10 I believe that every content entity type needs to define those.
#11 This has to do with queued thumbnail downloads. If queued downloads are enabled we set default field and rely on queue to set it properly later (i.e works as designed).
#12 Done.
#13 Done.
#14 Removed "items". See #5 for s/bundle/type
#15, #16, #17, #19, #20, #21, #22 Should be equivalent. Other core bundle entity types use the current approach as well.
#18 Done.
#23, #24 Yes, let's do that!
#25 Done.
#26 Done.
#27 Done.
#28 Done.
#29 See #5
#30 Done.
#31 Done.
#32 Done.
#34 I'd love to if it existed :)
#35, #36, #37 See #5
#38 Done. I used MediaBundle::load(), which should do the job.
#39 Done.
#40 Done.
#41 See #5
#42 Done.
#43 Done.
#44 Done.
#45 Done.
#46 Good point. How to address this? Throw an exception if data is not available?
#47 Done.
#48 Done.
#49 Done.
#50 Manager has different signature than parent.
#51 Done.
#52 Done.
#53 It is the way action communicates with the confirm form. Another node C/P.
#54 Main difference is field type. Core formatter is for image fields while this one is for entity reference fields.
#55 Well, since core does it it must be OK, right? :D Fixed. We probably want to open a follow-up to fix this in other core classes as well.
#56 Same as a above.
#57 I am not sure. There are some similarities, but no part is exactly the same (at least table name is different). Extending Media wizard might save few lines of code but require few overrides that would make core more confusing IMO.
Comment #72
slashrsm commentedThis should fix the tests
Comment #73
samuel.mortensonI noticed at /admin/structure/media/add that the "Type Provider Configuration" area is visible even when a provider has no configuration (i.e. the Generic Provider). The same applies to "Field Mapping", which isn't of much use if no metadata fields are available.
Can we conditionally hide these?
Comment #75
slashrsm commentedTry nr. 2.
Comment #77
tkoleary commentedA 'Media" is not a thing, and a noun is not really needed. 'delete, publish, unpublish, save' should suffice.
'Bundle' is tecnical language leaking into UI. We have content and comment types, this should be 'media type'
Comment #78
berdir@tkoleary: Media type has been discussed before and we expected that this would come up. The problem we have is that we have two different "types". We have plugins and we have config entities (the bundles). You could have multiple bundles that use the same type (media type plugin)
The most obvious way where this would show up is if we would rename Media bundle to Media type is that when you click on "Add media type", then the first thing (well the second, after the label) that you are asked is which Type your Type should be :)
That's why media_entity maintainers in contrib made the decision to use Bundle in the UI. The other options are:
a) deal with having two different things being called "Type"
b) Use "Plugin" instead of "Type" in the UI for the plugins, but I suppose that's not really better than using Bundle for the bundles..
c) Come up with a different name for either of the two types for the UI.. any ideas?
Comment #79
tkoleary commentedI'm leaning towards a) deal with having two different things being called "Type"
We could have:
a) 'media type' and 'Mime type' if mime type maps to the plugin (does it?).
b) If we think that even 'Mime type' is too jargony for end users (I do) we could just not refer to mime types at all but only to the plugins.
In that case we could call the plugins whatever we want. I'm leaning towards 'Media handlers'. To me that implies 'the thing that gets the media to actually show up on the page'.
Now it makes a lot more sense to the user following the logic "I can create a media type and call it whatever I want but if I want the thing to display I need to go find a 'handler' for it".
Comment #80
bojanz commentedI agree that the bundle/type duality is confusing. At this point we've been well trained that bundle == $entity_label type.
Feels a lot clearer to use "media type" for the bundle entity, and use "plugin" or "handler" for what is currently the media type (So "@MediaHandler" then?).
Comment #81
tkoleary commented@bojanz yeah, so I'd suggest:
Media bundle => 'Media type'
Provider/plugin => 'Media handler'
Media entity => 'Media item'
As I suggested above, we don't need to say 'Media item' every time there'a an action (Delete media item), just 'Delete' is fine, that's what we do with other entities. We can remove nouns even in other contexts like 'Delete media configuration' which can just be 'delete configuration'.
Also when we return a DSM we can shorten it to 'item' eg. 'The item was deleted'.
It's important to remember that the word 'Media' is plural (it's the plural of Medium, though it is now also used as the plural for 'a bunch of things created in a variety of mediums [sic]'). For that reason we should never use it in the singular eg. 'Edit the media'. This is why I'm suggesting 'Media item', but also why the tab 'Media' is ok (you are going to a page with a number of media items), as is '+Add media' (you may be adding more than one piece of media).
In addition to all the strings where 'Media', 'Media bundle', 'Media entity', or 'Provider' need to be changed there are also a bunch of stray outliers like:
'Allows for media assets to be created and used on the site.' - should be 'media items'
'The user ID of the media publisher.' -should be '...of the author of the media item'
'The following media translations will be deleted' -here we can dispense with media I think and just say 'the following translations...'
And I'm still looking for other instances.
One other thing:
Strings that say 'The date that' or 'The time that the media item was edited/updated/deleted' don't need 'that'. Just 'The date the media item was...' is less wordy and perfectly good grammar.
Comment #83
Bojhan commented@tkoleary Thanks, item is indeed correct as it aligns with how we name content - which we use content item for the singular.
Comment #84
catchHaven't reviewed the whole patch yet but a couple of things:
Has this been manually tested? Looks fine in theory but don't think we've done this before in the same way.
These conflict. Do we have to add them here? Or could we remove them and put them back once https://www.drupal.org/node/2831144 is done?
Can use __DIR__.
Also is it even possible for this to be different each time? Maybe with a $conf override?
Comment #85
slashrsm commented- #81 implemented
- "bundles" renamed to "types" in UI
- "types" renamed to "handlers" in UI
- Bundle form: field mapping is now updated when handler is changed and hidden if no metadata fields are available for mapping
- Bundle form: field mapping and handler configuration fieldsets are now hidden until a handler is selected
- Removed "Generic" plugin. Since we have no plugins now it is impossible to test. We could merge #2831937: Add "Image" MediaSource plugin into this patch. Another option is to enable "media_entity_test_handler" test module that provides dummy handler for test purposes.
We still need to decide how field mapping should look. See this video by @yoroy and my comment: https://docs.google.com/document/d/1BnkA4fXZWG1nC4ipyl90BSvnE6Buqyjuz4gT...
#84-1: Not sure. It is possible that default content entity destination will work. We would have to research this a bit. Do this in follow-up and create new plugin if we identify it is needed?
#84-2: We are planning to do that, but not yet. Media entity is just a first step. More things (handlers, configuration, field widgets, ...) will follow. I'd officially deprecate them when all this is ready and we're just about to start using new system in standard profile.
Comment #86
tkoleary commented+1, It's really not an MVP without at least one plugin.
Comment #87
slashrsm commentedThat's not even a question. Question is if we want to make this patch even bigger or do that as part of: #2831937: Add "Image" MediaSource plugin.
Comment #88
yoroy commentedThe MVP is what we aim to have at 8.3 release time. There can be many commits towards that. So lets take it step by step and work on #2831937: Add "Image" MediaSource plugin when this is in place. Lets have more than one moment to celebrate progress :-)
Comment #89
slashrsm commentedImproved underlying mapping logic to eventually allow us to auto-create fields and tried to made it more extensible along the way.
Comment #91
sam152 commentedShould thumbnails be part of the media type interface? It doesn't get satisfied for document, audio, tweet etc. Would a "preview" view mode be a more generic, non-assuming concept to use for the same purpose. It would take a lot of complexity out of the module.
Comment #92
slashrsm commentedThis is rebase on top of #2625854: Provide default source_field when creating new media entity bundles, which should solve one of the biggest UX problems for site builders.
It affects handler plugins which now need to:
- declare allowed fields in an annotation
- implement
createSourceFieldStorage()andcreateSourceField()Comment #94
slashrsm commentedRebase on top of #2669802: Add a content entity form which allows to set revisions .
Comment #96
gábor hojtsyCreated a placeholder docs page for media module at https://www.drupal.org/docs/8/core/modules/media (and an overview page under this), similar to how other proposed core modules are listed (eg. trash and workspaces). Links to docs on the help screen should be updated to point to this new docs and not a 3rd party site.
Based on the discussion at the UX meeting yesterday (as suggested by webchick) and after careful consideration, we decided to go with the "media" module namespace.
Also taking off the usability review tag, since we have that. See video at https://www.youtube.com/watch?v=3yZat7zXfoE
Updated issue summary.
Comment #99
phenaproximaAmong other things, this patch renames Media Entity to Media. Have a nice day!
My feelings on this: http://img.michaeljacksonspictures.com/wp-content/uploads/2014/08/popcor...
Comment #101
gábor hojtsyLet's unify the base field naming to be more consistent and user friendly. Comparing to nodes:
For example where we take the username for the author, the UI says we want an ID :) These are the green ones. (But there are some other inconsistencies with revision authors, so I would just propose to do a sweep of all base fields).
Then I added the orange stuff. I don't think the description for the name field provides any additional info whatsoever. And the title of it should be "Name" because we are already on the media item form.
Comment #102
gábor hojtsyComment #103
slashrsm commentedRename this too?
Rename issue nitpiks:
How do existing images relate to this.
Comment #104
dawehnerIt would be great to address #62 as well :)
Its good to put that onto the type itself
What about checking for the interface instead. It is a bit easier to read in that case
I prefer to use
$media->get($destination_field)as that's way less magic and no{}involvedI'll try to write a better
hook_help()at some pointThat feels like a legitimate usecase of
\RuntimeExceptionThis should I think better to a
restrict accesspermission.Nitpick: this is unused
Should we have a dedicated method for the name, much like there is
$node->getTitle()Let's make that consistent
NOte: A form should end with Form, shouldn't it?
why is this public?
This
<p>tags look weird, maybe you could useinline_templateCan't we filter by FieldConfigInterface or so? Aren't these simple the base fields?
It is confusing that we just catch the exception, this sounds like trying to hide problems in the future.
NoteTypeListBuilder uses
$row['description']['data'] = ['#markup' => $entity->getDescription()];Generic no longer exists :)
That's a serious testing effort
Note: All assertion methods have the expectation and the actual value in the wrong order, its
assertion($expected, $actual)Ubernitpick: two empty lines :)
Comment #107
gábor hojtsyHere is a massive help text update. Part of this was reviewed in the UX google doc but I made some further reorganization on putting this together to explain the media handlers and how they relate to types and fields. So the basic concepts are pinned down. Otherwise the extent/approach of the help is based off of node module.
No other changes.
Comment #109
slashrsm commented#101, #103 and #104 addressed.
Comment #110
xjmAfter Dublin, I had suggested we add Media as a beta experimental module, thus communicating the expectations that the API and data model should remain stable, but that we might need to do some work in followups. Otherwise we have to be a lot more stringent in reviewing this.
Comment #112
slashrsm commented#62 and few other things addressed.
Comment #113
gábor hojtsy@xjm that is quite different from what the core management team approved on https://www.drupal.org/node/2786785/revisions/view/10181694/10191559 (this is the edit webchick made while on the meeting). The only way to add it as a beta is to add it as an experimental. We definitely cannot build anything on top of that then for the standard profile (eg. default media types would need to be with the media module instead of the standard profile unlike eg. node types), or even start deprecating file fields or direct CKEditor file uploads. All of those would need to live in some experimental module then and later moved to other places in core which may cause problems for people who try them out and will be more pain to support with upgrade paths.
Comment #114
dawehnerIt would be great, especially for additional follow ups, which I work on atm. to have a media component: #2835817: Add a media component to the core issue component field
Not sure who to ask though to be honest.
Comment #115
xjmI discussed with @Gábor Hojtsy that the signoff he mentions above were only on the high-level idea, not the detailed plan, which I was not given opportunity to sign off on subsequently. It's not a big concern though because it is a one-line change to turn an experimental module into a stable one or vice versa. I'm definitely not proposing moving the code around.
Is there the expectation that those things should happen in this initial patch?
Comment #120
mtodor commentedChanged default source field creation in MediaHandlerBase. Removed 2 abstract methods -> and provided only one, that should return type of default source field.
Comment #121
naveenvalecha@Gabor, help text looks good.
As #2835817: Add a media component to the core issue component field has been done, Is "media system" component change fine ?
Comment #122
slashrsm commentedIt may be a simple change but it can have significant influence on many things :).
Oh, no! Please! :) Plan for this patch is more or less to do what it already does. We are not planning to introduce any significant changes. At this point it needs feedback to see if there are any missing pieces that people from outside of the Media initiative would bring up and can not be done in follow-ups.
Things that @Gábor Hojtsy brought up will follow after this gets in/are already in progress in other issues (as per the plan aligned in #2825215: Media initiative: Roadmap and #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core). The thing is that a lot of those steps depend on Media being stable and not achieving that would make the plan much harder to realize.
Comment #123
tim.plunkettThis needs a corresponding
media.handler.*This is a little odd, the only module that declares an explicit dependency on system is user.module... But I guess it's not wrong :)
Half of the yml files have a blank line between entries, might as well pick one
Copies
@throws \RuntimeException
Haven't written a procedural function like this in a while, but usually we explicitly have \RuntimeException
media_entity or media?
Should be unnecessary since $media is typehinted above
This docblock needs expansion. See \Drupal\Core\Display\Annotation\DisplayVariant for an example, but it should @see the interface, base class, and manager, should describe the plugin namespace, and some working examples.
Please do not use 'default' as a form operation.
Extra blank line
string[] or int[]?
Status is usually an entity key as well.
Does not need to be specified here.
The links section should have a trailing comma
ID
This is a question with no question mark, please rephrase
This uses DefaultSingle, so it's just one
Why would this need to be exposed? And the implementation must be
return $this->getHandler()->getConfiguration()if it is kept.ConfigEntityBase::preSave() loops through the plugin collections and updates the entity with the correct values.
Then this will do it again, always adding in 'source_field'? Why isn't that handled (hah) by the plugin itself?
Also, I disagree with exposing setHandlerConfiguration as it's own method, and then blowing away the collection within it.
If you keep the method, it should be as shorthand for $media_type->getHandler()->setConfiguration().
Half of the setters are fluent (return $this), we should be consistent.
Nit: can combine these
Don't need the
? TRUE : FALSEpartInject this service
Going to just not read this whole method O_O
if (!empty($element['#published_status'])) {is just as good. Also that (bool) cast is redundant.AFAIK the cast is redundant
\Drupal\Core\Entity\EntityFormInterface::save() has a return value!
Put } on a blank line please
#title strings shouldn't end with any punctuation
Curious why this is needed. If it's really important, please a line of documentation above it.
As mentioned before with the annotation, this should @see a bunch of stuff
Please rename to getProvidedFields()
Missing \Drupal\media\
Needs rewrapping to 80 chars
This should be consistent
These look odd together, but at least rename the first to getThumbnail()
Missing blank line
Can use the ::class version of these to be all modern
Not an interface
Instead of storing the entityTypeManager, do this retrieval in __construct. Also typehint by \Drupal\file\FileStorageInterface
Without reading further: why is the form storing the instances? Seems like it's just asking for serialization problems
All of this isn't needed for a callback
Ajax
Please do not put it in $form.
Also, just use $this->entity throughout, and then you don't need the @var
Very unclear why this is needed. Also that ternary seems backwards.
No reason not to use ===. Also, yuck this is why I prefer to use individual classes for each operation (with a base class)
Remove
No problem here, just not used to seeing these specified
Why specify these? Seems like overkill.
This can be omitted, that's the default
$plugin_id for clarity, also are you sure you don't want getSortedDefinitions here?
Oh my. Still no docs on why the instances are being stored.
Should use SubformState, see \Drupal\block\BlockForm::form()
Why is the
important? And other places usually the
is just concatenated onto the translated string.
Is there an interface to check here instead?
Needs an @todo or something
Seems a bit odd. Maybe a small protected method getWorkflowOptions($bundle)?
See note above about SubformState
Why not mess with #parents on the options and then let copyFormValuesToEntity() handle these?
Once again with SubformState
You shouldn't need any of this code. Once submitConfigurationForm is called, you should let \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() handle this.
This should be in submitForm, before calling parent::, and should directly manipulate the form_state values.
Same note as the last form, you might as well call parent::save, and you should return the result at the end
Yep, this is what needs to be done in the other form
Either cast it and then use strict comparison, or don't.
This is the third or fourth way I've seen it (media entity, Media entity, media entity object, etc)
I don't know which now or later is, TRUE or FALSE. If you want to keep it as a Boolean, maybe getQueueThumbnailDownloadsNow()? Not much better.
Called this out above, but I don't think the configuration should be directly accessible.
\Drupal\Core\Config\Entity\ConfigEntityInterface::status() already exists
A new revision of what? Not the media type itself, but media entities of this media type?
Can a longer explanation of what this is for be added here?
It's my understanding that filtering here isn't needed.
Can have a trailing comma to reduce noisy diffs for future additions
This is fine here, but you could also typehint $media as MediaInterface[]
Not sure we use @package?
If you fix the @return, then you don't need to use @var later
All of the drupalCreate* methods were renamed to create* and then imported with aliases like
createNode as drupalCreateNode, this should follow that pattern.Could be worth a protected method to do this, then you can codify the use of loadUnchanged and remove the need for the extra @var
Instead of casting it to a Boolean and using assertTrue, try
$this->assertInstanceOf(MediaTypeInterface::class, $bundle);(or assertNotInstanceOf in the opposite case)Side note, this assertion method seems to disagree with the assertion being made?
{@inheritdoc}
Comment #124
xjm@slashrsm, so my suggestion of having it beta initially could be for a very short period of time, and there would still be a strong chance of it being stable by 8.3.0.
The requirement is that it be stable before we make changes to stable core or contrib, right? So such issues are definitely blocked on the module being stable, but this initial commit itself does not necessarily need to be blocked on it being stable.
It's just an option to make iterative progress and allow us to do things in followups rather than all in the initial patch; beta still means we expect a stable API and stable data model. So the difference is mostly about followups. I'll make a recommendation once I have the chance to review an RTBC version of this patch in full. Seems like it is making really fast progress!
Comment #126
effulgentsia commentedI'll have more comments on this issue later, but just a quick drive-by:
A key difference though is expect is different than commit to. As an example,
big_pipemodule was marked as beta for 8.2.0, and just today, in #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal we changed its public API to @internal, in anticipation of breaking it in #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits. This highlights that there's both value in making/keeping something experimental until potentially API-changing follow-ups are done, and risk to contrib relying on beta-level module APIs.Comment #127
tim.plunkettOther notes from running PHPStorm's code inspect:
This neither passes in nor uses drupalSettings
Missing blank line
Here, and elsewhere, use Url::fromRoute() instead of \Drupal::url()
Unused
Unused
Unused
In addition to all of this code being problematic (see last review), \Drupal\media\MediaTypeInterface::getHandler() documented as returning a \Drupal\media\MediaHandlerInterface object, which can't be used as an array key here.
Which leads me to believe this has no test coverage?
$plugin may or may not be defined after this foreach loop, and it is used below here. This code also is not covered.
$image_style_storage should be typehinted with \Drupal\image\ImageStyleStorageInterface and is missing an @param
Use entity_type.manager
overridden
Unused
successful
Comment #128
phenaproximaI took a brave stab at fixing the stuff in #123. Here is what happened:
Then I did #127. The scoreboard from that:
I also incorporated a few of the changes requested in #42 (naming MediaHandlerInterface's getters consistently), and refactored @mtodor's changes in #120; MediaHandlerBase now implements createSourceField(), but it leaves createSourceFieldStorage() abstract so as not to over-abstract the act of creating the field storage. I have an idea for a possible compromise that would also remove the need for an abstract createSourceFieldStorage(), but I'll discuss with him tomorrow.
Comment #129
tim.plunkett#128 This is great! I went through and verified each, and below wrote @todo for the ones that still need doing. Most of it is around MediaTypeForm's use of plugins.
Parts from #123
1) Yes exactly, in core.data_types.schema.yml we have
10) Your change looks good. Default was supposed to be removed in #2006348: Remove default/fallback entity form operation but it was too late, let's not add more
13) It won't break anything, and honestly it won't do anything just yet as it's not being used properly: #2052083: EntityType::getKey() should be used instead of hardcoding id, status, weight, etc.
Best to add it IMO
19) @todo
20) @todo Let's revisit this one, but I believe it needs to change.
24) Oh, that's new! Nice
From here the number got off by 1, your 43 is referring to my 42, keeping my numbering
42) @todo
45) @todo The interdiff shows it being commented out, let's remove it
46) @todo
47) @todo I was pleased to see TRUE as a third param to in_array() elsewhere in the patch. I say default to strict comparisons unless there's a reason otherwise
50) @todo If they weren't in order for some reason, that'd make more sense. But they are.
52) For some reason I thought getSortedDefinitions() was available, but that's only CategorizingPluginManagerInterface which this doesn't use. Worth looking into though
53) @todo
54) @todo
57) @todo
58-63) @todo
68) @todo Docs are way better, method name still very iffy. It does NOT get you the thumbnail downloads, it returns a Boolean
69) @todo
72) @todo
73) @todo
Parts from #127
7) @todo
8) @todo
There are still several "unused import" warnings from PHPStorm, but maybe @alexpott can run his script to fix those later
Comment #130
berdirSmall note on 13. The actually meaningful key is now "published", not status, a per #2789315: Create EntityPublishedInterface and use for Node and Comment. We should use that trait now btw.
Comment #132
tim.plunkettEven for ConfigEntity? I don't remember any discussion about changing status to published.
Comment #133
berdirOh, misread that, got confused by the "want to avoid schema changes", that happens when you only read half of it :) Nope, status for config entities is fine, but I think that's actually a pointless left-over anyway? As far as I see, status isn't actually used and is probably a left-over, this really is the "published by default or not" setting like node types have, and is now stored as a field definition override.
Comment #134
tedbowNits
Class field name should be camel case
entity_get_form_display is deprecated
entity_get_display is deprecated
Unused use statement
Missing parameter type
Unused use statement
Unused use statement
Unused use statement
Missing @var tag in member variable comment
Missing @var tag in member variable comments
Errors
When editing as existing media type I got the error:
From
$handler was equal to an instance of \Drupal\media_test_handler\Plugin\media\Handler\Test(or the other handler).
I see @tim.plunkett commented on this. Also noticed that $configuration not declared before this line so empty($configuration[$handler]) will always be true.
And I think @phenaproxima punted. It looks like it should be $handler->getPluginId().
I am not why this error is not being picked up in MediaUiJavascriptTest it seems to test the edit form. Shouldn't a warning cause a test failure?
Later on at 171 we are setting this again
The earlier code assigning
$this->configurableInstances[$handler]['plugin_config']will also equal "[]".I commented out that line and MediaUiJavascriptTest still passes. Which makes me think the only thing that block is doing is setting $handler which has to be there for later if statements
Usability
"Field with source information"
The "Field with source information" on the Media Type edit/add form was confusing to me. I tested this with just the 1 handler in media_test_handler test module and then again with a test handler I made media_file_handler.
The first time I created a Media Type after I choose the test handler there was no "Field with source information" shown because there were no existing fields to choose from(I only knew this because I looked the code). This was a bit confusing because there was the "FIELD MAPPING" section that referred to "metadata fields" but no field for the source or indication that 1 would made. There was also no message that 1 had been made for me.
The second time I created a Media Type after I choose the test handler again there was now a "Field with source information" section with the select let me create a new field or re-use the existing 1. This was bit confusing because the automatic field creation was not something I was told about when creating the first type.
The 3rd time I created a Media Type after I choose a different handler, "file" there again was no "Field with source information" section. I could switch the handler to "test" and then I would see there was an option to create a new field.
The 3rd time was especially confusing because I just made the 2nd Media Type and had the "Field with source information" on the form. Switching to the other handler made me think that maybe the new handler didn't store it the media in the field as the other handler did.
I think it would make sense to always show "Field with source information" and if there is no field to reuse still let the user know that the field will be created automatically. Maybe even indicate the field type that will be created.
HANDLER CONFIGURATION vs FIELD MAPPING
These 2 sections are confusing because under "HANDLER CONFIGURATION" you are actually mapping a field, the source field and there could be multiple options here especially when editting a type. Would it make sense to move the source field under "FIELD MAPPING" and have 2 sub-heading Source Field and Metadata Fields?
Comment #135
tedbowCan we use StringTranslationTrait in this class?
Re my confusion about "Field with source information" section in #134 I see now there is
interface SourceFieldInterface extends MediaHandlerInterfaceSo not all media handlers will use source fields? If so then I think this is even more reason explicitly tell the user when a source field is being created(if there are no existing fields to re-use they are currently not if I understand).
Comment #136
phenaproximaAddressing some of the stuff from #123 that I had previously punted on:
@mtodor and I agreed to remove the createSourceFieldStorage() abstract method
in favor of defaulting to the first field type listed in the allowed_field_types
array in the handler's annotation. This is documented as well.
Addressing @tedbow's nitpicks in #134:
Also fixed #135.
Comment #138
gábor hojtsy@xjm: well, if the plan was not signed off on in general, just the idea then this is by far not the only issue to review. Also not sure how confident can we be in working on any of them given the exact same situation where sample content and theme initiatives were both of which had plans that they were confident that will be fine to implement (and ideas signed off on) but turned out to not getting signoff on their plans that they started to implement.
Comment #139
berdirI already mentioned this in IRC and it has been discussed a bit, so here is the official comment to propose my idea:
First, note that media_entity/media.module is very different for pretty much anything (the closest is moderation/workflow) else we put in core as experimental. bigpipe is a perfect use case for an experimental module. As it now official declares it has no API, it has no data.
You can enable it and disable it again, any time you want.
Media is different. It contains data but just as important, it *is* an API for a large ecosystem of contrib and custom modules that integrate with it. I think I can speak for all the media related/depending projects and distributions (like Thunder and Lightning) maintainers
that having experimental media module in core would bring us nothing and it would be impossible (mostly due to the amount of time it would require) to maintain modules that work with both media in core and media_entity in contrib, and as a result it would also be of extremely limited usefulness for users.
And while we can argue that moderation/workflow data can be removed without losing too much (your content is still there and still correctly published/not published), that is not the case for Media. You can't just uninstall it because we have no upgrade path in
8.4.x, all your media and metadata for it would be gone.
This proposal based on @catch's comment in https://www.drupal.org/node/2789315#comment-11762228, where he confirmed that anything new that has been added to 8.3.x is "experimential" until we release 8.3.0-beta1.
That means if we commit this issue now, then we still have time until then to improve and stabilize it to reach a MVP. What we need is a set of goals that we need to achieve until then. Both processes (like code/UX/testing quality) but also an actual feature set.
If we do not achieve that in time (say, a certain amount of days before beta1), we remove it again and try again for for 8.4.x and stick to media_entity in contrib. Nobody of us wants that, but IMHO it is better than having something experimental in core that nobody can use.
Basically, this is exactly what Dries proposed in his keynote in Barcelona with Feature branches, we just don't have them yet. But since (almost?) everything we do will be contained in one module, it should be relatively easy to remove again if we have to.
Comment #140
wim leers#126: While slightly off-topic, I felt like I needed to response to rectify and clarify something.
This is categorically wrong. It's not in anticipation of API breaks. It's to clearly establish that BigPipe offers no API. I'd been wanting to do that for a long time now, I just hadn't gotten to it yet. It turns out it would also help close/address some questions in #2657684, so I figured this was a good a time as any to do it. Please see the issue summary of #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal, that contains the full rationale.
#139:
Indeed, BigPipe has it easy. It simply layers on top of existing infrastructure (Render API + request processing system) and just delivers HTML in a different way. Zero configuration. Zero API (because it layers on top of existing infrastructure). Zero data (because it just optimizes a process). Hence it's simple & safe to install/uninstall at any time.
Indeed.
This seems very reasonable & practical. (Although we should ensure there's no in there, otherwise we risk not being able to do that.)
Comment #141
gábor hojtsy@berdir: thanks for the eloquent description of the situation. I think the #1 thing the media team needs is to identify any MUST have things that should be implemented in this issue and any MUST haves that should be implemented as followups either to accept this as stable out of the gate or just generally before 8.3.0. That would help us work on the most critical things rather than the nice to haves.
Comment #142
tstoecklerRe #123.10: The
defaultform operation is (unfortunately) needed for Content Translation. See\Drupal\content_translation\Controller\ContentTranslationController::add()and\Drupal\content_translation\Controller\ContentTranslationController::edit(). Let's bring that back please.Comment #143
xjmI responded to @Berdir's comments on #2825215: Media initiative: Roadmap as @Gábor Hojtsy quoted them there. Let's talk about the overall plan there, because as I said, it's at most a one-line change for this patch itself, so I don't think anyone working on this issue needs to be concerned about it until we are at the committer review stage.
Comment #144
slashrsm commented#123-19 - Removed it.
#123-20 - Removed setHandlerConfiguration(). Moved source_field logic into handler plugin.
#123-52 - Would be nice to have ability to get them sorted, but that assumes we introduce categories to handlers which is an overkill IMO.
#123-57 - Handler plugins will need to update anyway. Removed.
#123-58 - Done.
#123-60 - Unfortunately I can't do that since the individual values of the element can't be represented separately, which prevents us form moving it to the top level of the values array.
#123-63 - Done.
#123-68 - Looks better?
#123-69 - As already mentioned, killed and forgotten. :)
#123-72 - Done.
#123-73 - Done.
#123-42,46,53,54,59,61,62 and #127-7,8 (AKA media type form strangeness) Killed static saving instances and used sub form state correctly. Form's code looks much nicer and cleaner after the change. Thank you for pointing this out! Also, the object being used as an array caused a PHP warning, but didn't break the form's functionality (another proof how bloated the code was :)). We have extensive JS tests for that form that obviously didn't fail. Not sure if how to test for a harmless PHP warning.
#134 - Usability
Source field and fields in the mapping part have different purpose and should not be presented together. I tried to improve messages/descriptions that are displayed related to the source field. Hopefully this helps clarify this.
I've also decided to hide the source field drop-down when editing an existing type. Source field is so basic part of a media entity that should not be changed after the type has been initially created.
#142
defaultis backAll things raised by @tim.plunkett, @tedbow and @tstoeckler should be addressed at this point.
Comment #145
tim.plunkettNot going to respond to all of the changes that were perfect, because there were a ton, but thank you for being so thorough.
Okay having 3 is a bit odd now. Not sure what to do here, but this is even more confusing.
One idea would be to follow Term's pattern, and just use a single 'default', and vary by $entity->isNew() instead of $this->operation.
If you moved the rest of the handler section before the parent::preSave() you wouldn't need to do this hunk yourself
This could cause major schema errors in more complex handlers. They'd have to strip other values from $form_state before calling parent::, or skip calling parent:: altogether. I know of no other plugin that blindly does this.
Why have this? a) not used in the patch b) it's already accessible via $form_state->getFormObject()->getEntity()->id();
The second
$handler =is redundantComment #146
tstoecklerThis is completely out of scope in my opinion. We want to have dedicated form operations for clarity and to eventually be able to get rid of the default form operation. Therefore omitting them promotes a discouraged pattern, namely the default form operation. As stated above, we simply cannot omit the default operation because Content Translation has a hardcoded dependency on it. I am all for improving this situation and also making this more consistent among the other core entity types, but for the scope of this issue let us just leave it at that.
Comment #147
slashrsm commented#1 Left like this for now as per #146
#2 Neato! Thank you for the suggestion. Done
#3 Changed a bit. Should still save all values automatically assuming plugins add them in
defaultConfiguration()#4 Removed.
#5 Grr! Removed.
We also just realized that we introduced circular config dependency. Removed that as well.
Comment #148
slashrsm commentedNits that were discussed on IRC.
Comment #150
gábor hojtsyBeen thinking about tagging this
Needs framework manager reviewbut this does not satisfy the definition as in an issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change in architecture or public APIs, and their signoff is needed. This introduces a new subsystem and does not in itself affect multiple subsystems and is not an architectural change it is an addition. The bigger picture in #2825215: Media initiative: Roadmap would obviously do that so tagged that oneNeeds framework manager review.At the same time realized this needs a maintainer.txt entry since its a new component / module.
Comment #151
slashrsm commentedInjected a service into
MediaHandlerBase.Comment #153
gábor hojtsyBTW as per https://www.drupal.org/contribute/core/maintainers#decisions
. Not sure how to tell which subsystem is connected to which maintainer, but this has the release manager review tag, so that may in itself be enough. If somebody knows which manager is needed for approving a subsystem maintainer, let us know. I assume release manager is enough.
Comment #154
gábor hojtsyBased on https://www.drupal.org/node/1207020/revisions/view/9588197/10252774
As per @xjm, the issue may get to RTBC without the reviews of the respective managers, but committers would consult the respective managers.
Comment #155
phenaproximaOnly one tiny suggestion that should not block RTBC.
SubformState::set() is chainable, so these can all be chained.
Comment #159
chr.fritschIn order to fix the tests for https://www.drupal.org/node/2831936#comment-11827805 we had to move the logic for saving the source_field from the api into the ui.
Fixed #155 on that way.
Comment #160
tim.plunkettThis seems a little odd. Does getSourceField() have side-effects that matter to calling parent::save()? If so, it's not a getter.
Also the } else { syntax is wrong, else belongs on a newline.
Extra blank line
IMO this is a red flag. The API and the UI should be interchangeable, and unless you are explicitly trying to test the UI, this should use the API. Written another way, helper create*() methods shouldn't use the UI.
This also looks worrisome.
O_O
Comment #161
chr.fritschI removed the lock on the source field, because it was not possible to change any field settings. Instead of that, i added an entity_access hook, which forbid to use the "delete" operation.
Beside of that, i also made the source field required.
Comment #162
slashrsm commentedA bit different approach to #159 which should fix concerns form #160 in my opinion. Instead of different logic for UI I basically kept pre #159 approach with a flag that allows us to control whether field will be created automatically or not.
Note that we have a bit complicated logic around automatic field creation due to dependencies. Field storage needs to be saved first, media type follows and field save should happen at the end. This results in a bit unconventional code. I added few more test asserts to really make sure everything works as intended.
Also added another helper function to the handler, which could be useful (speaking based on experience working on #2831944: Implement media source plugin for remote video via oEmbed).
Comment #163
tkoleary commentedThis is a strange pattern with no precedent. THe common pattern for an optional field is to have a parenthetical suffix added to the label eg. '(optional)', or to add it to the description eg. This field is not required.
In this case since this is the behavior for when the select does not find any available options (and the setting is not required) the expected pattern would be to either disable the field or hide it. My preference would be disable, in which case the default option does not need to change.
Comment #164
gábor hojtsy@tkoleary: its fruitless to document a select element optional if you then have options and none of them are actually doing the *nothing* that you wanted. For it to be possibly optional but also let the user choose something, it needs an empty value and an empty value label. In fact Drupal select boxes have #empty_option and #empty_label for this as is used in the empty option code for example:
Which is what this code would ideally use as well, instead of adding this custom _none option.
Comment #165
catchMassive +1 to #139, we need to balance scheduled releases, with what goes into each release, with minimal breakage for contrib, with not making very hard/impossible to reverse mistakes with people's data, and this covers all of those.
Comment #166
alexpottI don't understand why this is a method and this functionality is not just inlined here? As it stands media_copy_icons() is new procedural API.
Does kakec mean poop?
Not used.
Comment #167
alexpottComment #168
alexpottRe #166.1 @Berdir said there had been earlier discussion - saying:
@slashrsm wrote
My thoughts are:
Comment #169
tkoleary commentedI'm confused. Why can't we just reference the media type icons from core/misc/icons as we have done for patches to other modules. As long as they are new and not changed we are not breaking BC right?
Comment #170
slashrsm commented#166-1: Removed
media_copy_icons()and moved the logic inline. It would be nice to have some standard logic that would allow modules to declare assets that it ships with and need to be copied into a publicly accessible location. Follow-up?#166-2: This is a small tribute to my younger daughter Lea. It is kind of an internal joke that the two of us have. And yes, it could be translated like that. :)
#166-3: Fixed.
#167: Fixed.
Entities can only reference managed files, which means that they need to be either in
private://orpublic://.Comment #174
jhedstromI was attempting to review the queue-based thumbnail processing functionality, and realized the while part of this has been moved to a service independent of the
Mediaentity as suggested in #62, that old method is still used in the queue worker:which will result in a fatal error since the method is gone. This also indicates we need a test around the queue worker I think.
Comment #175
jhedstromI'm also not sure how or where thumbnails would actually be downloaded, or by what mechanism... This would appear to be the intended method for the queue worker to call, but since it offers no alter hooks (or events, etc), how would this ever be set to anything other than the default thumbnail image?Edit, I see it would be performed in the handler--I missed the
isNew()check.Comment #176
seanbHere is a patch that addresses the issue in #174. Besides that, a check for the source field was added to MediaHandlerBase as mentioned in https://www.drupal.org/node/2831936#comment-11827950
I had some issues writing the tests for #174. Also not sure if I'm doing it right. A separate patch is attached with what I got so far. Any feedback is very much appreciated, I'm still trying to figure out how to do proper tests.
Comment #179
seanbAdded a test for #174.
Comment #180
seanbAdded a fix for #2831937-26: Add "Image" MediaSource plugin. The thumbnail should be updated everytime a media entity is saved, since the source field could be changed. I added an extra flag to the entity to skip queueing, because without this saving and queueing ends up in a infinite loop.
Comment #181
tim.plunkettI would have expected that interdiff to include test coverage. How much of this patch isn't covered?
Comment #182
slashrsm commented#176 and #179 look ok to me. Thanks!
Re #180:
Doing this unconditionally on every save is problematic. It can cause performance regressions for remote media types as they usually rely on a 3rd party API call to get the thumbnail.
What if we add an annotation value to handlers that would indicate if thumbnail should be updated on every update? That way we can enable this for handlers like images. For the handlers that can't do that I'd add something into UI that would allow editors to manually trigger update.
Comment #183
catchJust another plug for #1189464: Add an 'instant' queue runner - if we had a way to trigger interim queue runs outside of cron, could we get away without inline processing at all?
Comment #184
tedbowre @slashrsm's comment in #182
If this is an update rather than insert do we have access to the original entity here in preSave? If so could we just check to see if the source field value has been changed or not. It seems like if the source field value has not been changed then we could skip this.
If it has been changed we would probably want to generate the thumbnail for remote and local sources.
Comment #185
gábor hojtsy@tedbow: local files we can maybe compare, but remote media may change the thumbnail anytime, so that definitely needs some other logic. This is a question that was raised even before the media sprint in December, and the conclusion at the time was that we need to define how big a priority is this / if it needs to be fixed before commit given that in the life of the contrib module so far people did not encounter this as a problem. Often media library items are created and forgotten, new ones created for new media instead of existing ones replaced (especially given that media entities may be reused so if you replace their media, you don't necessarily know what are you changing). So that informs the discussion about the severity of this task/bug.
Comment #186
tedbow@Gábor Hojtsy
re:
That makes sense. I could how we can just leave it the way it is and it would fine
But one the other hand....
For remote sources even though we can't guarantee that if the source field value is unchanged the thumbnail will be the same. I would think that is almost always guaranteed that if the source field value is changed then the thumbnail will be changed.
So something like
Then we cover both remote and local and only make remote calls when can pretty be sure it is needed. Then we could solve the problem of remote sources not changing but still needing an updated thumbnail later in core or contrib.
Comment #187
seanbAfter discussing this with slashrsm on IRC we came to the conclusion that the handler should provide a method the check when a thumbnail should be updated. By default the MediaBaseHandler will check if there is a thumbnail or if the source field value changes.
The interdiff is based on #179 since I used that version as a base for the patch.
Comment #189
seanbDoh, this should fix the tests...
Comment #191
wim leersReviewed this patch at #2831936: Add "File" MediaSource plugin with the file handler. Only halfway through my review, I realized I was reviewing all of media. I didn't notice the separate "plugin only" patch. Posting it here instead.
Woah, I didn't expect this to support the
actionmodule in this first iteration!Nice to see it be so complete already.
icon_basesounds like it's a . What does that mean? The only meaning I can think of is . But then the label says it's a URI.So I think
icon_base_uriis probably a better name.Related nit: the description can also be cleaned up.
Surely this is not just any handler? I'm guessing it's a ? Wouldn't
media_handlerthen be a more appropriate config key?Nit: , because this is a boolean.
The description says . The config key implies it's not just a default. If it's a default, then the config key should be
new_revision_default.This PNG still contains Adobe metadata. Please optimize it with something like ImageOptim to strip that.
This provides summaries, but the docblock doesn't say so.
Should use
context.querySelector(…).There's no reason for this to depend on jQuery.
EDIT: never mind,
drupalSetSummary()is tied to jQuery. Gah.This provide summaries but the behavior name does not say so.
Nit:
handlers plugin definitions->media handler plugin definitionsWhy even say ? Why not just say ?
Why say ? Where else would you use them other than in Drupal?
Interesting that it depends on these, but not on
file.Perhaps it does depend on
file, but it's omitted for brevity, becauseimagedepends onfileanyway?I think it's better to be explicit here.
This seems … unorthodox. I guess it's because you want customizability?
Still, it's interesting that you choose to then copy files over. Why not just point to the files that are shipped with the module?
What if you update the default icons in the future?
Why limit it to
pngandjpg, why not send everything over?Nit: Unnecessary quoting.
- title: "media type"
- desc: "media type"
For consistency, it should be "media entity type bundle".
Or, "entity" should be omitted from the other one.
- title: "media"
- desc: "media entity"
Should this not be "media item" for consistency with the help hook and other places?
Let's omit these quotes, we don't have them elsewhere either.
Supernit: Title at the top vs bottom. Let's be consistent.
===
===
Why is this removing the
deleteoperation?Let's add documentation, or at least an
@see.===
and more later. Won't repeat again.
Do we have test coverage for this? Do we need it?
Didn't we banish
*.incfiles, except in extreme cases?i.e.
views.theme.incis >1000 LoC. What's in this file could easily be moved into the*.modulefile, no?Incomplete docs.
s/media/media item/
Yay, correct cacheability metadata!
Supernit: I think we always have the first method call on the same line?
s/
'medium'/$date_format->id()/First time we see "asset" mentioned!
In fact, in all of this patch, this very important/noteworthy keyboard is only mentioned TWICE! And both times in this class.
Let's remove it.
So is it a handler or is it a plugin?
Views is the only module in all of core that still mixes "handlers" and "plugins". This would add a second one.
This adds unnecessary confusion.
IMO media should be free of the term "handler", it should always use "plugin".
For consistency sake.
s/handler/media handler/
Huh, display? How is this a display? Seems a c/p remnant.
I read this in sequence as "midvid" — sounds funny. At least in my head. :P
This queues thumbnail creation. Elegant. Nice!
It would be more understandable if
getQueueThumbnailDownloadsStatushad a name likethumbnailDownloadsAreQueued… which is exactly what the interface docs say:Better name = more legible code.
The current function name does not indicate at all that this returns a boolean.
"node" -> c/p remnant
s/media/media item/
"username", but it's really a user ID. "username" is just what the default widget happens to allow you to type.
"media publisher" -> first and only time in all of this patch that this terminology is used. Should be simplified.
Again "publisher". Let's remove this.
Nit: s/array()/[]/
(This is done everywhere else, except here! Tiny oversight.)
This is not present in
media.type.file.yml, nor is it listed inconfig_exportin the annotation of this class. That seems like an oversight, because e.g.\Drupal\Tests\media\Kernel\BasicCreationTest::setUp()does use this.If this is for internal or testing purposes only, then that should at least be documented.
This seems like a strange helper method to have on a config entity type. If you keep this, consider marking it
@internal.- s/media/media item/
- doesn't mention this is for multiple, not single?
s/DeleteMultiple/DeleteMultipleForm/
and probably actually:
s/DeleteMultiple/MediaDeleteMultipleForm/
well actually:
s/DeleteMultiple/MediaDeleteMultipleConfirmForm/
… as evidenced by the form ID.
The typehint seems wrong?
Also, the ariable name doesn't seem to make a whole lot of sense?
That seems like a non-helpful cancellation URL? Should this not redirect to the media overview?
Wow this is complex! I guess there is no code we can reuse?
Should definitely have a follow-up to extract this to a trait or base class for other entity types to reuse.
Also should have explicit test coverage.
Again crazy complexity here. Same remarks/concerns apply.
Ah, this class name does make sense!
s/content type/media type/
s/controller/control handler/
Let's be consistent with the base class.
===
These are wrong.
They both make the same mistake. They both always vary by user, and always add the entity as a cacheable dependency.
But this is not necessary! If the user has the "any" permission, then there's no need to check the owner and hence no need to vary by user nor entity.
So, this needs to be split up into multiple step. The cacheability of the returned access result must reflect all data that was taken into account to reach that conclusion.
IMO it'd be clearer to list this as the
defaultcase in the switch statement.Then future refactoring to the other cases there could not cause this fallback statement to be reached, which would be incorrect.
Why is this necessary?
\Drupal\media\Entity\Media::baseFieldDefinitions()has a default value callback for the owner? (And probably should also have one for the created time.)===
Woah, 90! Let's document why.
This rings alarm bells for me. A getter that's modifying stuff.
It seems like this should not be possible. Should this not happen when creating a media type?
Yet more creation stuff? This seems brittle.
s/constraints/validation constraints/
Supernit: s/The/A/
By "URI", does this mean "file URI using the public:// scheme"?
Surely this means "URI as accepted by file_create_url()"?
Just "URI" is kind of meaningless.
Let's at least say "file URI".
s/media/media item/
The clash between "handler" and "plugins" is again super obvious here.
Shouldn't this be:
?
s/the/a/
s/The media/A media/
And should also do
@return $thisThis uses subform state, great!
===
s/sub-form/subform/
===
See earlier remark.
Don't URLs use
:placeholdernowadays?Mismatch.
This is tricky, but correct.
s/Collect cache tags to be added for/Add cacheability of/
s/Collect cache tags related to/Add cacheability of/
===
Mismatch.
This is a strange comment.
"created" or "changed"?
More strange comments.
This is not asserting cache contexts.
Supernit: s/logged-in/logged in/
s/service/handler/
Nice, JS test coverage!
Hm…
This seems rather pointless. If
contentis empty, printing it would not print anything anyway.Also, can
contentever be empty?If so, then you'll render an empty
articletag… which I don't see the point of?\Drupal\node\Tests\NodeCacheTagsTest: every entity type should have a cache tags test, based on\Drupal\system\Tests\Entity\EntityWithUriCacheTagsTestBase. That's how we ensure cache tag invalidation works correctly for every entity type. All the hard work is already done in there.Comment #192
seanbThe first bunch of comments in #191 are fixed. The tests are still broken from the patch in 189, but I think there are no new issues after the changes (still working on that). It would be nice to get some feedback on the todo's.
TODO:
3. Didn't touch it for now.
8. Since we depend on jQuery anyway, should we still switch to querySelector?
10. Related to #3. Skipped for now.
13. I think the module images should not be accessible. Didn't touch this for now.
16. There has been a lot of debate on this. Not sure what to do.
23. There is no test coverage for the theme suggestions. The node module doesn't have this either. Not sure if this is a blocker? Skipped for now.
26. Related to #16. Skipped for now.
31. I renamed this instance of plugin to handler. Handler was specifically chosen for all plugins handling media type logic. Not sure if it is a good idea to rename handler to plugin everywhere.
32. Related to #3. Skipped for now.
37. Related to #16. Skipped for now.
42. I think we can just remove this, not sure? Left it for now.
82. Didn't touch this for now.
87. Didn't touch this for now.
Done
1. Yay!
2. Done
3. Didn't touch it for now.
4. Done
5. This is actually in line with how the node module works. I think it should stay the same for a clear DX. Maybe fix this in all places through a followup?
6. Done
7. Done
8. Since we depend on jQuery anyway, should we still switch to querySelector?
9. Done
10. Related to #3. Skipped for now.
11. The 'on this site' probably was taken from the node module info file. But I agree and removed it.
12. Done
13. I think the module images should not be accessible. Didn't touch this for now.
14. Done
15. Done
16. There has been a lot of debate on this. Not sure what to do.
17. Probably taken from the node module. But done.
18. Again same as node module. But done.
19. Done
20. Done
21. Added documentation and @see.
22. Done
23. There is no test coverage for the theme suggestions. The node module doesn't have this either. Not sure if this is a blocker?
24. Fixed
25. Fixed
26. Related to #3. Skipped for now.
27. Yay!
28. Done
29. Done
30. Done
31. I renamed this instance of plugin to handler. Handler was specifically chosen for all plugins handling media type logic. Not sure if it is a good idea to rename handler to plugin everywhere.
32. Related to #3. Skipped for now.
33. Done
34. :)
35. Done
36. Done
37. Related to #16. Skipped for now.
38. Same issue in node module, but done.
39. Done
40. Done
41. Done
42. I think we can just remove this, not sure? Left it for now.
43. Done
44. Done
45. Done
46. Agreed, but since the media overview is not part of the MVP, this has to be done later.
47. This is basically the same as the node module. No tests there as well. Maybe this should be part of a follow up to fix it right everywhere. Didn't touch this for now.
48. This is basically the same as the node module. No tests there as well. Maybe this should be part of a follow up to fix it right everywhere. Didn't touch this for now.
49. Yay!
50. Done
51. Done
52. Done
53. Done
54. Done
55. Done
56. Done
57. Same as node module?
58. Removed getSourceFieldStorage() since it was called only once and added the code to createSourceField().
59. See #58.
60. Done
61. Done
62. Done
63. Done
64. Related to #3. Skipped for now.
65. Done
66. Done
67. Yay!
68. Done
69. Done
70. Done
71. Done, see #35
72. Done
73. Done
74. Yay!
75. Done
76. Done
77. Done
78. Done
79. The node module has the same thing, but removed it.
80. The node module has the same thing, but changed it.
81. The node module has the same thing, but changed it.
82. Didn't touch this for now.
83. Done
84. Done
85. Yay!
86. Done
87. Didn't touch this for now.
Items to check for handlers depending on the patch:
2.
3. And related
16. And related
35.
Comment #193
dawehnerFor #192.47 we have #2670730: Provide a delete action for each content entity type already.
If we care about the whitespace level details, we could use
{{- content -}}here.Nitpick: Let's use the Interface instead of the concrete class.
I'm wondering whether we could add a todo for d9 to extend from
RevisionableContentEntityBaseinstead. This would simplify things a bitIts weird/funny that we don't have a
EntityCreateInterfacebut aEntityChangedInterfaceIMHO the doc here should describe what this is actually doing. This documentation tells simply nothing.
According to the only method, it sets a thumbnail, so what about naming it
MediaThumbnailSetterInterface?Nitpick: you could use
::classThis method name is less obvious IMHO. What about using
getMediaLabelto make it clear that this is not the label of the media type?:linkshould be:urlHow does this even work? Doesn't it have to be $entity->changed->value at least?
Note: Value should be '1', as of #2459289: Boolean default values are not saved
Note: In phpunit based tests the expected value is on the left side
Just an ubercomment: To be honest it feels more readable to just repeat the line multiple times.
The drupal prefix is sort of a pure BC layer in core. As this are new tests we IMHO don't need that.
Again ... the expected message is on the left side.
Comment #194
seanbLooked at #193. Also think I found the issue for the tests. Let's see...
TODO/Discussion:
2. I'm not sure about this one. As pointed out by Berdir in https://www.drupal.org/node/2669802#comment-11817894, there could be multiple 'functional' base classes that you might want to extend. Revisionable, translatable, 'insert-random-feature'. I don't think there is a solution for this yet?
3. That would make a excellent followup :)
5. The setter was actually fetching a new thumbnail as well before setting it. I changed setThumbnail to updateThumbnail to reflect this. Getting the actual file URI was moved in a different function. I left the name MediaThumbnailHandler, not sure if this is a blocker.
Done:
1. Done
2. I'm not sure about this one. As pointed out by Berdir in https://www.drupal.org/node/2669802#comment-11817894, there could be multiple 'functional' base classes that you might want to extend. Revisionable, translatable, 'insert-random-feature'. I don't think there is a solution for this yet?
3. That would make a excellent followup :)
4. Done
5. The setter was actually fetching a new thumbnail as well before setting it. I changed setThumbnail to updateThumbnail to reflect this. Getting the actual file URI was moved in a different function. I left the name MediaThumbnailHandler, not sure if this is a blocker.
6. Done
7. It actually is returning the label of the media type. I changed the comment a little to make this more clear.
8. Done
9. The node module does it the same way?
10. Thanks, weird, but done.
11. Done
12. Done
13. Done
14. Done
Comment #195
slashrsm commented#191-3 - I wouldn't change the config key. It appears as part of the media type which implies it is a media handler in my opinion. +1 to change the human-readable labels though.
#191-10 - Similar as above. Yes, let's update it.
#191-13 - Default icons will eventually become managed files. Managed files can't be in the module's folder, they need to be in public, private, hash or whatever wrapper site is using.
#191-16 - If I recall correctly @tkoleary suggested to use "Add media" (omiting "item"). Let's check this with UX people one more time, decide and fix all strings to consistently follow the decision.
#191-31 (and related) - Calling it plugin doesn't say anything about the purpose of it. It just tells that it is implemented using plugins, which is not relevant from the functionality point of view. Handler kind of implies that it "handles media type" or "handles business logic related to a media type". I am open to giving it a better name, but I'd refrain from calling it a plugin.
#191-42 - Yes, I think that we can remove it.
Comment #196
yoroy commentedre: #196-16 yes, leave out "item" in the human facing labels please.
EDIT: of course, do use "media item" where without "item" the wording becomes awkward.
Comment #197
wim leers#192:
5. Hm. Probably. But doing so in a follow-up means you'll need update path tests for each of these. But yes, probably best to keep the same.
8. Indeed — I already said the same there :) I just kept this remark so you could see my reasoning, in case somebody else makes the same suggestion in the future.
16. Okay. But this should definitely be resolved before commit. Because it'd cause string translation changes afterwards. And it will undoubtedly confuse translators too. So, I think it's a hard requirement that this be made consistent. I'll leave it to others to determine what the right terminology is. The key point is that it must be consistent.
21. Great, thanks!
23. It probably is not a blocker, but it'd improve reliability + maintainability of the module for sure.
41. This was fixed by adding a single line worth of comments. The existence of this field is still highly questionable. I'm not at all convinced by this single comment that it's not just lingering technical debt.
42. I still am fairly confident this should be removed.
44. Tip for the future
git diff -M— records moves, so that you don't see an entire file being added and another being deleted :) Makes the interdiff a lot easier to read.46+47+48. That's okay, but then let's add a
@todothat points to another Drupal core issue that contains that follow-up.53+54: Explicitly confirming here that the fix looks correct. Thanks.
55. Explicitly saying here that the cleaned up version looks much better :)
58+59. That's entirely missing the point. It was
protected. It's not about having an extra method. In fact, it's better to have it as a separate method, that makes it easier to understand. The point is that it's very scary that a getter is not just a getter, and it's in fact making big modifications in completely different places. This definitely still needs to be addressed.78. Still wrong I'm afraid :) Easy fix though!
82. This must definitely still be tested. It would've caught the bugs in the
MediaAccessControlHandler.87. Definitely also still needed.
#195:
3. That's fair.
13. But why do they have to be managed files? Why can't the defaults be shipped files instead of managed files? That also addresses the "update" question: if you at some point change the default icons, then you'll need to somehow ensure that you overwrite the original icons. But only if the site hasn't customized them. But what if the file was just pushed through an image optimizer/minifier, and is in fact functionally equivalent? Having a system where the default icons are shipped files, and if a managed file with the same name exists (
file_exists('public://somewhere/pdf.png")) you use that managed file instead, this would avoid that problem.16. +1 for consistency. Like I said above, I don't care about which one per se, as long as it is consistent.
31. I'm sorry, I find your justification not convincing. "install the media plugin" can make just as much sense. Your argument could be made for "block handlers" instead of "block plugins", "filter handlers" instead of "filter plugins", etc. I do agree that we don't need to expose "plugin" in the UI. Nor do we need to expose "handler". It could just be "media type". Because we don't say "configure a filter plugin" or "configure a filter handler", we say "configure a filter". Similarly, it can be "configure a media type". Now, it's entirely possible that I'm just misunderstanding your justification. If others agree with calling "media type plugins" not plugins, but "media type handlers" or "media type handler plugins", then that should at least be very very clearly documented. For now, it's not clear why media is the only one with this special need to not call plugins plugins.
42. Excellent :)
Comment #198
seanbOk another round of changed based on #191.
Still TODO:
#191-41.
#191-82.
#191-87.
Discussed / done:
#191-3. So, we leave this as is I think.
#191-10. Done
#191-13. Since the icon files are (probably) made publicly available, I think it could be a security issue when everything in the folder is copied. Checking for images makes sense, I did extend the list to also move jpeg/svg/gif images. The reason why icons are managed files is because we currently use the thumbnail image field to store a preview of a media item. This could be a default icon, but also a actual thumbnail of a uploaded image. Always having something in the thumbnail field makes it less complex. I do see the issue with updating the icons and Wim is definitely not the first person to have doubts about this code, but I don't think this is a blocker.
#191-16. I've changed a whole bunch of places (mostly code comments). While doing this it seemed to me the best solution is to use just 'media' when it's related to actions. Like 'Delete media', 'Add media', 'View media' etc. In regular sentences using just 'Media' is weird. Eg. 'The thumbnail of the media item.'. There will probably be places where it still feels weird, and I think we should try to be flexible when things just look awkward or confusing. But as a general rule this could probably work.
#191-23. Since the node module doesn't have this as well it would be nice to do this in a follow up. Imho this is probably not a blocker.
#191-26. Done
#191-31 (and related). I Agree with slashrsm. Plugin is even more generic and not saying anything about it's function. There is actually a difference between media types and media handlers. Multiple types can implement the same handler (For example. Publication and Document media types implement File media handler). The handler part is only exposed when creating a new media type. Maybe the UX experts have a opinion on this? The correct technical term should be media handler plugin (in case we want to make a comparison with other plugins).
#191-32. Done, also renamed a lot of references in comments and labels to media handler.
#191-37. Done
#191-42. Removed it.
#191-46. Added todo, issue was already there.
#191-47+48. Added issue and todo's https://www.drupal.org/node/2843395.
#191-58+59. Sorry about that, I think I see what you mean now. I added back the getter and moved out the creation of the storage.
#191-78. Tried again..
Als got rid of all the references to bundle in comments and variable names (where possible).
Thanks Wim Leers for taking the time to discuss this :)
Comment #199
seanbFixed #191-82 and #191-87 by adding assertCacheContext to MediaAccessTest and a new MediaCacheTagsTest class.
Tried to remove the auto_create_source_field property as well, but after getting a bunch of test failures decided this needs a little more investigation. When adding a media type through the interface, the source field can never be empty. It makes sense that the source field should always be passes through handler_configuration or just create it automatically when that is not the case. Maybe slashrsm can provide some more info on this?
Comment #200
effulgentsia commentedThanks, everyone, for all the great work getting this patch to where it is. I think it would be awesome for it to make it into 8.3. I haven't reviewed the code in depth enough to remove the "Needs framework manager review" tag from it yet, but I'm posting this comment with screenshots in order to cover what are currently my biggest UX concerns with the module. There are UX team meetings on Tuesdays, and I've already pinged @webchick to get this on the agenda for tomorrow.
I mentioned in #2825215-28: Media initiative: Roadmap that although that issue has been referred to as the minimally viable product (MVP) for core media, I don't see a way of getting all of those pieces into 8.3. Which means if we commit the patch here, then by definition we end up with something less than what is minimally viable. And in #139, @Berdir gives compelling reasons for why Media module needs to be released as non-experimental. Which puts us in a bit of bind: we've never yet released a brand new (to core) module in a minor release without it first existing for at least one minor release as experimental. And here, we want to do so for something we're saying is less than minimally viable.
Because of this, @slashrsm said in #2825215-29: Media initiative: Roadmap that he prefers punting this to 8.4. That's of course an option. However, if there's value (for contrib maintainers and/or for the core product) to getting this into 8.3 core, then I think it makes sense to do so as long as it:
I think the use-case that this patch (when combined with #2831936: Add "File" MediaSource plugin and nothing else) solves is separating the steps of uploading a file to the site from referencing that file from within content. Which allows for 3 nice things:
There are various situations where this is handy. For example, a school where only admissions administrators should be allowed to upload admissions forms (PDF files), but where it's useful to allow content editors to link to those documents. Plenty of other use cases too, but mostly I'm pointing out that there's a useful use case here, even if just limited to files and not images or remote media.
With that in mind, here's feedback for how we can solve that use-case cleanly without polluting the UI for others. Feedback for each screenshot is provided below the screenshot.
With just this patch alone, it's impossible to create a media type, because a required field can't be filled out. Normally in a situation like this, I would say that we need to merge #2831936: Add "File" MediaSource plugin into this patch, since this module can't stand on its own without it, so we shouldn't commit them separately. But, @catch and @xjm pointed out that alternatively we could set this module to
hidden: truein the info.yml file. And remove that line in #2831936: Add "File" MediaSource plugin. I'm fine with that approach if people prefer reviewing them separately rather than together. The rest of this review is with #2831936: Add "File" MediaSource plugin applied.Question for the UX team: are we sure we want "Media types" within "Structure"? Or should we move it to within
Configuration -> Media?This screenshot shows that when I add a new field somewhere (in this case to the Article content type), I'm presented a bunch of choices, and
FileandMediaare two separate choices within the"Reference"section. How do I know how these differ? Some options for how to fix this:Referencesection and into theGeneralsection, since they don't really function like references from a content modeling perspective (you can't reference them from more than one place)."Other..."in order to create a Media reference field.My recommendation is #1.
This is the screenshot for what happens when you click "Other...". Again, both File and Media are options. How do I know which one I want? Here's some options for fixing this:
Filefrom this screen. It's actually broken. If you select it, you get an ER field to File entities (not a File field, like you do if you picked File from the previous screen), which then lets you reference files that were uploaded to other nodes. But usage from such a reference isn't tracked, so if you delete the original node, you lose the file from these other places. Now that we have Media as an option, there's no reason to choose File on this screen.My recommendation is #1.
This screenshot shows the
/admin/contentpage, which has a Files tab for listing the files on the site, but no Media tab for listing the Media entities. I think either in this patch, or in a followup, we should add a simple View to show the media entities. #2834729: [META] Roadmap to stabilize Media Library, whether in core as an experimental module, or in contrib, can replace the simple View with a much nicer library UI, but even without such a module installed, I think we need to have a simple listing.This screenshot shows an "Add media" link in the Tools menu, just under "Add content". I think that's too prominent a link for a module with such a narrow use case. We could consider whether to add this link into 8.4, when the module does more, but for now, I think we should remove it from here. And instead, just have an "Add media" primary action on the
/admin/content/medialisting I recommended above.This screenshot shows that the regular entity reference select widget works just fine for a simple use-case. People can install contrib modules for other widgets.
This screenshot shows that the regular "Rendered Entity" formatter already in core can successfully display what needs to be displayed for a simple use-case. People can install contrib modules for other formatters.
I'm curious to hear what the UX and Media teams think of this feedback. Thanks.
Comment #201
xjm#200 gets into some ER usability territory that came up in #1847596: Remove Taxonomy term reference field in favor of Entity reference also, so adding a bunch of related issues here. (Apologies if any of these are unrelated.)
Comment #202
andypostSome nits in code
Naming is strange but looks separate task
Alter[s]
this may lead to exception while installing and probably better to use state or hook_requirements() instead of exception
you can remove it, theme hook will set it for you
updated any media is not restricted?
is this used?
defined already in parent class
Comment #203
seanbHere are some thought on #200. Hopefully I addressed everything.
This issue/patch should solve:
I hope we can agree to do the following to get this done, but please let me know if there are good reason to add or remove some of these points in this patch:
Other issues/followups should solve:
Comment #204
naveenvalechaThanks for the reviews.Here's the attached patch.
Addressed #202
#202.1 Filed a follow-up #2844206: [PP-1] Naming is strange but looks separate task
#202.2 Done
#202.3 Done. Added a hook_requirement.
#202.4 Agreed. Done.
#202.5 Let's file a follow up to discuss. Node module also does not provide restriction for it. Let's file a followup issue to discuss how it should be handled for all content entities http://cgit.drupalcode.org/drupal/tree/core/modules/node/src/NodePermiss...
#202.6 Yup Removed.Also removed $entity which is already defined in the parent class.
#202.7 Done. Removed.
#203.3 Removed the 'Add media' from 'Tools'
TODO:
#203.1
#203.2
#203 : Amen for #200
// Naveen
Comment #205
naveenvalechaJust adding
hidden:truefor the media module so that it will not visible to site owners until #2831936: Add "File" MediaSource plugin// Naveen
Comment #206
wim leersHaving reviewed #2831936: Add "File" MediaSource plugin for how it integrates with this patch, and what problems it might have, I once again see further confirmation of my concerns about calling the
@MediaHandlerplugins "handlers". What they really are, are :image,file,youtube,instagram, et cetera. But this patch already reserves the concept of as the name/label/concept/description of bundles of media entities, i.e. for the bundle config entity type.That leaves me with two conclusions:
media typeconfig entities altogether: automatically create bundles for each@MediaHandlerplugin. Because they always correspond. There are no two different bundles (Media Type config entities) both using thefile@MediaHandlerplugin. As @slashrsm's screencast even demonstrates at the 2:07 mark: https://youtu.be/4Q9fE043GIM?t=2m7s.I know you could create and s that would both use the
file@MediaHandlerplugin, and + both using theyoutube@MediaHandlerplugin, but is that really a common case? If not, why all this complexity?@MediaHandlerto@MediaType. Or perhaps,@MediaTypeProvider.I'm assuming my first point will elicit very strong negative reactions. Remember I'm just trying to do due diligence here, and playing devil's advocate. It is essential that we question every architectural decision that significantly increases complexity, hence worsening supportability/maintainability.
In fact, that second point is something I think we can do anyway: renaming
@MediaHandlerto@MediaTypeProvider, even if we keep theseMediaTypeconfig entities.(See #2831936-54: Add "File" MediaSource plugin and #2831936-56: Add "File" MediaSource plugin.)
Comment #207
phenaproximaI am strongly against renaming the MediaType config entities. They are very similar in purpose and function to node types and block_content types, and therefore should follow similar nomenclature. Changing their name is asking for confusion and big trouble down the line. It will only exacerbate Drupal's existing naming problems. I am also against the idea of making media handlers directly responsible for bundles. Bundles are like stored configuration for the handlers. Even if it's not a common use case to have more than one bundle per handler, the flexibility should be there for a few reasons:
Regarding handlers -- I think the word "handler" perfectly describes what they do. They handle the business logic for a media entity type. "Provider", in my mind, does not imply that at all. However, because handlers are developer-facing and never really exposed to normal users, I'd be willing to compromise on renaming these. I'd much rather stick with "handler", but in the interest of moving forward, I won't push back as hard against changes in this area.
But I'm dead-set on referring to media types as "media types".
Comment #208
wim leersI never suggested renaming
MediaTypeentities. I say in #206.1 that Media Type config entities are mostly pointless, and in #206.2 I say that@MediaHandlerplugins should be renamed.Comment #209
phenaproximaFrom #206.2:
If we renamed MediaHandler to MediaType, we'd likely have to rename MediaType to something else to avoid confusion. :)
Comment #210
gábor hojtsy@wimleers: no, image is a kind of data a media entity type can hold. There may be media entity type for "ads", "illustrations", etc. that all use the image handler (are image based) but they have different uses, display mode configuration, administration UI, etc. The media types concept is separate from the media handler concept because these do not necessarily correspond 1-1 in ambitious digital experiences (to borrow Dries' wording). Yes, by default it makes sense to ship with a default type for them, but that does not mean there should only be one type for each handler used.
Comment #211
seanbI think the case for having multiple media type with the same handler is presented very strong. So +1 for just leaving the media type vs media handler as is.
About renaming the concept of media handler to media provider: there was some discussion about this and the choice for handler was something we agreed on. It would be nice if more people could give their opinion on this.
Personally I think media handler describes best what it actually does. There are already a lot of different types of handlers for all kinds of things. Since it is not ambiguous the word handler should always contain more context about what kind of handler it is. So referring to it as media handlers solves this. The concept of providers or plugins have the same issue, you should always refer to media provider or media plugin, so renaming doesn't help all that much imho.
Comment #212
gábor hojtsyRe two of Alex's review points:
No, this is the same problem as in #2811407: "Workflow entities" admin page title should be "Workflows" and requires either the same backend change / feature at #2767025: Add entity type label for a collection of entities or a stub route provider to fix the page title also. That will in itself fix the breadcrumb. I think this was put on hold in hopes of the backend change. I put the release manager review tag on it, so we can see if that is even feasible. We can add a stub route provider but then removing that later may or may not be considered a backwards compatibility / API change.
IMHO this could result in data problems for existing sites, if you go edit an existing ER field that has a file reference, would that show the file option still or would it break after this change? Either way this would need to be done very delicately. BTW overall the goal was to get rid of the file and image field eventually in favor of media yes.
Comment #213
wim leers#193.10: wow, WTF!
#198:
@todopointing to a follow-up, and that follow-up's issue summary should list the concerns to be addressedjpg+jpeg/gif should never be used for icons (because JPEG is lossy and GIF is less efficient than PNG-8). Only SVG or PNG.
Well thats' not all this module does, is it?
Probably best to copy/paste what's in
media.info.yml.Nit: 80 cols.
Nit: 80 cols.
#199:
Thanks for adding that test coverage!
I'm glad you tried to remove
auto_create_source_field(which is what I was getting at in #191.41, .58 and .59. You got stuck, that's fine. This is now blocked on @slashrsm's feedback.Hurray! :)
Nit: 80 cols.
Supernit: I'm slightly disappointed that this doesn't use the label
'Llama'like all other entity cache tags tests :PNow reviewing #200 — #205.
Comment #215
gábor hojtsySo originally media types were media bundles and media handlers were media type plugins. That went all the way up to #79 onwards when in discussion with @bojanz, @tkoleary and several others the name handler was suggested both from a UX and a backend perspective. That followed by a team discussion where everyone on the media sprint agreed handler was the way to go. Reviewers like @tim.plunkett, etc. did not raise this concern before.
From my (user) point of view, image/file, etc. are not media providers, they are containers/storage for media, I provide the media (upload the file, provide the youtube URL for Drupal). They are also not storage from Drupal's perspective, handlers use a field to store the media data on the entity (eg. youtube URL for youtube or the file itself for local files). So the handler does not store anything. All it does is handles what the user provided and uses the field system and integrates with media entity to manage the kind of media it can handle.
Comment #216
slashrsm commentedTotally agree that we should not remove media types. We have a very clear use case for them in core. Take #2831944: Implement media source plugin for remote video via oEmbed for example. The current idea it to implement it through an oEmbed handler. oEmbed supports so many different things that it would be very hard to justify to throw all of them into one bundle. It is extremely likely that a YouTube video will require quite different set of fields than a Blackfire.io graph.
"Backend" and "storage" are bad suggestions in my opinion since they *wrongly* describe what the plugin does. It does not store media but it works with it. It is aware about its specifics and translates them into a form that is understandable to the media entity. It *handles* them. "Type storage" is even worse as it incorrectly indicates that it handles storage of the type, which is absolutely not true. "Provider" is better, but not vastly superior than what we currently have in my opinion.
Confusion that @Wim Leers identified in #2831936-56: Add "File" MediaSource plugin was mainly caused by the rename from bundle/type to type/handler as the issue summary in the other issue wasn't correctly updated. We should have done better job keeping everything up to date, but the community also needs to understand that it is sometimes hard to do so when you're dealing with so many big changes in such a big ecosystem.
This is essentially about detecting whether we're saving a type through the UI (in which case source field should be automatically created) or in some other way (config import, programmatically, ... - in which case it should not be created automatically). If we find a better way of reliably detecting that I'm totally in support of removing it.
I think that Llama definitely needs to be there! :)
Tests! Yay!
Comment #217
slashrsm commentedDestination should not be hardcoded, since it is configurable. But... config is obviously not imported yet at this point.
I am not sure how to solve that.
Comment #218
andypostMy 5c for
Mediaentity- it always confusing "bundle" field (somehow not defined in
baseFieldDefinitions()that ER to "media type"- there was some limitations on "bundle" name for fields (just can't find issues) in entity api (at least at early days of d8)
- handler as plugin can have a fallback but that maybe not in scope so we can end-up with media types that have their "handlers" uninstalled... just not checked
- the most confusin is to write code -
$media->bundle->...causes autocomplete withbundle()method and DX--#210 greatly explains why there's no direct mapping of handler plugin to bundle (btw entity api allows dynamic bundles) but "handler" still confusing.
Why plugin instance using Handler name so I'm +1 on find better name!
Looking at
MediaHandlerInterfaceit's about provide fields, mappings and manage thumbnails it does not handle anything it provides...Comment #219
wim leers#200:
This is not true. It's totally possible to reference the same File or Image. It's just that with the default widgets, you can't do that. It's totally possible to create a widget that allows you to select an existing file/image entity. In fact, that's how we originally envisioned getting a MVP media library in, long before this Media inititiative.
This surely is just a bug that must be fixed, and not intentional?
Why shouldn't I be able to still choose ? What if I don't use Media, because my site is already built? i.e. this must be implemented in the Media module altering the File/Image modules. EDIT: seems Gábor raises a similar point in #212.
#203:
This is ignoring the fact that we already have a separate submenu. Which seems like a clearly better fit. The same does not apply to . So +1 for #200's suggestion.
#209: Well, yes. But I gave multiple suggestions. Also see #213.198.31 for even more suggestions.
#211:
\Drupal\media\MediaHandlerInterfaceand\Drupal\media\MediaTypeInterfaceto have very clear explanations in their class docblocks to explain their responsibilities. And have them point to each other. This is a must-have for maintainability. And it'd have prevented me from even raising this point :)So aside from the suffix being unnecessarily vague IMHO, it's also exceedingly strange to see: every single other plugin type has a more specific term than , even Views handlers (confusingly, Views handlers are a "category" of plugins if you will).
So let me repeat what I wrote #213.198.31 to have it all together here:
#215:
Probably because there were bigger problems to address first then :)
Exactly! So then
@MediaStorageplugins forfile,imageandyoutubewould make sense, wouldn't they?By that same reasoning (), is a wrong name, because the user is what handles the media. The plugin is what stores the media.
AFAICT this is not entirely true:
interface SourceFieldInterface extends MediaHandlerInterface— hence not every media handler must be storing a field.(Thanks for bringing that to my attention, it made me see another problem.
interface SourceFieldInterface extends MediaHandlerInterfacestrongly suggests\Drupal\media\MediaHandlerBaseshould not implementSourceFieldInterface, or it should be renamed. It also suggests thatSourceFieldInterfaceshould be renamed toMediaHandlerWithSourceFieldInterface.)Your description makes it sound like all we do in
@MediaHandlerplugins is handling/interpreting metadata. Is it a proxy of some kind?All this makes me realize a very simple thing: what is it actually that
@MediaHandlerplugins do? What are their responsibilities? They seem to be very glue-like, but glue between which things? What we're missing is documentation on\Drupal\media\MediaHandlerInterfacethat documents, explains and rationalizes all this. Doing that will help us determine if we should rename them, and if so, to what.To be clear: I'm not 100% against
@MediaHandleras a name. But I see dozens and dozens of alarmbells: from the vagueness of the name, to core not using this in any plugin name, to "handlers" being the old/original name of "plugins", to Media module developers themselves confusing this all of the place, to the responsibilities not being clearly documented, and so on. I just want to make sure we get the name right, because we'll be exposing this concept in the UI of every Drupal site.Comment #220
wim leers#216:
\Drupal\media\Entity\MediaType:)@MediaHandlerplugins. That will make this discussion a lot more productive, and it might indeed just confirm the current name :)auto_create_source_field. Thanks for the explanation. But I can't help but wonder: when isauto_create_source_fieldset toFALSE? AFAICT it's always set toTRUE? Or perhaps it's only happening when saving a newMediaTypeconfig entity? If so, we should changeif ($this->auto_create_source_field && $handler instanceof SourceFieldInterface) {toif ($this->isNew() && $handler instanceof SourceFieldInterface) {. Then we can simply dropauto_create_source_field?#218
\Drupal\media_test_handler\Plugin\media\Handler\Testas the media handler, and then tries to uninstall it. That should be impossible because the Media Type config entity's config should be depending on themedia_test_handlermodule.Comment #221
slashrsm commentedIt is set to FALSE by default:
and changed to TRUE in MediaTypeForm::save():
Working on this.
Comment #222
gábor hojtsy@wimleers:
I think these are primarily the responsibility of handlers:
1. so handlers are dealing with a kind of data (eg. file handler deals with local files, image handler deals with local images, oembed handler deals with some kind of oembed source);
2. they have the responsibility to help media entity create the base field they need (eg. image field for image handler)
3. they deal with the metadata associated with the type of source data, eg. image handler can extract EXIF data from the file and may map it to (a subset of) fields created on the media type
4. they handle the thumbnail generation process for the media, eg. take a thumbnail from oembed and store locally or provide an icon for a local file type based on its file type (eg. pdf)
+ for future UX improvement handlers may provide the supporting data to make it easier to create the metadata (eg. EXIF) fields on the media type (or a subset of those fields) see #2836153: Improve metadata mapping UI on Media type form (because handlers know what kind of metadata can it extract/produce based on the provided media and the type of those metadata values)
Comment #223
wim leers#221: RE: "set to FALSE by default" yes, I can see that, but it's set to TRUE in many places. I'm tempted to think all places. Hence my suggestion. Which you did not reply to.
#222 Let's see if @slashrsm also documents those 4 responsibilities (see the bottom of #221), or if there are more :)
Comment #224
slashrsm commentedFirst attempt to improve the docs of types and handlers.
In "real" code it is set to TRUE only in the form. All other assignments belong to tests covering this functionality. As already mentioned in #216: this is not about new vs existing. It is about *how* media type is created (UI or some other way).
Comment #225
wim leersThanks, this is a solid step in the right direction :)
"Media entity type" or "MediaType entity type"?
s/sort/type/ ?
"Media entity" or "MediaType entity type"?
This sentence is broken.
s/and a/. A/
Just now, you wrote "Media type", now it's "media types". Let's be consistent.
Main? Doesn't this list all responsibilities?
So it is about storage?
s/media/a media entity/
s/default/a default/
s/media/media type/ ?
Which entity? The host entity, right? (I.e. the content entity on which a media field exists, hence updating fields that are siblings of a media field.)
I think the documentation here can be improved further. I'm missing two things:
So far, I only see this confirming my concerns: it's very hard to clearly explain the concept, because the concept is not clear (IMO). This suggests the concept is flawed, or needs clarification.
s/with same/with the same/
What I'm still missing is one or two concrete examples. The examples mentioned in the last several comments seem valuable to state here, because concrete examples make it easier to understand an abstraction.
s/meaning/semantics/
s/in context/in the context/
Also: this makes a ton of sense, this is an excellent explanation :)
s/types/media types/
FALSE?isNew()) wouldn't work?Comment #226
slashrsm commented#225-1: Used "Media type".
#225-2: Used this on purpose to prevent conflicts with the media type (as in bundle).
#225-3: This is about media, not media type. Fixed the sentence.
#225-4: Done
#225-5: Of course. It is plural.
#225-6: Removed.
#225-7: No. It is about defining *how* it is stored. It is not *actually* storing it.
#225-8: Done.
#225-9: Done
#225-10: Done.
#225-11: Fields on media entity. Tried to explain it in more detail.
#225-12: Tried to improve it.
#225-13: Done. Added examples.
#225-14: Done. Great!
#225-15: Done.
See my comment from #216:
So... It is set to FALSE when created programmatically and during config imports.
See above.
Comment #228
wim leers#226: Thanks, that's another big step in the right direction :)
This still sounds strange. Should be verified by a native speaker.
I personally think (as a non-native speaker myself) that the following changes would make it better:
- s/the essential/an essential/
- s/the media type/a media type/
- s/sort of media/kind of media/
- Media entities, without a media handler retrieved from their media type, don't have any knowledge about the media they represent.
s/Media/media/, for consistency with the rest.
s/SoundCould/SoundCloud/
This is still a big contradiction.
This is a lot clearer already. Especially this portion:
However, isn't the part between parentheses only true for
SourceFieldInterface? I also brought this up in the parenthetical in #219.This one is now clear.
This is kind of clear. Where it's not clear is that media handlers are only (optionally) adding entity validation constraints.
It doesn't help that
FilenorImagemedia handlers are actually using this API.In fact, doesn't that mean this should be split off to a separate interface?
Clear now.
Well not specific to the media type, but specific to the handler, right?
And this is referring to
\Drupal\media\MediaHandlerInterface::getField(), right? So apparently it's not a field (in the Entity API "Field" sense), but it's metadata. It just happens to be that that metadata may be mapped to an actual Entity API Field. That mapping is explained in the next point.Doesn't that mean that
\Drupal\media\MediaHandlerInterface::getField()should be renamed to::getMetadata(), and\Drupal\media\MediaHandlerInterface::mapFieldValue()should be renamed to::mapMetadataToField()?Also clear, but see previous point.
In other words: let's make the
MediaHandlerInterfacemore clear. Then it'll become clear automatically whether@MediaHandler/MediaHandlerInterfaceis the right name.I'm looking into the "auto create source field" stuff.
Comment #229
slashrsm commentedThis patch was discussed on yesterday's UX meeting: https://www.youtube.com/watch?v=LzU97QLUnQU
To summarize:
I'd create follow-ups for 1. and 2. Attached patch adds 3 as it currently is in contrib version of the module.
With the current approach you don't need any conditional display logic if the thumbnail is missing. It is also relatively easy to override default thumbnails. If we'd go with a dedicated view mode we'd need to handle all this in display logic which is not nice in my opinion. This was one of the main reasons why we went down this path initially.
Comment #230
wim leersTo get this issue back on track/more focused again, here are my three major concerns:
MediaTypeconfig entity type@MediaHandlerpluginMediacontent entity typeand the responsibilities of each not being clearly defined (being address right now in the preceding comments). Plus, most having some weirdness:
MediaTypehaving thumbnail-related methods (I'd expect them to be@MediaHandler-specific);@MediaHandlerhas lots of weird things (see next point).It's easily dismissed as technical nitpicking. But remember that we have to maintain this for years to come. It's considerably more complex than any other entity in Drupal core.
EDIT: Finally, while writing all this, I realized one other severe limitation of this
MediaTypeconfig entity: what if I have videos on both Vimeo and YouTube? Or images both locally and on Flickr? What if I want to treat them the same in the UI, i.e. have the same "type or category" of media, but just have them be stored in different places? Then they'd have to have the sameMediaTypeconfig entity in the current architecture, but that's impossible. Won't that cause big UX problems too? I fear this will become a major usability complaint in the future.@MediaHandlerplugins in specific (which is being addressed right now in the preceding comments):SourceFieldInterface extends MediaHandlerInterface, but it seems all media handlers are implementing that interface, since it's also implemented byMediaHandlerBase.SourceFieldInterfacealso is a separate interface?auto_create_source_fieldstuff, and the associated fact that a getter may end up creating stuff. This affects the following:\Drupal\media\Entity\MediaType::preSave()\Drupal\media\Entity\MediaType::postSave()\Drupal\media\MediaHandlerBase::getSourceField()\Drupal\media\MediaHandlerBase::createSourceFieldStorage()+\Drupal\media\MediaHandlerBase::getSourceFieldStorage()+\Drupal\media\MediaHandlerBase::createSourceField()Having looked into this more, I think that all this "create source field" logic actually only belongs in
MediaTypeForm, i.e. in the UI code. Any developer that wants to deploy media fields, would do so by deploying configuration. So then the "automatic creation" simply is not necessary.AFAICT the only reason to have it be there, is to simplify certain tests (
\Drupal\Tests\media\FunctionalJavascript\MediaUiJavascriptTest::testMediaTypes()+\Drupal\Tests\media\Kernel\BasicCreationTest). But really, it's the job of those tests to create any necessary configuration. We should not be complicating the design of theMediaTypeconfig entity for that, nor should we be makingMediaHandlerBasemore complicated for it.I'd be happy to be wrong, of course! If I am, I'd like the rationale for this architecture to be thoroughly documented though, because it looks like it would be difficult to maintain.
Comment #231
berdirI was wondering about that too. That's what we did a while ago with the body field for node types. Only the Form and some test helpers create a body field when you create a new node type, and it used to be in postSave() or so.
Comment #232
wim leersWhile working on #230 I found another problem.
File@MediaHandler. You get to see this:The user can't do anything wrong. Yay :)
Image@MediaHandler. Now you get to see this:Now the user must make a choice:
Sadly, there's only one correct choice: . If you pick the existing field, you won't actually be able to upload images.
Yes, default configuration helps. But it's still very easy to get yourself in very weird situations.
I again point to my concerns about the
MediaTypeconfig entity +@MediaHandlerplugin.Comment #233
tim.plunkett#230.1 I agree with this 100%
#230.2 I can see this being confusing, it doesn't bother me quite as much. If the separation between MediaType/Media/MediaHandler was resolved, I imagine this would be less problematic.
#230.3 Also agreed 100%
Finally, this change made my IDE much less confused about what was happening in \Drupal\media\Entity\Media :)
Comment #234
jonathanshaw@WimLeers asked for a native English speaker to look at the MediaHandlers help text. I did this, changing the introductory paragraph completely and making a few grammatical corrections and clarifications elsewhere. Sorry for not providing this as patch and interdiff:
Media handlers provide the critical link between Media entity items and the actual
media itself, which typically exists independently of Drupal. Each media handler
works with a certain kind of media. For example, local files and Youtube videos can
both be catalogued in a similar way as Media entity items, but they need very different handling to actually display them.
Each Media type needs exactly one handler. A single handler can be used on
many media types.
Some examples of possible handlers are:
- File: handles local files
- Image: handles local images
- oEmbed: handles resources that are exposed through the oEmbed standard
- YouTube: handles YouTube videos
- SoundCould: handles SoundCould audio
- Instagram: handles Instagram posts
- Twitter: handles Tweets
- ...
Their responsibilities are:
- Defining how an individual media is identified. Handlers are not responsible for
actually storing the media. They only define how it is represented on a media
entity (usually using a field of some type), for example as a URL field for Youtube
videos or a File field for local files.
- Providing thumbnails. Handlers that are responsible for remote media typically
fetch the image from the 3rd party API and make it available locally.
Handlers that represent local media (typically files) will usually generate an image.
Both also fall back to a default thumbnail if it cannot be otherwise obtained.
- Validating a media entity before it is saved. The entity constraint system is
used to ensure the media entity has a valid structure. For example, handlers that
represent remote media might check the URL or other identifier, while handlers
that represent local files might check the MIME type of the file.
- Providing a default value for the media name field. This saves users from manually
entering the name when it can be reliably set automatically. Handlers
for local files typically use the filename, while a handler for a remote media
might for example obtain a title attribute through the 3rd party API. The name can
always be overridden by the user.
- Providing metadata specific to the given media type. For example, remote media
handlers generally get information available through the 3rd party API and make
it available to Drupal, while local media handlers can expose things such as
EXIF or ID3.
- Mapping metadata to the media entity fields. Metadata that a handler
exposes can be automatically mapped to fields on a media entity. Handlers are able
to define how this should be done.
Comment #235
wim leersDiscussed #230 during the media meeting.
#230.3: Everybody agrees with this direction: @slashrsm, @seanB, @phenaproxima, @Berdir. @phenaproxima has volunteered to take this on. WOOOT! One major concern will likely evaporate :)
#230.1: Specifically, this portion:
This capability is supported, in the following way:
Of course, this scenario/use case/solution needs to be documented. And the assumption that is associated with
MediaType(Interface)must also be documented.Also, it makes me wonder why not every media entity simply has a MIME type property?
#230.2: No actual progress on this yet. Here's the relevant portions from the IRC meeting. I can't really summarize this very well, because it doesn't have a conclusion yet.
That chat log has me saying I'll make a concrete proposal, because @slashrsm said he's not opposed. Posting that in my next comment.
Comment #236
wim leersHere's the proposal for #230.2 that I mentioned in my previous comment:
MediaHandlerInterfaceandSourceFieldInterface extends MediaHandlerInterface, but alsoMediaHandlerBase implements SourceFieldInterface, which means the base class isn't really a base class. @slashrsm saysSourceFieldInterfacecould be merged intoMediaHandlerInterface, but I don't think that'd make things clearer.getLabel(),mapFieldValue()andgetSourceValue()are useless. That's 3/11 = 27% of the interface surface area! And quite possiblyattachConstraints()too, then it's 4/11 = 36%.SourceFieldInterface::getSourceValue().)MediaHandlerInterface. Then yes,MediaHandlerInterfacewill still have the exact same end result, but at least it's now clearly partitioned.MetadataHandlerInterface. @slashrsm ruled out , and in #216.So for now, I'm going with
MediaStorageProxy.MediaHandlerMetadataInterface::getMetadata()needs the data stored in the source field (MediaStorageProxyInterface::getSourceField()) to return the correct value.MediaHandlerThumbnailInterface::getThumbnail()andMediaHandlerThumbnailInterface::shouldUpdateThumbnail()both need the data stored in the source field (MediaHandlerStorageProxyInterface::getSourceField()) to return the correct value.MediaHandlerMetadataInterfaceandMediaHandlerThumbnailInterfaceboth depend onMediaStorageProxyInterface.MediaHandlerStorageProxyInterfaceinterface as stand-alone things? That'd make it a lot easier to explain. Except that there would be a 1:1 relationship between an implementation of that interface, and the (required) implementations of the two other interfaces. Does that get us anywhere? Have we seen this elsewhere?It turns out we have: entity types have "handlers" for storage, access, form, route provider, translation, and so on.
@MediaStorageProxyplugins that implementMediaStorageProxyInterface@MediaStorageProxyplugin annotations to havemetadata(implementingMediaStorageProxyMetadataHandlerInterface) andthumbnail(implementingMediaStorageProxyThumbnailHandlerInterface) handlers (much likeEntityTypeplugins have handlers)oembed_providerhandler which would allow a Drupal site to become an oEmbed provider, allowing any Media to generate more advanced/detailed oEmbed data. Or for example anaccessible_thumbnailhandler which also provides alternative text (perhaps through image recognition, perhaps by fetching a remote description). Or atwitter_cardhandler.MediaHandlerInterface. But they're different areas of responsibility. This is what the entity system has developed the concept of "handlers" for. This is extremely similar AFAICT.Comment #238
tim.plunkettUsing annotations similar to EntityType's
handlersseems like a great approach to me.It allows for composition and follows an existing pattern.
I would go one step further and suggest that plugins be allowed to opt-into acting as their own handler, see \Drupal\Core\Plugin\PluginWithFormsTrait::getFormClass for an example (used by blocks).
MediaHandlerBase is the place to compose all of the most common interfaces, but I agree that each interface should be clear and have a single well-defined responsibility.
EDIT: Clarified my last statement
Comment #239
andypost@Wim #236 is great idea, lots of ++ for detailed research
The only question that raises here for me is do we need some method to detect applied "handlers" a-la
\Drupal\Core\Entity\EntityType::getHandlerClass()and where it can live?Tim also pointed interesting nuance about nesting for kind of thumbnail handlers like
\Drupal\Core\Entity\EntityType::getFormClass($operation)to provide this extendability with oEmbed and twittercardComment #240
slashrsm commentedAlready discussed this with @Wim Leers on IRC, but it is worth mentioning here that I really like and support his #236 proposal. Same goes with @tim.plunkett's and @andypost's suggestions.
Comment #241
wim leers#238: So you're adding to what I said: you're saying it's possible for the default plugin base class to implement the different kinds of handlers in one class. That's great: it's great that we have that option.
In this particular case, I'm not sure how helpful it is. It could be great. I suspect it may be great. But it could be that we feel it's clearer to have it in separate classes. I think once we refactor it towards handlers, that it will automatically become clear whether it makes sense to have it together or separate.
#239: RE:
EntityType::getHandlerClass(): indeed: we'd need to duplicate that fromEntityType. Which is fine. In the future, we could even put it in a shared trait.#240: WOOHOO!
Discussed with slashrsm in IRC. If we want to see #236 happen, somebody needs to do it. slashrsm and Berdir both already have obligations for most of today. My colleagues in the U.S. are still sound asleep right now. So we agreed the best course of action would be for me to take on this rerolling, and for slashrsm+Berdir+my colleagues to do reviewing. Because time is ticking.
So, assigning to myself.
Comment #243
wim leersFirst, removing
getLabel(),mapFieldValue()andgetSourceValue(). This reduces the API surface, and hence reduces the amount of moving I'll have to do in subsequent patches.(This made me notice that
getSourceField()is also incorrectly named, it should be namedgetSourceFieldDefinition()because it returns a\Drupal\Core\Field\FieldDefinitionInterface.)Comment #245
wim leersAll failures are due to
Field field_media_test is unknownbeing thrown inSqlContentEntityStorage.The only actual logic change in #243's interdiff is here, the
!empty($source_field)was removed. Because it should not be necessary.Turns out it is, and it's related to the
auto_create_source_fieldmagic.This now reintroduces this, but keeps it separate, with an explicit
@todoso that @phenaproxima can remove it as part of his work addressing #230.3 (see #235).Comment #246
wim leersNow a pure renaming patch:
MediaHandlerInterfacetoMediaStorageProxyInterfaceMediaHandlerBasetoMediaProxyStorageBase@MediaHandlerto@MediaStorageProxy(the plugin annotation)Plugin\media\HandlertoPlugin\MediaStorageProxy(the plugin namespace) — if people preferPlugin\media\MediaStorageProxy, that can be easily arranged.MediaHandlerManagertoMediaStorageProxyManagermedia_handler_info_alter()tohook_media_storage_proxies_alter()… plus updated docs everywhere.
This means that the following have not yet been updated:
MediaType(Interface)(plus associated configuration)Media(Interface)Comment #247
berdirSome notes on the removed methods below, other than that I think I'm OK with the direction, not sure I fully see yet how the handler stuff will look like exactly. Worth mentioning that some also critized that pattern, e.g. because they are not services. But we have plugins either way. We do have a few examples where we actually return service names instead of class names, for example for entity query factory in \Drupal\Core\Entity\EntityStorageBase::getQueryServiceName().
You do need to solve the problem problem of creating those handlers and dependency injection, and since they are not plugins, you can not use the ContainerFactoryPluginInterface, you can also not use EntityHandlerInterface as this is not an entity type, you can also not use the plain ContainerInjectionInterface as we do need to provide those handlers some context about what they are working with. Or maybe not, if we always pass them everything they need through their methods. If not, then we need Yet-Another-Injection-Create-Method-Interface. Yay ;)
In TMGMT, we do have something similar, mostly for historical reasons, where we have translation provider plugins and they have a separate class for their UI-related methods, basically a left-over of the 7.x architecture and while it is kinda nice to have all that clunky UI code somewhere else, interacting with it is actually pretty bad DX, for example because we never solved the dependency injection problem. What I'm basically trying to say is that while it sounds nice and clean on paper, you might end up with more (glue) code than you realize now.
Example use case for "mapping": You could map eg. the category of a youtube video to a term reference field, so you don't want to store the plain string, you want to look look up a matching term, auto-create if not existing and then assign the term object/ID.
That is still possible, you can implement that logic in getField(), it is a bit weird though, and we need to very clearly document what the return value is exactly (anything that can be assigned to a field.. including multi-property and multi-item array structures).
One thing that's no longer possible is doing anything dynamic based on the destination field. You couldn't support both a text field and an entity reference as destination, for example. And if you do anything like that, you also can't really validate the destination in any way (for example check the max length of text fields.. we frequently have troubles with imports that write too long node titles, or descriptions or so). Or check the target type of an entity reference field, because it's not going to go well if you save a term into a user reference field :)
At least parts of this can be done on other ways, we could give them a way to validate the field map or so. And we could maybe still pass $destination_field to getField(), then they could do basically the same in getField() as would have been possible in mapFieldValue().
That said definitely +1 to renaming source fields.
Note that this is not the same as before. Before you compared the property value (e.g. "foo".), now you compare the value array for the whole field (e.g. [0 => ['value' => 'foo']]). should still work if we never need this as an actual plain value.
I also agree with removing from the interface, makes sense, but maybe keeping as a protected method might make sense? This line is pretty long and complicated enough. that said, maybe let it return a plain value (so just "foo", and not a String typed data class with the value foo) as I see no benefit of going through typed data.
so here we need the text, and you see the difference here now nicely.
This can't use a protected helper of course, which is fine, but use $media->get($source_field_name)->value. Yes, magic, but actually faster and more readable.
Comment #248
wim leersThis splits off metadata-related methods from
MediaStorageProxyInterfacetoMediaStorageProxyMetadataHandlerInterface. This does not yet introduce ahandlersannotation onMediaStorageProxyplugins. That's for a next step.(What's written here expands on my analysis of metadata-related methods that I wrote in #236.)
In doing so, I noticed that the existing method returned both metadata labels (using the
'label'key) and optionally anything else: an ArrayPI. The only other thing besides'label'that is documented is'field_type'… but nothing uses that, nor is there test coverage.Furthermore, returning
'field_type'there is also needlessly tying this interface to Entity Fields. It should be solely about media metadata. If in the future you want to have suggestions for particular field types, add ametadata_field_suggestionhandler.If we wouldn't remove
'field_type', then we would also have to document the boatload of assumptions inherently made and not mentioned anywhere: media metadata can only ever have cardinality 1, behavior in case you only set up a metadata<>field mapping after you've created metadata is undefined, behavior in case of optional metadata is undefined, behavior in case of required fields is undefined, and so on.We can avoid all that by focusing this handler narrowly on one thing: documenting which metadata attributes exist, their labels, and the ability to retrieve them.
Which brings me to the final point: (which was in the patch so far) is a terrible name because it's so easily confused with Entity Fields — even more so because these (metadata) "fields" were being mapped to Entity Fields. I also ruled out "properties", because that's another term we use in Entity Field API, i.e. for fields that contain multiple properties (e.g. text + text format). That's why I went with , which seems the best fit, also considering it's the most "web like" (because HTML elements have attributes).
Of course, this new interface has comprehensive documentation, so that hopefully it's crystal clear to everybody what its responsibilities are.
Comment #249
wim leers#247:
Yes, there are definitely downsides to having "handlers" the way entity types do them. But at the same time, can you imagine what it'd have been like if the
Nodeclass included all the code in all of its handlers? Tagged services may be a possibility, but is entirely new ground: a plugin type with associated service tags, and hence plugin instances with associated tagged services is not a pattern that exists in core.RE: dependency injection: that may very well become a problem. But the first one (see #248) didn't need anything to be injected at all. Thanks for your advance warning — I really appreciate that!
Overall, I agree with your concerns/warnings for carefulness. The fact remains though that
MediaHandlerInterfacewas really just a dumping ground for lots of responsibilities. Which explains why it's so darn hard to understand, describe and document.RE: advanced use cases: like I explained already, when those use cases arise, we can add additional handlers to support those advanced capabilities.
I hope you like what I did in #248.
I considered keeping
getSourceValue()as a protected method. But the fact is that it's duplicating part of the logic ingetSourceField(): it also reads$this->configuration['source_field']and it also throws aRuntimeException. Plus given what I wrote above in response to this point: it's only comparing a subset, which is not quite correct.Although I have to say that this particular assertion is rather pointless. It used to test
getSourceValue(). IMO it can be removed. Or we could just test thatgetSourceField()works correctly. I figured I'd keep it just to ensure we keep the test coverage equivalent.Comment #250
tedbowJust a couple nits for now
Unused local var
Unused local var
$error doesn't need to be declared here. Will always be set in the else clause if used.
Unsued imports
Comment #251
berdir> Would you believe me if I said I googled for 10 minutes without finding a helpful response on how the hell to get that value?
I totally do :(
Entity API documentation--
Berdir--
That said, it's a somewhat tricky term to search for, there *is* some documentation
* https://wizzlern.nl/drupal/drupal-8-entity-cheat-sheet
* http://drupal.stackexchange.com/questions/221103/access-entity-reference... (And a ton of other similar questions/answers)
* https://www.drupal.org/docs/8/api/entity-api/entity-api-implements-typed...
Comment #252
wim leersThis moves the thumbnail-related methods into a separate interface. No big explanation here like for #248, because it's a simple move. The only noteworthy thing is that I added extensive docs to this new interface.
(And fixes #250.4. The other points in #250 I'll leave to somebody from the media team to fix — I'm not touching that file.)
Comment #253
wim leersThis patch:
attachConstraints()togetConstraints()and updates the documentation to be far more specific (looking at\Drupal\Component\Plugin\Context\ContextDefinitionInterface::getConstraints()as an example)@todoto ensure this patch will not be committed without test coverage, because the patch so far has failed to provide that —FileandImageplugins should be updated so they add constraints!MediaStorageProxyBase(previouslyMediaHandlerBase), to force developers to think about the constraints!Comment #254
wim leersThis patch:
SourceFieldInterfaceand moves its sole method intoMediaStorageProxyInterfaceMediaStorageProxyInterface: it documents the complete architecture. That will be the test: does this actually make sense?Of course, there's one critical remaining step before that documentation makes complete sense: we still need to actually make
@MediaStorageProxyplugins use handlers. In any case, even just having separate interfaces already makes this a lot more understandable, in my opinion. I'd love to hear if others agree.Finally, I think that
getSourceField()is itself also still fairly confusing, but I think that once @phenaproxima addresses #230.3, it'll become possible to clarify/simplify it further.Comment #255
wim leersWe now have both
\Drupal\media\MediaStorageProxyThumbnailHandlerInterfaceand\Drupal\media\MediaThumbnailHandlerInterface.That's confusing.
\Drupal\media\MediaThumbnailHandlerInterfacedidn't stand out before, but now it does. NOFI, but it's incredibly terribly named. Again that "handler" suffix! I didn't even notice before because it wasn't as important, but now that@MediaHandlerare more understandable, this really stands out. It's called by theThumbnailDownloader@QueueWorkerplugin.I wanted to make renaming suggestions, but then I realized… why is this even a separate interface+service? There doesn't seem to be any reason to have different logic? And if there is, this seems like a premature abstraction.
So it is a separate class (and hence interface+service) because it's called both from
\Drupal\media\Entity\Media::preSave()and\Drupal\media\Plugin\QueueWorker\ThumbnailDownloader::processItem().\Drupal\media\MediaThumbnailHandler(Interface)can be removed quite easily AFAICT:$media->thumbnail->…setter logic from\Drupal\media\MediaThumbnailHandler::updateThumbnail()intoMedia::preSave()setThumbnail()method toMediaInterface, and move the "create File if it does not exist" logic from\Drupal\media\MediaThumbnailHandler::updateThumbnail()in there@MediaStorageProxyplugin'sgetThumbnail()method from\Drupal\media\Plugin\QueueWorker\ThumbnailDownloader::processItem()and let it pass the result of that toMediaInterface::setThumbnail()And voila, one service less, again less API surface, and code that is much easier to understand.
Comment #256
wim leersWe just had a meeting with @phenaproxima, @effulgentsia, @Gábor Hojtsy, @tedbow, @tim.plunkett, @sam.mortenson and I. In other words: a subset of the folks working on Media.
We reviewed the progress made above today. We reached a few conclusions, which we'd like to see confirmed or rejected:
getDefaultName()can be moved into the metadata handlergetConstraints()needs to receiveMediaTypeconfig entity (because e.g. oEmbed@MediaStorageProxy, but restricting to just YouTube for this particularMediaType)getConstraints()is always applied to the source field, see http://cgit.drupalcode.org/media_entity_instagram/tree/src/Plugin/MediaE... + http://cgit.drupalcode.org/media_entity_twitter/tree/src/Plugin/MediaEnt... — therefore it should be renamed togetSourceFieldConstraints()getSourceField()andgetSourceFieldConstraints()and that's all that's left on the plugin interface. It makes sense to move it to a separate interface (yes, again), and then have an "empty" plugin interface, and have everything be composable via handlers.@MediaStorageProxyto@MediaSourceEverything except the last point is done, except for docs updates.
Unassigning. Hopefully Tim can push the handler stuff forward.
Comment #258
wim leersUnfortunate that nobody was able to push it forward while I was asleep. But ah well.
Pushing it further. @seanB +1'd this direction, and also #256's suggestions:
@slashrsm will be reviewing it in a few hours.
Progress!
Comment #259
wim leersFirst making the patch green again.
I forgot to say that #256 would fail because I rolled that patch during the meeting it documented, and I was unable to run tests before having to leave. I'm surprised I managed to only break one test, instead of many.
Comment #260
wim leersI forgot to write in #256 that we also discussed
\Drupal\media\MediaStorageProxyThumbnailHandlerInterfaceand\Drupal\media\MediaThumbnailHandlerInterface, i.e. the point I raised in #255.We:
media.thumbnail_handlerservice is kind of pointless: i.e. we agreed we'd refactor that service awayalt, but it should also be possible to sendtitle,widthandheight. Because the current "media thumbnail handler service" is hardcodingalt = $this-t('Thumbnail')andtitle = $media->label(). In other words: we either need aThumbnailvalue object that requiressrcandalt, and allows all other attributes to be optionally set, or we need to rename the existing method fromgetThumbnail()togetThumbnailUri(), because today it only returns an URI. We may want to choose the latter approach if we want to keep thealtoptional, i.e. if we want to move that to a separate handler.(I hope that mostly captures it, @effulgentsia and others, please correct if wrong/incomplete.)
EDIT: this was not discussed in that meeting, but AFAICT the
shouldUpdateThumbnail()method is also pretty pointless. It's only called from bothMedia::preSave()andMedia::postSave(), always has the same logic, and could therefore easily be moved into theMediaentity. For local media, pre-save calls$plugin->getThumbnail(), post-save does nothing. For remote media, pre-save always calls$plugin->getDefaultThumbnail()and in post-save it queues the proper thumbnail to be fetched.Comment #261
slashrsm commentedThank you @Wim Leers! I like the direction this is going to. Architecture is much clearer now. There will be some more work for contrib plugins to catch-up, but I think that it is worth the effort.
Re dependency injection on handlers: I think that we'll need to solve that as there are definitely dependencies needed on them. The simplest example I can come up with is
\Drupal\media\MediaStorageProxyThumbnailHandlerInterface. Most (if not all) of them rely on a configuration object to figure out where thumbnails should be saved locally - config factory is needed. There are many more examples like this. Could we useDrupal\Core\DependencyInjection\ContainerInjectionInterface?This changes the behavior as the old function expected handlers to attach constraints to the entity or any of its fields. Then
Entity::validate()automatically picked them up and validated them.New version expects handler to return the list of constraints and currently we don't do anything with them. Old approach was a bit more flexible since handlers were able to use entity-level or field-level constraints. It seems that the new approach only supports the latter.
@dawehner pointed at that we might have a problem if the actual metadata value is FALSE. Since we're already revamping this it might be worth looking into that and finding a better way to indicate that a given metadata field is not available.
I think that this would make sense and would also help us remove the
alt = $this-t('Thumbnail')andtitle = $media->label()nonsense. Could we do this through Typed Data?Agreed. The main reason for this service was to be able to use this code on both the entity class and queue processor. I am not entirely sure where to move the logic. Currently we only set the thumbnail if the a) field is empty or b) it is updated from a queue. This is fairly limiting and there were reports already complaining about thumbnail not being updated etc. I can foresee this becoming smarter in the future. We could move responsibility for the decision when to update to the thumbnail handler, which would allow for smarter logic than the current "only if emtpy". Another thing that I can see being added is something on the media form, which would allow content editors to force thumbnail update.
We need to structure this in a way that will allow us to achieve this kind of things in a nice and meaningful way.
Comment #262
wim leersTL;DR
I'm proposing a
@MediaSourceplugin (name suggested in #256), without handlers, with only 3 methods, and many prior methods' responsibilities being implemented in the form of annotation keys that point to particular metadata attributes.@Berdir was prescient in #247 when he said:
The choice in #256 to move the "source field" stuff to a separate handler too only makes this harder, because both other handlers (metadata and thumbnail) need the source field. Because that means that the metadata and thumbnail handlers both need to receive (get injected) the source field handler… or at least be able to access the
source_fieldconfiguration.Contrast this with entity handlers; they only are able to access what's in the plugin definition (i.e. in the plugin annotation).
See the attached
do-not-testinterdiff to see what that code started to look like… even before I was passing in the necessary context (at minimum, the source field configuration) or providing dependency injection. It'll start to look significantly more complex once all handlers are receiving that too.This got me thinking.
@MediaSourceas the new proposed plugin name makes me think that perhaps having all three responsibilities (source field, metadata and thumbnail) perhaps can live in the same plugin and the same plugin interface. Until #236, what these plugins were doing was a lot more vague:@MediaHandleras the plugin name is as vague as is possibleshouldUpdateThumbnail()like I suggest in my last comment, it'll be 7getDefaultName()andgetThumbnail(). Because… they're actually metadata too!. For example, look at the Instagram API response, and how the Instagram plugin for Media handles this: http://cgit.drupalcode.org/media_entity_instagram/tree/src/Plugin/MediaE... + http://cgit.drupalcode.org/media_entity_instagram/tree/src/Plugin/MediaE.... It's literally reusing metadata. So then why not get rid of those methods, and specify in the plugin annotation which metadata attribute to use? Now we're down to 5 methods.getMetadataLabels()into the annotation as well, since it really is just a hardcoded key-value mapping anyway! Now down to 4 methods! Let's turn it into two examples:Image+Instagram:getDefaultThumbnail(), because it is really just returning the path/URI to one particular image file. Further indication that this makes sense: it receives no parameters.The tricky thing there is that we have no precedent of referencingor perhaps:
I hear some of you say: , but I don't think that's true. Thanks to all this refactoring, I think we have come to an important realization: these plugins really are only about two things:
I came to this realization thanks to two big shifts from #236 until this point:
Framed in this way, I can explain what the responsibilities of a
@MediaSourceplugin are (this is what could go in the interface classdoc):Thoughts?
Comment #263
wim leersWe've been discussing #262 in IRC.
Takeaways:
@MediaHandlerwas before, without handlers, so less divergence from the existing contrib API. We're "just" refactoring a bunch of methods away.interface CKEditorPluginButtonsInterface extends CKEditorPluginInterface— this is feasible once we no longer have 11 methods)\Drupal\ckeditor_test\Plugin\CKEditorPlugin\LlamaContextualAndButton+\Drupal\ckeditor_test\Plugin\CKEditorPlugin\LlamaContextual+ …. (The remark was made that many experimental modules landed with more test coverage than this stable module would have had.)So at this point, it's clear that my reviews have been incomplete/inaccurate, because I didn't look at the ways that every existing
@MediaHandlerplugin in contrib has been exercising this API. So I'm not aware of all the ways that may be possible. So I drew overly simplistic conclusions.To fix that, we need to add that missing test coverage proving that all those advanced use cases work. And then we can have actually informed reviews, not too simplistic ones.
In other words: this is not feasible to do before 8.3, I think.
Comment #264
slashrsm commentedBefore proceeding with this patch we need to decide which overall architectural approach we're taking (a) initial MediaHandlerInterface, b) StorageProxyInterface with handlers or c) the last one with heavy use of annotations). It is not worth spending any time on adding additional tests before this decision is made. Adding more code to this patch will made future rewrites even harder and we should avoid that.
c) is not feasible in my opinion since it removes a lot of flexibility which essentially media entity is all about. b) is nice but we have knowledge sharing problems between storage proxy and handlers. We could solve that by sharing all the information between them. Since that would make them mostly inter-dependent we lose the main benefit of this approach - clear separation of concerns. a) is also not ideal, but we have to be aware that is the only approach that is tested in real life. Thunder, Lightning and many other productions projects are using it. It is stable (as in 1.0) since spring 2016. There was no reports in last 2 years about any major flaws. Just minor things that we were able to easily resolve. If we fix minor problems in it and update documentation it could be the best option that we have.
Comment #265
wim leersHow are we going to land on the right architecture if we don't know which use cases we need to support?
I'd be fine with throwing away all the work I did, going back to
@MediaHandler(a) and add the test coverage to it. My work may or may not be a dead end. That is fine :)(Also forgot to unassign myself in #262.)
Comment #266
slashrsm commentedWe tried, came up with two alternative solutions and realized at the end that they are at least as bad as the original one. That says something too, right?
Comment #267
wim leersIndeed! Which is why I said:
https://www.drupal.org/node/2825215#comment-11878451
Or, alternatively, starting from before my changes in #236, and just renaming the plugin type to MediaSource, renaming the methods (e.g. "Field" → "Metadata") and adding docs and tests.
Also, please know I did this with the best of intentions. The pain we are going through with the rest and serialization modules and their APIs that seemed solid and inspired on contrib modules is something I'd like to spare you. Those modules also worked fine in the seemingly broad use cases that we tested in core.
Furthermore, I am reading and hearing +1s for the concerns I raised, so likely core committers would raise similar concerns.
Also, I almost only had remarks on the plugin type, most other things I didn't say anything about — so it's not like I'm shooting the entire patch down!
The best course of action here is *adding tests*. I can't stress enough that if there were tests exercising the advanced use cases that my proposals are making impossible, that I'd never have made those proposals! So, really, ignore the majority of what I said. Add tests. Then ask for reviews again.
This patch would never have been committed to core with the current test suite.
P.S.: wrote this from my phone, forgive typos and lack of formatting.
Comment #268
dawehnerThank you @Wim Leers for your hard effort to try to simplify things.
Media already has a long history in Drupal. People have been working on media related functionality since basically forever. This resulted in the amazing work of the media team in Drupal 8, with all of the contrib modules out there. The result is a huge range of fundamentals, like inline entity form/entity browser, but also more important here integrations into all systems.
This here is for me the major difference to REST and serialization. Unlike REST and serialization, the media suite, with all its implementations (from twitter, facebook, soundcloud) already have running solutions on live sites, while these modules got developed. This proves that the given usecases of those modules haven't been just academical. On the other hand we had REST/serialization which came from a high theoretical background and then have been thrown onto people, who struggled with them. IMHO the verbosity of HAL for example would have been uncovered way earlier, when people would have used it on a day to day base.
Comment #269
gábor hojtsyLet's start with drawing up what we have then. This is not all the classes, there are lots of form handlers and actions and views integration, etc. but those were not in question. So focused on the main architectural pieces. We probably need to improve this picture to be more expressive of the interactions between how responsibilities are shared/delegated, or we may want to draw another figure for that that does not involve all the class hierarchy.
Google doc at https://docs.google.com/drawings/d/15U9pdSSSUjLlOshpvKf_i5PVVhlFK7_xTKbR...
Comment #270
wim leersTo clarify, as far as I'm concerned, the amount of work or change does not need to be big at all (or not as big as it's been sounding lately):
@MediaHandler->@MediaSource,getField()->getMetadataAttribute()etc.)auto_create_source_fieldand mergingSourceFieldInterface)Much of point this is trivial. Much of it can be lifted from the interdiffs I posted.
If you write Functional tests then those tests won't need to change at all when architectural details change.
Functional tests can be written regardless of any architectural change we make. That's what needs to happen now to move this issue forward IMHO. I can't do that.
Comment #271
gábor hojtsyI agree that the field vs. metadata problem is pretty real in the API, and needs to be rectified. I had the same complications understanding it due to clashes with Drupal terminology.
Also wanted to point out that @Wim Leers is not a framework manager, so framework managers could still raise further concerns or disagree with Wim (or agree with Wim). I think @effulgentsia took this on himself to provide that kind of feedback so far and will still need to follow up on the latest discussions. For now he is as far as I know focusing on things that do have a deadline this week to get in before the 8.3 alpha release.
Comment #272
slashrsm commented#270 sounds good to me.
I am off and mostly offline this week. I will be back and focusing on this after I am back. However, I do not want to block anyone so feel to continue if anyone has spare resources to dedicate.
Comment #274
gábor hojtsy#2835861: Discussion: Expose a path field for media entities concluded with adding the path field. I would love to add it here and close that (it is a one line change), but since its not clear which direction to work off of yet, holding off on it for now.
Here are relevant meeting snippets from the media meeting today:
Comment #275
phenaproximaHere's an interdiff that removes auto_create_source_field entirely. I can post a full patch if needed, but I'm not sure if I should, considering how much major refactoring is going on. With these changes applied, all media tests are passing on my localhost.
I had to axe a bunch of test coverage that no longer makes sense in the absence of auto_create_source_field. I added a bit of new coverage to the Media UI functional test to compensate, but we could probably add more.
The crux of the change(s) here is this: media types are no longer responsible for ensuring the existence of the source field. The handlers are responsible for loading the source field, or defining it if it doesn't exist; the MediaTypeForm, or other calling code, is responsible for saving the field and field storage definitions.
EDIT: I apologize for the use of warts like
method_exists($handler, 'getSourceField'). For some reason, I was having test failures when I tried to do$handler instanceof MediaStorageProxyInsanelyLongWhateverSourceFieldSupercalafragilisticexpialidociousInterface(somehow, that interface was not being loaded), so in light of the other refactoring that's happening, I took a shortcut.Comment #276
seanbHere is a list of all the MediaHandlerInterface/SourceFieldInterface methods and the contrib modules implementing them. I basically came to the same conclusion as #2831274-236: Bring Media entity module to core as Media module.
Summary:
The remaining work I see when using the patch in #245.
Some more details:
getLabel()
None of the contrib modules providing MediaHandlers/MediaSources implement this. This basically means that the annotation should be enough and you can fetch it from there as suggested in #2831274-236: Bring Media entity module to core as Media module.
getProvidedFields()
Basically all contrib modules implement this. Most of the times it is empty or a static list. Some modules have conditions though, eg media_entity_instagram, media_entity_twitter provide a extra option on the configuration form to use the instragram/twitter API. When using the API you get more fields to configure. We could expand documentation and provide a test for this. Renaming to getMetadataDefinition or getMetadataLabels makes sense.
getField(MediaInterface $media, $name)
Same as previous. Renaming to getMetadata makes sense.
attachConstraints(MediaInterface $media)
This is implemented (at least) by media_entity_instagram, media_entity_slideshare, media_entity_slideshow and media_entity_twitter. As slashrsm suggested in #2831274-261: Bring Media entity module to core as Media module it is good to be able to add constraints on entity and field level. For example media_entity_slideshow adds a contraint on entity level (although I think there is room for improvement in this example). Some concerns were raised in #2831274-236: Bring Media entity module to core as Media module, but I think it safe to assume that developers that create constraints need to know something about the Entity API. My suggestion would be to expand documentation and provide a test for this.
shouldUpdateThumbnail(MediaInterface $media, $is_new = FALSE)
This method was discussed between #180 and #187. Some media thumbnails will never change, some will change more often, most will depend on the source field value. Specially when dealing with external media sources it is good to be able to influance this. Since it is introduced in this patch, no contrib modules implement this yet. We could move this to the Media entity? My suggestion would be to expand documentation and provide a test for this.
getThumbnail(MediaInterface $media)
All contrib modules implement this in various ways. Some use a metadata field, some use mime types or the source field. I think this one should stay for sure.
getDefaultThumbnail()
Most contrib modules implement this, but it could be moved to a annotation. It basically returns $this->config->get('icon_base') . '/[source].png'; all the time. There could be a case where you want to fetch an image from another location, not the icon base dir. My suggestion would be to expand documentation and provide a test for this.
getDefaultName(MediaInterface $media)
All contrib modules implement this in various ways. Some use a metadata field, some use the source field or a value of the entity related in the source field. I think this one should stay for sure.
mapFieldValue(MediaInterface $media, $source_field, $destination_field)
No contrib modules implement this. It is only used in 1 place. I think it's safe to just move the code there and remove this method.
getSourceField(MediaTypeInterface $type)
No contrib modules implement this, but this method is actually important. I think this one should stay for sure. Renaming to getSourceFieldDefinition makes sense.
getSourceValue(MediaInterface $media)
No contrib modules implement this. It is only used in 1 place. The function docs speaks about possibilities for edge cases, but contrib doesn't provide any examples of this. I think it's safe to just move the code there and remove this method.
Comment #277
seanbIn the end I based the patch on #229 and tried to fix everything that was mentioned after that. Applied (parts of) the patches posted and fixed some left over comments.
Still to do:
Not sure where to move MediaThumbnailHandler, the Media entity might not be the best place. Suggestions are welcome. Also adding test coverage should be done once we agree that the MediaSourceInterface is correct now. Hope to get some feedback on this, I can move forward tomorrow.
Setting to 'Needs review' to get some feedback.
Comment #279
wim leers#275: that looks splendid :) Thanks!
#276: It's interesting that you arrived at the same (or at least similar) conclusion even after inspecting all media contrib modules. Because @slashrsm has been saying that my conclusions were incorrect, because more flexibility was necessary? Of special interest:
attachConstraints()is necessary, and needs to be able to add constraints to both entity and field (happy to be wrong there!).getDefaultThumbnail()always uses a static (hardcoded) return value, yet you don't suggest moving it to the annotation?Overall, I'm glad to see that you're validating my analysis, but then also refining it further. It's always reassuring when two people independently arrive at very similar conclusions :)
Also: impressive that you analyzed all media contrib modules! Thank you so much, it's a huge contribution to this issue and initiative!
#277:
If #276 was impressive, this one is even more impressive. I think the correct technical term is:
HOLY SHIT. This is one hell of a way to get this back on track!Basically +1 to #277 — it's a huge step in the right direction, basically incorporating EVERYTHING since #229! Once again: holy shit-level impressive work. Thanks so much.
I also agree that the most likely cause for the test not passing (but wow, only a single fail!) is a small problem in @phenaproxima's interdiff in #275. Pinging @phenaproxima to hopefully fix that :)
Finally, a review of #277:
s/$name/$attribute_name/ for slightly better clarity.
I'm still concerned about this actually attaching constraints. Would it not be better to return both entity-level and field-level constraints, and then let the Media module take care of the actual attaching?
Regarding this and the associated todo you mentioned (): I think comment #260 — which is posting the conclusion of a discussion — is still a sane way to go with this.
I still think we can remove this in favor of moving this into
getMetadata(Attributes). Usually this is literally returning some metadata attribute. Where that's not the case, this would be a computed metadata attribute.(I wrote about this before in #262.)
Either way, just allowing the annotation to specify which metadata attribute to use for the thumbnail + default name would reduce the API surface.
I don't feel strongly about this — just making sure that is considered.
See earlier in this comment: why can't this be an annotation?
This is not actually a field definition. Because that would be a
\Drupal\Core\Field\FieldDefinitionInterfaceinstance.So this name still needs some refinement.
Comment #281
webflo commentedI tried to debug Drupal\Tests\media\Functional\MediaCacheTagsTest and i could not run it via phpunits, it is actually a simpletest in the wrong namespace.
Comment #283
webflo commentedComment #285
seanbThanks webflo, for fixing the tests! Just finished working on 279.
279-1. Done
279-2. Still todo
279-3. Done. Not every media source needs to update the thumbnail all the time. Probably only media sources that use images as a thumbnail (image/youtube/instagram) will need this. When we move this to the Media entity as suggested in #260, we lose to possiblity for Media Sources to decide when to update the thumbnail. I tried to solve this with a new annotation. I think this is all the flexibity we actually need. Media entity in contrib never updated the thumbnail anyway.
279-4. Done. After thinking about this a bit more, I started implementing this and now see changing this doesn't remove flexibility, it just moves some logic to getMetaData. I like it!
279-5. Done, while having the ability to use another folder is flexible, it is not used in contrib at the moment. So it's an edge case we probably won't need. Another option is to also move this to a meta data attribute, but I think it's ok now.
279-6. It returns a configurable field definition (which is still a field definition). So the exact name would then be getSourceFieldConfigFieldDefinition or getConfigSourceFieldDefinition. Which also means we might have to think about renaming createSourceField to createConfigSourceField. I prefer to keep it as it is, the current situation is easier to read and understand. But if we must change it, getConfigSourceFieldDefinition and createConfigSourceField are my second choice. Thoughts?
To sum up, still to do:
Comment #286
seanbAlso removed the MediaThumbnailHandler and moved the code to the Media entity. That seemed to be the most logical place to do this.
BTW I moved the MediaCacheTagsTest back to it's original location since the namespace was not the problem. All the tests are now in 1 place again.
Comment #288
gábor hojtsyAdding collection label as per #212 above, resolving one of @effulgentsia's concerns.
Adding path field on media entity as per #2835861: Discussion: Expose a path field for media entities.
Comment #289
slashrsm commentedImpressive work @seanB! Thank you (and all others!) for all your help pushing this forward. Just a few comments:
shouldUpdateThumbnail()
I actually think that we should update the thumbnail every time when source field value changes. I don't think that we need annotation value to enable that. If source field changes it is extremely likely that the asset changed significantly which will result in a new thumbnail.
There is one case that is not covered now when we removed this function. Example: YouTube video, thumbnail was fetched from their servers, it changes upstream. This won't result in the thumbnail being updated. However, to cover this case we'd need to issue an API call whenever a given media entity is updated which we wanted to avoid.
I think that current approach solves 80% case and that there are still ways to solve problems like the one described above in contrib.
getDefaultThumbnail()
This is always static and can be an annotation. It is already done in the patch so basically +1-ing it.
mapFieldValue()
The reason this is not used in existing contrib modules is that it was added in this patch. Yes, we didn't add tests. Yes, I take responsibility for this. We should have done it better.
UX team agreed that we improve mapping UI in a follow-up. This function would give us enough flexibility to be able to do that properly.
The reason to add this was the fact that we have some limitations in the current approach. They were identified based on the contrib experience with the module and we wanted to solve that:
getMetadata()is aware of this it can prepare metadata value in a right way. I don't think thatgetMetadata()should care what the destination of the value will be.I was thinking about this a bit more and it seems that we could use a separate plugin type to solve this problem (
MediaMetadataMapperor something along those lines). It is true that it isn't the job of the MediaSource to solve mapping. By introducing a new plugin type we separate responsibilities and clearly document them.It is also not needed to introduce this new plugin type in this patch. We could do it as part of the mapping UI follow-up or even in a separate issue that would be a requirement for UI one.
Would love to hear thoughts about this.
attachConstraints(MediaInterface $media)
I agree with @Wim Leers that it is not very nice that developers need to add constraints. It would be much nicer if they would only return them. However, I'd like to keep the possibility to attach to both levels (field and entity).
Two solutions come to my mind:
getEntityConstraints()andgetSourceFieldConstraints(). Since we're trying to lower the number of functions this isn't the best solution I guess.Any other ideas about this? Thoughts?
MediaSource testing
Yup, we need to improve this. We need to cover more complex use-cases related to it. I can volunteer to look into his in the next few days. I will assign the issue to me when I find time for that. Feel free to grab it if anyone finds time sooner.
Comment #290
seanbshouldUpdateThumbnail()
Removing the annotation sounds fine. If a MediaSource has a hardcoded value the overhead is minimal.
getDefaultThumbnail()
Yay!
mapFieldValue()
Thanks for your input. Sounds like good arguments to keep/improve it. About the extra plugin type for metadata mapping, should we create mapping plugins for source field types (for example taxonomy fields)? It would help if we can come up with a list of mapping plugins and what they are supposed to do.
attachConstraints(MediaInterface $media)
How about moving this to the annotation?
Comment #291
boobaaTried to jump on this train early by adding a MediaSource plugin to our Brightcove module. We'll need a source field with the entity_reference type (
brightcove_videois an entity type in Drupal that stores info about the videos hosted on Brightcove). So I addedallowed_field_types = {"entity_reference"}which is the only thing that makes sense here, I guess – but I cannot find any way to provide additional required info: the entity type this field must reference.So I think there should be a way to provide this additional info, probably in the annotation, but as an optional thing. Something like
settings = { "target_type" = "brightcove_video" }would do in our case, which would mean "field configuration needs to include these configuration values" (per IRC discussion with @slashrsm). Rationale: there are some field types which have required settings, so the MediaSource plugins should be able to provide those settings.Not sure if this warrants a "Needs work" status, tho.
Comment #293
seanbSo after discussing this on IRC it seems the following is the case (@boobaa/@slashrsm please correct me if I'm wrong)
This makes the list of things to fix:
I think this example shows we need to start porting other media source modules as well. This might bring up more issues?
Comment #294
seanbWe just discussed the issue of boobaa in #291 on IRC. The following is a summary of what was discussed. Please correct me if I'm wrong!
Boobaa suggested adding settings through annotations. Allowing field settings for the source field through an annotation could remove the need to override createSourceField() in #2. The settings could at the same time be used to filter the list of source fields and remove the need for #3. Locking auto created source fields by default could remove the need for #4.
The annotation's look like the easier solution for module developers. Any thoughts?
Comment #295
seanbJust had time to take another look at this. Attached is a patch with the following changes:
Please review! We haven't discussed everything in depth, but I hoped this would help move the issue forward. I think we should start writing tests when we are sure what the MediaSourceInterface would look like.
Comment #297
Bojhan commentedI am wondering here - do we really need to have a top level link next to 'add content' to "add media" ? I dont think the 80% case will be to separately add it, most people will do it through the content creation screen and those that really do media management will be pointed to content > media.
Comment #298
gábor hojtsyBojhan I think you mean shortcuts in the menu? I don't think this patch is adding that (after searching through the patch again). The patch *does* add an Add media link on the media admin page itself (similar to admin/content's Add content link).
Comment #299
seanbIn #200 effulgentsia mentioned that one of the nice things provided by the media module is the separation of adding and referencing media and also provides a use case for this scenario.
Also a new patch because I broke the tests...
Comment #301
wim leers#285:
Review:
This is not yet handling attributes other than
'default_name'or'thumbnail_uri'.So much simpler! :)
Nice work!
Review:
Why does this need to be public?
EDIT: oh because it needs to be called from
ThumbnailDownloader.I'm wondering if we should make
updateThumbnail()protected and add a separate public method without this$from_queueparameter?Because the only reason to call the method with
$from_queue = TRUEis if it's a call from outside theMediaentity class, which means that that is what we actually need to live on the interface.#287 + #288: nice!
#289:
shouldUpdateThumbnail()This sounds great!
getDefaultThunbnail()mapFieldValue()getMetadata()should not not care what the destination of the value is. Updating mapped values seems quite important — although to solve this, it seems we also need to track some "remote timestamp" or "remote version", which we don't do currently.All of this is arguably advanced functionality. Can we keep the current architecture with its simplistic assumptions, and allow a
@MediaSourceplugin to optionally implement additional interfaces to support those more advanced use cases? Or is that not desirable?My point is: I'm not so sure we need a new plugin type for this. We can solve this by allowing our existing plugin type (
@MediaSource) to have a base interface, and allowing specific plugin implementations to choose to opt in to advanced functionality. This is what the@CKEditorPluginplugin type already does: by modeling CKEditor plugins onto Drupal@CKEditorplugins, we can reason about them inside Drupal. The base plugin interface doesn't do much, but not all CKEditor plugins need the same integration.CKEditorPluginInterfacecovers the basics/essentials.\Drupal\ckeditor\CKEditorPluginButtonsInterfacemust also be implemented if it provides buttons that user can enable for their toolbar.CKEditorPluginConfigurableInterfacemust also be implemented if it provides a configuration UI in Drupal. Both of those interfaces extend the basic plugin interface.I think a similar approach can work here?
But regardless of approach/architecture, what is most needed for this is test coverage to prove that the architecture works. As a side effect, we also have at least one implementation, which means you can get at least a basic feel for how it ends up working/looking code-wise.
attachConstraints()MediaSourceWithEntityConstraintsInterface extends MediaSourceInterfaceandMediaSourceWithFieldConstraintsInterface extends MediaSourceInterface. That makes both methods optional, and any@MediaSourceplugin can opt in to provide them.#293:
Is it necessary that the source field is editable?
#295: see the suggestions I made in my reply to #289.
Comment #302
seanb#300
Not exactly sure what you mean? Should it handle more by default? You are supposed to override this right?
We can lock all source fields storage configuration the module creates, but you can re-use existing fields as well. Maybe we can lock storage config for all fields selected as a source field when saving a media type? Since not locking fields could cause issues we should probably do this.
MediaSourceFieldConstraintsInterface extends MediaSourceInterfaceandMediaSourceEntityConstraintsInterface extends MediaSourceInterfaceIf we can agree on fixing the more complex mapping in a followup, I think the last thing to do is tests.
Comment #305
slashrsm commentedThis should fix the test fail that happened in #302. It was caused by #2851635: DefaultSingleLazyPluginCollection retains stale instance IDs. Before that patch DefaultSingleLazyPluginCollection didn't initialize the plugin when you initially created it. Now it does here:
$configurationcan't be NULL since constructor type-hints an array. I am not sure if this should be considered a bug or not since the lazy collection doesn't seem to be lazy any more.However, it is probably also a good idea to not initialize the collection on our side if the plugin ID was not set yet. The patch takes care of this.
In case you are wondering why it can even happen for plugin to be unset: it happens in the media type add form when the plugin wasn't selected yet.
Comment #306
tim.plunkett#2851635: DefaultSingleLazyPluginCollection retains stale instance IDs is a standalone bug that was surfaced by the above problem. It should fix it for this case.
But that interdiff also seems like a reasonable optimization.
Comment #307
slashrsm commentedAdded some MediaSource tests. Covered a lot, but I still need to add few things so consider this a WIP.
Also added annotation support for thumbnail alt and title fields. This was suggested at some point but never done and while I was looking at that part of the code already I did it.
I'll continue tomorrow morning and submit a final patch during the day.
Comment #308
slashrsm commentedI think that we're now testing all MediaSource functionality which means that this should be ready for the review.
I also added interfaces that @seanB forgot in #302 and he sent them to me over email. Field storage is now marked as locked as discussed in the recent comments.
Comment #309
wim leers#302:
Oops, you're right, you are supposed to override this. Never mind me :)
Sounds good!
Specifies whether the thumbnail update is triggered from the queue.
I don't quite like this name but can't think of anything better. Hopefully somebody else sees a way to make this clearer.
So much better! :)
#307:
Yay!
Why can these be NULL? Because they're optional?
If so, we should document that.
s/image/thumbnail/
Probably a stupid question, but need to ask it anyway: Why did we not need this before yet need it now?
How does this figure out automatically to use the
['value']key of the metadata attribute?Ah, because
\Drupal\media_test_source\Plugin\media\Source\Test::getMetadata()'s logic looks at['value'].All good :)
Can be simplified to
FieldStorageConfig::create()Can be simplified to
FieldConfig::create()YAY for testing the basics here, and doing that so thoroughly!
Yay for testing this edge case!
And yay for testing this edge case!
s/default//
I'd have thought we'd start with the default thumbnail, i.e. with the thumbnail not originating from metadata. But starting with this is fine too of course.
… reviewing rest of this test …
Oh! We don't test non-queued thumbnails plus default thumbnails? Isn't that a valid use case too?
This is essentially already tested in
testMetadataMapping()but that's okay.Interesting: deleting a Media entity field value and then saving that Media entity causes empty fields to be repopulated with current metadata?
If so, then we should also test this in
testMetadataMapping()./change the source field value/change the source field value too/
Yay!
Great! But this made me realize that all prior assertions should also be asserting the value of
$media->thumbnail->titleand$media->thumbnail->alt.#308:
MediaSourceInterface?Looks like a sane addition.
Great! Although somebody with deep Entity Field API knowledge should confirm this is correct, I don't know this well enough.
These look great!
:D
:D
GREAT to see that this edge case is tested!
s/source/source field/
WOOOOT
Hm, I'm not sure what this means. Using a form? Just about any form?
Don't you mean something like "when using the media type creation form"?
(And to other reviewers: no, this does not cause problems when doing config deployments: when doing config deployments, you need to deploy _all_ depended config anyway, which would include that source field config. So this is correct.)
This is such a huge leap forward. I mostly have nitpicky remarks and enthusiastic remarks as you can see :)
The key thing that I'm still missing is test coverage for the "map metadata to complex fields" functionality.
Comment #310
slashrsm commentedIn current patch we removed that. We only have simple mapping in it at the moment. In #289 I proposed a possible way forward for that part. I also think that it could be done in a follow-up in a BC way.
My idea was to take this hunk:
and convert the logic into a plugin. Default plugin implementation would have the exact same logic. Plugin would probably do 2 things a) check if the fields should be updated and b) do the actual mapping. I don't see any BC problems with this approach, but I am not sure how to prove that with a test in a current patch.
I am working on the other things that were brought up in #309.
Comment #311
wim leersI find #310 sufficiently convincing that I'd be in favor of postponing "complex field mapping" functionality to a follow-up.
It'd change this:
to:
That's a trivial change, which does not require API changes.
Comment #312
slashrsm commentedThis is addressing issues that were brought up in #309. Everything mentioned by @Wim Leers in that comment should be fixed with this patch.
#302
#1 - Done
#2 - Not sure either.
updateThumbnailFromQueue()? Or maybe evensaveFromQueue()which would then update the thumbnail and save the entity?#307
#1 - Done
#2 - Done
#3 - When writing the tests it turned out that there is not way to set this programatically (outside of the media type form) since the map is
protected. Tests++#5 - Done
#6 - Done
#10 - Done
#11 - By default no. Plugins can use default thumbnail if they are unable to get the proper one for whatever reason, but this is implementation dependent. Not sure if there is much benefit in doing that just for testing purposes.
#13 - Done
#14 - Done
#16 - Done
#308
#1 - Done.
#8 - Done. Did s/source/source plugin/ since I felt it better describes the situation.
#10 - Done.
Also ran the code through the CodeSniffer one more time and fixed that too.
Comment #315
chr.fritschI discovered #2855630: Field mapping not working properly in the contrib media_entity, that should be addressed here, too.
Comment #316
slashrsm commentedRe #315: @phenaproxima mentioned the same problem in #67-46 already. We had few discussions about that after it but never come to a conclusion.
The problem is that we return boolean
FALSEfromMediaSourceInterface::getMetadata()if a metadata or its value is not available. This becomes problematic in cases where FALSE represents an actual valid value.So far I imagine two possible solutions:
NULLinstead: Keeps things similar to how they stand right now but it should be more robust since we use something that actually means "no value".Comment #317
mtodor commented@slashrsm - I think that using of exceptions here is wrong, exceptions should be used when something unexpected occurs. For me it's normal behaviour that handler/source/plugin (or however it's called now), cannot retrieve value for some metadata (fe. EXIF for image) - for me that's not exceptional situation but normal common case.
So I would just stick to
NULLbecause it's simple and everyone knows what that means -> "no data". Possible problem that can appear with that is: what if NULL is for plugin "valid data" and not "no data" -> and it's something plugin want to set? In that case you can go with NoData class or some value wrapper class with state "no data", but all that is to much overhead in my opinion - it's better to keep it simple as long as it covers majority of use cases.Comment #318
slashrsm commentedOK. Let's do that then.
Comment #319
phenaproximaWelp, it took like 5 hours, but I read this entire patch from top to bottom. All of it. Every line. Even the tests.
Overall, I think it looks effing fantastic! I wish all of core looked like this. The effort and care that everyone has put in shines through each and every line, every single class, every single test. I can only squeak a humble bravo to all who helped make this happen. Every day I am privileged to code with you all.
That said, I found some stuff, which is inevitable for a 7,000 line patch. Most of it is nitpicky doc comment stuff; some is just confusion. But none of it is major. Frankly, I'm tempted to RTBC right now (I know I have contributed to this patch, but it was long enough ago that most of what I wrote has been significantly changed and refactored), but we should probably answer at least some of the questions that came up before it's time to do that. Also, we'll probably need to bribe a committer to sit down and review this patchzilla. Or maybe a committee of committers, 'cause alliteration is fun!
Without further ado:
Should say "Defines JavaScript behaviors for the media type form."
If this returns FALSE (i.e., an error condition), should we handle that case? Or are we just relying on hook_requirements()?
What is the benefit of using an inline_template here?
Correct me if I'm wrong, but I believe "tweet" is a normal noun and therefore should not be capitalized.
I don't see why the "Media type" link should be capitalized. Can this just be "media type"? Also, I don't see any reason to link to the same page twice -- in fact, it might be confusing. I would keep the first link and remove the second.
Will people with the 'administer media' permission be able to access this route?
There is an errant apostrophe at the beginning of this string :)
Can this first sentence make it clearer that media.settings.icon_base_uri is a config setting?
Should say "of how to do to this".
Total nitpick, but $this->set() already returns $this, so we could remove the additional return here and elsewhere, if we want to knock out a few extraneous lines.
I'm not a huge fan of the way getSource() is repeatedly called. Can we just call it once before we do all this stuff? It will make things a little easier to read and be a nice little micro-optimization.
Nitpick, but we could reorganize this so that we only need to call thumbnailDownloadsAreQueued() once. Might make this a little easier to read.
Nit: If I'm grokking this correctly, we could simply return the result of the if statement directly. No need to explicitly return TRUE or FALSE.
Again, this method is repeatedly and needlessly calling getSource(). Can we call it once at the top of the method to make it a little cleaner?
Nit: $queue is never used, so we could just chain these calls together.
Nit: We could reorganize this a little bit so that it's only necessary to call $this->isIsNewRevision() once.
It's too bad there's no trait for this yet (is there?)
Nit: We could call $this->set('description', $description) and return the result directly (which will be $this). Same applies to other property setters.
$map should have the array type hint, here and on the interface.
Can this clarify by saying "Provides a confirmation form to delete multiple media items at once"?
I'd like the type hint to be more specific about what this array contains. Is it an array of IDs? Fully loaded entities?
$manager description should say "The entity type manager".
The $query_factory description should say "The entity query factory".
Again, I don't quite understand what benefit we get from inline_template. What's wrong with just #markup?
Can these chained calls be on their own lines, for clarity?
Ditto here.
And here.
Do we want per-bundle creation permissions? If so, should we introduce those now or open a follow-up issue?
It'd be nice if there were a trait or base class for this :) Follow-up issue, perhaps? If one is already open, maybe we should add a @todo that refers to it?
What does it mean for a button to become "primary", and how will unsetting #button_type accomplish that?
Should say "interface defining an entity..."
Is there an issue for EntityCreatedInterface and an accompanying trait? If so, can we reference it in a @todo?
We should probably explain why we do this, rather than let the parent constructor do it (I think it's to merge in the default config, but it's still not too obvious).
Should say "filtered by the allowed field types..."
Can the return type be string[] for greater specificity?
So, uh...won't this only overwrite non-NULL values in $this->configuration? That seems like it might have strange side effects. If it's the intended behavior, I think we should explain in a comment why we're only overwriting non-NULLs.
Should say "defines an interface..."
Should say "To add constraints at the source field level..."
Should say "defines an interface..."
Should say "To add constraints at the entity level..."
s/Youtube/YouTube
Can this say "media items" for consistency, instead of "Media entity items"?
"Media" does not need to be capitalized.
I'm not sure "tweets" is supposed to be capitalized.
Should say "responsible for actually storing..."
Should say "from a third-party API..."
Should say "Media sources should fall back..."
Should say "Providing a default name for a media item".
Should say "a third-party API".
Should say "...available through a third-party API..."
Unless mine eyes deceive me, that looks like a so-called smart ellipsis at the end. Should probably be replaced with three periods for, I dunno, ASCII friendliness or whatever.
Should say "...or NULL if it doesn't exist or has not been configured yet."
Er...why is there a parenthesis at the end of $form['source_dependent(']? That looks like a typo, but it's used elsewhere in the form, so I figure it's intentional. But I've never seen that in Drupal before. What gives?
No need to do the ternary logic here; $this->entity->getSource() will be NULL if the source is not configured yet.
Can we use $field->isBaseField() rather than the instanceof check?
I feel like '_none' should be a constant on MediaSourceInterface.
"Administer media" should be italicized or otherwise differentiated.
This description is pretty useless. Can it be explained a little more? What is the advantage of queueing thumbnail downloads? Is it for performance? If so, we should mention that.
I could be wrong, but I believe it is SubformStateInterface, not SubFormStateInterface.
Can these calls be chained?
Where is $form['source_dependent(']['source_configuration'] defined?
We could flatten the method a little bit if we replaced this with an elvis operator.
Should say "Added media type %name."
Should say "...bundles for media items".
Should say "...but their meaning and uses in a Drupal site are different".
Can this say "Returns the media source plugin"?
Should say "media item-related metadata..."
Type plugins are no longer a thing, and "Entity" does not need to be capitalized.
$map should be type hinted.
ConfigEntityListBuilder already implements EntityHandlerInterface.
Should be "Provides the Views data..."
$entity can be NULL but we're blithely calling methods on it? We should add an if ($entity) check here. Also, why is $entity not type hinted to MediaInterface?
Why not simply pass $return_as_object to the andIf() call and return the result directly?
Should use the active voice, like the other actions: "Saves a media item".
We need to type hint $entity, and check if it's set in the method.
Again, $entity needs to be type hinted to MediaInterface and checked for null-ness in the method.
Once again, this could be simplified by not passing TRUE to andIf() and directly returning the result of the call chain.
I kinda find myself wishing that this were a protected helper method called for each media item inside the foreach loop; I feel like it'd be a little cleaner that way. Not married to the idea, just a preference.
Forgive me if I'm wrong, but doesn't calling a content entity's get() normally return a FieldItemListInterface...?
Nit: Could all be one line.
All this, and ContainerFactoryPluginInterface, can be removed. It doesn't look like it's doing anything for us...
...unless we inject the media storage handler here and use it to load the thing.
"views" should be capitalized.
Once again, "Views" should be capitalized.
Why is this different from the same constant in the Media wizard?
Nit: Should have a comma after "item".
Doesn't seem accurate :) Copypasta?
s/generic/test
Why is this stuff coming from state?
Seeing as how defaultConfiguration() is implemented, $this->configuration['test_config_value'] should never be empty.
Nit: We could remove all of this and just create the media type in testMediaAccess().
This line should probably be a // comment in the method.
Should say what the default value is.
Should be if (empty()) to prevent against falsy values.
This should probably be assertSame().
This doesn't seem to be used in the test, so we can probably ice it.
Why aren't we asserting that $media->label() and $media_name are equal?
Same here?
It seems a little dicey to assert against raw HTML, since that can vary by theme. Could we do an assertElementNotExists() instead?
Could we also assert that a new revision was, in fact, created?
I've never seen anything in Drupal use the @package annotation, should we get rid of it here?
A lot of the stuff in here ($modules, $adminUserPermissions, $adminUser, $nonAdminUser, etc.) seems awfully similar to MediaFunctionalTestTrait, which this is already using. Is it copypasta? Can we remove it?
This is neat but it seems like it might be a little more stable if we assert this stuff at the API level.
I wonder if this should be moved to a helper method of MediaJavascriptTestBase.
Should say "Check that no new fields were created."
Should say "Source field storage definition".
I dunno how I feel about this. It makes it a little harder for us to change the text easily -- as I have suggested about a million times over the course of this patch review. Seems overzealous to me. What are these assertions really proving for us?
This should be reworded, it's kinda unclear.
As far as I know, Views wizards don't do AJAX things once you click 'Save and edit'...do they?
Ditto here.
I'm not sure what this proves; the field map is empty by default. I think we should check for some sort of sane default value here.
The assert message seems wrong; isn't it supposed to be a failure message?
I'd rather call getSource() once at the top of the test and reuse that variable throughout (or at least, as often as possible). Also, given everything else I've seen in this test suite, are these assert messages supposed to be affirmative or negative? It's totally mixed.
Ah, so that's why the source plugin uses state. Couldn'twe just as easily call setConfiguration() on it, rather than doing this?
More than one sequential call to getSource(). This is my nemesis :)
Can we remove the sequential calls to getSource()?
The EntityStorageException could have been thrown for a whole mess of reasons. We should either catch a more specific exception, or assert the exception message.
I think ConstraintViolationListInterface is countable, so we could use assertCount() here.
Could use assertCount() here.
Again, let's see if we can't trap the exception a little more specifically.
Can be assertCount().
Can be assertCount().
Can we reuse the media type created by the base class?
Can't we re-use the media type created the base class?
Should say "media item".
Should say "media item".
Comment #321
gábor hojtsyLooking at the patch writing a change notice draft at https://docs.google.com/document/d/1IlfkkVLbFhlrj0NRfIa8UIXgoqQBudiRxRDP...
I think hiding the module is not useful given that this goes into a branch that will not get released for months. I would suggest not to hide the module in its info.yml.
Comment #325
marcoscanoMost quick-fixes from #319 are addressed here.
There is still a failure with
MediaCacheTagsTestthat I am unable to troubleshoot right now...Status of the feedback from #319:
1: Done
2: Needs work
3: Needs work
4: Done (yes: https://www.merriam-webster.com/dictionary/tweet)
5: Done
6: Done. Changed to administer media, to keep consistent with how nodes deal with this
7: Done
8: Done
9: Done
10: Done
11: Done
12: Done
13: Done
14: Done
15: Done
16: Done
17: Needs work
18: Done
19: Done
20: Done
21: Needs work
22: Done
23: Done
24: Needs work
25: Done
26: Done
27: Done
28: Needs work
29: Needs work
30: Needs work
31: Done
32: Needs work
33: Needs work
34: Done
35: Done
36: Needs work
37: Done
38: Done
39: Done
40: Done
41: Done
42: Done
43: Done
44: Done
45: Done
46: Done
47: Done
48: Done
49: Done
50: Done
51: Done
52: Done
53: Needs work
54: I don't believe we should do that. We can't use $this->entity->getSource() if the MediaType source property is empty.
55: Needs work
56: Needs work
57: Done
58: Done
59: Needs work
60: Done
61: Needs work
62: Needs work. (Are you sure? I believe there is logic inside the if that should only happen if !$source_field)
63: Done
64: Done
65: Done
66: Done
67: Done
68: Needs work
69: Done
70: Done
71: Done
72: Done
73: Needs work.
(IMHO: I believe this is done this way to be consistent with with what node does, and it may allow for easier debugging perhaps?)
74: Done
75: Done
76: Done
77: Needs work (same as 73)
78: Needs work
79: Needs work
80: Done
81: Needs work
82: Needs work
83: Done
84: Done
85: Needs work
86: Done
87: Done
88: Done
89: Needs work
90: Done
91: Done
92: Done
93: Done
94: Done
95: Done
96: Done
97 and 98: Needs work
(IMHO, being a functional test, doesn't ::titleEquals() have a better coverage than simply checking the entity's value? We are testing the whole "create an entity using the form" process, not just the entity creation itself. Although I agree it is a very small difference :)
99: Done
100: Done
101: Done
102: It's similar to the MediaFunctionalTestBase, but we are not extending it. Do you mean move all this elsewhere?
103: Needs work
104: Needs work
105: Done
106: Done
107: Needs work.
(IMHO: Well they prove that all elements are on the page, with their respective descriptions. If in the future we change any of these, the same change should also update the test, shouldn't it?)
108: Done
109: Done
110: Done
111: Needs work
112: Done
113: Done
114: Needs work
115: Done
116: Done
117: Needs work
(I kind of don't get this line. Wouldn't the message be printted only if the assertion fails? (which is never in this case) )
118: Done
119: Done
120: Needs work
121: Done
122: Done
123: Needs work
(I believe the idea here is to test the source field creation that happens during the creation of the media type. The one in the base class is already created and saved at this point)
124: Needs work
(same as 123)
125: Done
126: Done
Comment #326
xjmHiding the module is the release management decision to allow adding module to core now. Regardless of when the release is released, Drupal 8 branches need to be in a shippable state at all times. This means hiding the module until the agreed followup steps are completed.
Comment #327
gábor hojtsy@xjm: ok, if that is the only way, it is still better than not committing this until all the required followups are committed, so that will be the way then. Let's work together to identify which of the 9 existing followups would be a requirement so we can prioritize work accordingly. Also of course if any followups needed that were not yet identified.
Comment #328
berdirI'd argue it *was* the decision for getting it into 8.3 and 8.4 might be a bit different, at least if we get it in *soon*. My suggestion would be to get it in now without hiding it and then decide before 8.4.0 beta or RC if we're going to release 8.4.0 with it being hidden or not? It will make testing it for non-experienced users and on simplytest.me a lot easier for the next few months.
Comment #329
xjmThe module presently offers no user-facing features, and, as @Gábor Hojtsy has said, "A UI which tells you that you can't do things." Presumably, those non-experienced users who are going to be testing the module will be doing so with a contrib module that prompts them to enable it anyway as a dependency, or with patches for additional user-facing functionality like the library. Also, in the followup issues, exposing two image entity types is problematic and we'll need a solution for that. So, until we address those things, the module should remain hidden, so that contrib can depend on it but core does not offer a user experience with it yet.
Edit: Looking back at #2825215-42: Media initiative: Roadmap (sorry it's been a month), the plan was to add the module as beta experimental, in the experimental package, then mark hidden if it was not stable. The module needs to either be marked hidden, or in the experimental package, until the user-facing experience makes sense with the relevant plugins for other types, a way to handle the duplication with stable core types, CKEditor support, etc. We can discuss more on that issue, but just reiterating that the module needs to be hidden if it's not in the experimental package, and for me that is commit-blocking. :) Moving it to the experimental package and removing the hidden instead is also an option (and what slashrsm and I agreed on), but let's not spend time discussing that here.
Comment #330
seanbI addressed all the comments marcoscano marked as 'Needs work'. The 2 following comments are still open:
Added interdiffs for 318 and 325 for easier review.
Here is more detailed feedback on all other comments:
$this->mediaItems[id][langcode] = langcodeFieldDefinitionInterfacedoesn't defineisBaseField.\Drupal\media\MediaTypeForm->buildForm().FileFieldItemList. Which works, since the get() method onFieldItemListuses the first() method. And in this case, first() returns a ImageItem object. Maybe it's more clear to use$media->get('thumbnail')->first()MediaCacheTagsTestused this trait as well for media type creation, I added a separate trait for this.Comment #332
seanbI fixed the tests (hopefully). The 2 following comments are still open:
Added a bunch of interdiffs for easy review. Phenaproxima, could you check and confirm all points are properly addressed?
This was done:
access()returns a bool when $return_as_object = FALSE so we can't chain it then.assertIdentical()sinceassertSame()is not supported by simpletest. Added todo with a link to https://www.drupal.org/node/1945040.MediaUiFunctionalTestgetCreatedTime()andsetCreatedTime().Comment #333
xjmAttention reviewers!
Last week during the Media meeting, we discussed how we can ensure that all points of feedback are addressed on this issue so it is easier for people to sign off on the issue with confidence. I made this spreadsheet:
https://docs.google.com/spreadsheets/d/1Ba_dN9K7SbfHMBHZmcGVAI576Gy_u8ZX...
Anyone can help populate the spreadsheet. If you have previously reviewed this issue, please enter your review comment in the sheet and check whether your feedback has been addressed!
Thanks!
Edit: I have review notes for comments from #1 through #107 so I will enter those in the spreadsheet in another tab for now (so we don't edit over top of each other).
Comment #335
phenaproximaChecked my review (#319) and updated the spreadsheet from #333.
Here are my comments regarding the ones that have not been addressed. I'm quite comfortable letting many of these go; the rest can be addressed in follow-ups. None of these are commit blockers.
#319.3: Reviewing @dawehner's original comment in #104, it looks like the inline template was only beneficial when we wanted to include HTML tags. This is no longer the case, so the inline_template serves no purpose. That said, I'm not hell-bent on removing this. Can always be done later or in a follow-up issue.
#319.6: The _permission requirement allows us to "or" permissions together like
administer media+delete any media. That might be a better way to go here, but I'm OK with that being a follow-up issue.#319.10: $this->set() is being called but the value is not returned, which breaks interface conformance. Same for setOwner().
#319.28 and #319.29: Both should be follow-ups.
#319.36: OK for now, but should be a follow-up issue.
#319.45: Still incorrect but I don't really mind. We can fix this when we fix it, in some future issue...
#319.46: Ditto.
#319.50: Ditto.
#319.54: Not fixed, but it's just a style thing. No big deal.
#319.58: Greatly improved, although there is a tiny grammatical snafu in the new description. It can be fixed at some later time, as far as I'm concerned.
#319.62: Doesn't seem to be fixed, but like #54, it's a style thing and not a big deal at all.
#319.73 and #319.77: Not fixed, but I don't care much.
#319.103 and #319.104: Don't appear to be fixed (bit hard to tell), but these tests are incredibly dense anyway and could use some refactoring anyway, IMHO. Doable in the future, as a follow-up or series of follow-ups. The most important thing is that the tests are robust and very thorough, which they most certainly are.
Comment #336
seanbAddressed #335:
#319.3: Removed inline template.
#319.6: Changed to
administer media+delete any media#319.10: Added missing return.
#319.28: Follow up -#2862422: Add per-media type creation permissions for media
#319.29: Follow up -#2835535: Standardize basic content entity form logic in ContentEntityForm
#319.36: Maybe slashrsm can comment on this one?
#319.45: Fixed.
#319.46: Found a missing one indeed and fixed it.
#319.50: Fixed.
#319.54: This won't work: Call to a member function get() on null in media/src/Entity/MediaType.php on line 174
#319.58: Changed it a bit. If there are better suggestions please let me know!
#319.62: I don't think we can change this (I could be missing something). See my comment in 332.
#319.73 and #319.77: I don't think we can change this (I could be missing something). See my comment in 332.
#319.103 and #319.104: Follow up: #2862425: Refactor tests for media module
Comment #337
wim leersMy last comment was #311. Catching up on everything since then.
#312 addressed my feedback in #310.
#316: I like . I dislike exceptions for the reasons given in #317. This was implemented in #318. Great!
#319: wow, nice write-up! Also, nice of you to soften the blow of a 126+-point review with a very positive introductory paragraph :P Many of the points are nitpicks, but also quite a number are not nitpicks at all, for example points 28, 30, 36, 53, 54, 56, 81, 107. (Also, I've never heard of :D)
The only one I think that doesn't make sense is #73/77 (same remark). The answer to 89 is that it's okay to use State in tests to emulate different implementations of hooks/event subscribers. 103 I disagree with explicitly, because the whole point of that test is that it's testing that the UI works as expected. It's not about testing what's stored, and hence testing the API would be missing the point — the point is that we're testing the auto-generated UI directly.
#325 and #330 addressed all this, amazingly!
This is wrong.
This might cause
BOOL->andIf(BOOL), which will simply fail.Please revert this. (That was for #319.73 + #319.77.) EDIT: already done in #332, great :)
I was going to say that I don't like this. But it's protected, so that's fine.
However, the description is wrong.
#321 + #326 + #327 + #328 + #329: regarding the module being hidden or not: sounds like that's been sorted out :)
I think this is pretty much ready. I already indicated that in #309. Would be good to still address #319-28 + #319-36. And probably the nits in #335?
I could do another complete review, but I think we caught pretty much all the things at this point. Now updating the spreadsheet for my review points.
My main concern was the "complex field mapping" functionality, but I was convinced by #310 + #311 that it's fine/safe to defer it to a follow-up.
Comment #343
amateescu commentedHere's the small update for #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table that I promised to write here at Dev Days Seville :)
The current usage of
RevisionLogEntityTraitonly works because\Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys()uses the BC layer by default and specifies default names for revision metadata keys, but we should be future-proof and specify them explicitly.Comment #345
webflo commentedgeneric.pngwas broken. I replaced it with the one from media_entity module.Comment #346
imiksuRe #337.2
As per quick IRC conversation with Wim Leers, the problem was that the method is
getMediaUrlbut the description is "get media thumbnail". A media entity's URL !== a media entity's thumbnail.Therfore he suggests renaming the method name to
getMediaThumbnailUrlor even justgetThumbnailUrlsince it's protected anyway.Comment #347
xjmMarking NW for #346.
I think #345 undoes previous feedback in #191 about removing Adobe metadata (though probably better to have that than a broken image).
I thought the intent was that media entities should eventually replace file and image; how does media depend on file and image then? (Edit: pasted the wrong snippet; fixed now).
Comment #349
imiksuI decided to go with
getMediaThumbnailUrl, I felt it was more descriptive to what the method does.Comment #351
wim leers#349: thanks!
#347: RE: Adobe metadata: there's actually no Adobe metadata in there. It's already good to go.
Discussed the remaining unaddressed feedback at DDD Seville. Several of the ones that we agreed required some changes to the patch were assigned to me:
FILE_EXISTS_REPLACEwithFILE_EXISTS_ERRORComment #352
slashrsm commentedThis addresses #319-36.
Comment #354
slashrsm commentedOK.... I was to fast. Test only patch will pass in #352. Improved a test a bit and made sure it actually fails without the fix. Sorry for the noise :(
Comment #361
alexpottI think this use of t() might be incorrect - or at least use the entity's language and not just the global language. Tricky - I'm not sure I can think of precedent.
I'm pretty sure that the thumbnail field should be translatable - see #2861508: Thumbnail is not language-aware - and certainly the alt and title columns need to be.
Comment #363
slashrsm commented#361 addressed.
I also parsed the commit history of the contrib module. Attached is the list of all contributors. It would be great if we could attribute them when committing this patch.
Comment #364
alexpottI think we should have some tests around #361 and the solution in #363. Seems something that could easily get broken.
Comment #366
webchickI'm trying to test the proposed change record at https://www.drupal.org/node/2863992 by porting the https://www.drupal.org/project/media_entity_audio module.
The first thing you run into is the removal of the media_entity_copy_icons(). I see earlier in the discussion there was speculation that this function isn't used outside of Media Entity module. However, in a quick survey of the sub-modules listed at https://www.drupal.org/project/media_entity (Twitter, Audio, etc.), all of them are doing this in their hook_install():
This seems further backed up by the documentation at the top of the
$default_thumbnail_filenamewhich reads:Unfortunately, the media_install() function went from 3 lines to many lines, due to the inlining of this function:
That's an awful lot of code to copy/paste X every media plugin type. Could we add the helper function back?
Additionally, it should be noted that the inline code is different from the original copy of this function (exception checking removal, additional icon types, etc.), but I surmise those changes were by design.
EDIT: Oh, and @xjm notes that it might be nice IN A FOLLOW UP to automatically copy icons to their destination rather than adding boilerplate code to hook_install().
Comment #367
xjmSo reading over the map-of-comments, I think #2863438: Allow modules to declare the list of files it ships with that should be publicly accessible is the issue intended to address the "Why do we need to copy this manually anyway?" point, although it's scoped differently than I would expect.
Regarding these three lines:
To me they only seem like they would need to be one line? The module name and icon_base_uri can both be retrieved from the module in a less boilerplate way. So we move that code or the like into the module's helper API for
hook_install(), and then follow up to eliminate the need for anyhook_install(). Feels a similar pattern to optional config, but for assets.Comment #368
boobaaFixing two typos in comments only.
Comment #370
xjmI've updated the issue summary with some but not all of the remaining tasks, and removed resolved ones. I'm marking comments on the DDD meeting notes resolved as I work through the action items from the meeting and check them.
Regarding contribution credit, we will add issue credit for the Media contrib module contributors (provided by @slashrsm and linked in the summary) just prior to commit. There are 66 names (not hundreds or thousands) so I think this is safe to do on the issue.
Remaining tasks I've added to the summary, plus a couple new things:
I see a followup issue for #2862453: Make "update any media" a restricted permission, or remove the flag from "delete any media", but this is actually a pretty important issue from a security perspective. Let's do that in this patch instead. So add the restrict access flag here:
Few docs nitpicks; just saw these scanning to confirm the fixes made during the sprint:
@internal.@todotext should be wrapped as per https://www.drupal.org/node/1354#todoAddress #366 / #367 (possibly with a followup to avoid adding procedural API unnecessarily).
Can we file and link those now?
There is a small point that I think we missed following up on in our discussion on Thursday regarding which image file types should be allowed as icons. In #213 @Wim Leers says:
But these file types were (I think) added in response to previous feedback which I've lost track of. During the meeting we said this could be discussed in a followup because it's not that important, but I don't see a followup issue for it. Added a note to the summary for now; maybe @Wim Leers can confirm/clarify.
Finally, @tedbow and @webchick worked on the change record draft during the DDD sprint. This document should eventually communicate what contrib developers need to know to easily update their code from Media Entity in contrib. This is probably the biggest remaining single task (other than committer review and signoff...) and so we could definitely use the help both of people trying to upgrade contrib code but not familiar with this issue, and of people with strong knowledge about this patch and about contrib Media Entity to confirm its accuracy. (@webchick was testing the documentation out by updating one particular module, as she describes above).
Edit: regarding:
I'm wrong about this. The "Edit any X" and "Delete any X" permissions in core for node content do not have restrict access, so we shouldn't add it to media either. And, at least, they should be consistent. I'll reopen the other issue.
Comment #372
boobaaAnother question has been surfaced during the IRC meeting today: how could the MediaSource plugin (or the module providing it) change the default entity form display for its auto-created field?
As
createSourceField()only creates the field_config but does not save it, I can't find any way to change the MediaSource entity form display for this field to a more sane default (like a custom derivative of inline_entity_form in my case). As this method is called fromMediaTypeForm::save()(and not from anything in MediaSourceBase), I cannot even intervene. There's not even a hook there. In the contrib era, I could get this done viahook_media_bundle_insert()/hook_ENTITY_TYPE_insert(), but this isn't available either. So we have this question.@seanB asked if this means we can't change the display in the interface because it's locked, or does it just mean the default is not good enough?
As locked fields' form display settings can be changed (at least now, before #2274433: Do not allow to alter Locked field via UI is fixed), it's only about the latter part: allowing the MediaSource plugin (or at least the module providing it) to change the form display settings so they can be "good enough". In the contrib era, brightcove.module solved this by changing the form display settings from
hook_ENTITY_TYPE_insert(). As you can see, that approach wasn't the best one either, because the field wasn't created with appropriate form display settings (honestly, I don't really know if it's even possible), but those settings were only changed afterwards.OTOH, you can see it's only about providing that
$form_display_settingssomehow, as it's fairly clear when to do what with that information. @seanB and I agreed that it should be doable to provide this information via the annotation. Because of that, @seanB said we could do this without changing the interface, ie. we could fix this in a followup.To cut this long story short:
MediaTypeForm::save()should be able to create the field with form display settings provided by the annotation (or fall back to defaults if the annotation doesn't have this info). A followup is created for this: #2865184: Allow MediaSource plugins provide default field form/view display settings.Comment #373
seanbWhile the field storage is locked, you can edit the field widget/field formatters for the field you have created. Besides that when you ship a module with default config for a media type you can also add some defaults.
Boobaa, could you add an example of the settings you would like to be able to do? I'm not sure what you exactly would like to change (widget and/or formatters) and how far we should go in allowing modules to change through the annotation. Adding more methods to the MediaInterface is something we should try to avoid.
I do see cases where you want a specific widget or formatters when creating a field, but since you also get defaults when adding a field to a media entity (or a node for that matter) through the interface, I'm not sure if we should do this.
Comment #374
boobaa@seanB, the followup at #2865184: Allow MediaSource plugins provide default field form/view display settings already has an example, both for the widget and the formatter stuff.
Regarding the defaults: for complex stuff (like our Brightcove Video entities), MediaSource plugins might want to have their own defaults for their fields instead of relying on the defaults the field type provides. Particularly in our case, we prepared a custom inline_entity_form for handling our brightcove_video entities when they're linked eg. from a node via an entity reference field. While the generic, core-provided defaults for entity reference fields might be sufficient for most cases, it's not not the best for our admittedly complex case.
But hey, as you already suggested on IRC, the MediaInterface does not need any changes for this to work – more details on this can be found in the followup (which has some copy-pasted code that actually works, btw).
Comment #376
seanbFixed:
Still todo:
Comment #377
phenaproximaThe change record has been updated with explicit one-to-one conversion instructions. It would be helpful if @webchick (or anyone else) could try to port a media contrib module again using those instructions, to see if there are any gaps left. I based the instructions on everything in the Google Doc, though, so hopefully it's pretty thorough.
Comment #378
naveenvalechaRemoving File followups for Devel Generate and Plugin contrib modules? from remaining tasks in the favour of Plan issue #2860796: Plan for contributed modules with Media Entity API in core
//Naveen
Comment #379
naveenvalechaI did the port of couple of the media_entity contributed modules here #2860796: Plan for contributed modules with Media Entity API in core and during the port of the Facebook Media Source #2869021: Facebook Port to the proposed Media core module API, I encountered the issue which was due to the module didn't define the config metadata of the media source which results in the WSOD.Isn't this seem to be the blocker here I have posted its backtrace there #2869021-13: Facebook Port to the proposed Media core module API?
I have re-read the whole thread to find that there's nothing follow up left.Found that we need tests for the media_theme_suggestions_media.
Fixed:
#191.23 Added tests for media_theme_suggestions_media
//Naveen
Comment #380
phenaproximaForgive me (and correct me) if I'm wrong, but I see two things wrong here --
I think this call might be wrong. According to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21StringTra..., this call should be:
$this->t('Thumbnail', [], ['langcode' => $this->langcode->value])Corollary to this, it would appear that the tests added by @seanB in #376 are not actually testing that the thumbnail's alt text is translated. It seems to be a generic test of general field translatability, in this case covering the media item's source field.
By no means am I against having a lot of test coverage, but that seems like something that should be, and probably is, thoroughly tested elsewhere in core. We should change the tests, or at least amend them, to specifically test the translation of the thumbnail's alt text.
Comment #381
boobaaAdditionally, the patch should not add calls to deprecated functions (eg. the current
MediaTypeForm::save()calls bothentity_get_form_display()andentity_get_display(), which are marked as deprecated).Comment #382
phenaproximaWell...those functions are deprecated, sure, but they have no real replacement. There is an issue open to create replacements on EntityDisplayRepositoryInterface (I know 'cause I wrote the latest patch for it :) but it has stalled for the time being. So, for now, it's OK if the patch calls those functions.
Comment #383
chr.fritschI fixed the first comment from @phenaproxima in #380
Also i tried to add test coverage, but then i ran into one problem. It seems that $media_translation->get('thumbnail') is always null in MediaTranslationTest.
Unfortunately i have to go in a meeting now, so i can not continue on it now. Here is my work so far.
Comment #384
seanbGood thing we added more tests. It appears the
preSave()method on Media only runs for the active translations (not for all other translations). So the metadata was not being added for the translations. Same forpostSave(). Both are fixed.Also replaced deprecated
QueryFactorywithEntityTypeManagerand a deprecatedassertEqual()call.I left
entity_get_form_display()andentity_get_display()since they don't have real replacements yet as phenaproxima mentioned. Added followup #2872159: Media: Replace deprecated entity_get_(form_)display() with replacement methods in EntityDisplayRepository and a @todo in the code.Comment #385
phenaproximaIn reviewing this change, I realized that there is a flaw in the logic:
($value = $media_source->getMetadata($translation, $metadata_attribute_name)) && $value !== NULLIf the first condition passes, $value is by definition truthy (and therefore most definitely non-null). So the second condition is redundant.
But worse, checking $value for truthiness means that values like FALSE, '0', '', 0, and so forth will not pass the condition...and therefore not be updated in the translation, even if they are valid values. This means we might be throwing data out. Seems like something we might want to fix? NULL is the only value that should represent "no value".
I'm not sure $translation->label() will ever be empty. Correct me if I'm wrong, but aren't all the values copied into the translation when it's created? Therefore, aren't all translations guaranteed to have a label?
Comment #386
seanbI decided to remove the check for the
getMetadata()value. It could be that a metadata value is unset after a update of the source field. In that case you also want to update the field to a NULL value.The check for the translation label is there because the code is also being executed for the original entity (not only for translations). Btw, if for some reason the label is empty for the original entity and the translations are created before the media entity is saved (like in the test), the label is empty on all translations.
Comment #387
phenaproximaSo many people have gone over this patch so many times. We've iterated and improved a lot of things and caught a lot of possible issues, and we have our work cut out for us in the follow-ups.
That said:
media_entity_copy_icons()bugaboo has been deferred to a follow-up.In light of all this, I don't think we need to wait any longer. The time has come for this patch to move on to the committers. It's Miller time.
Godspeed.
Comment #388
phenaproximaA small re-roll (very much still RTBC), which removes the hidden flag from the Media module's info file and puts it in the Core Experimental package. This is in accordance with @xjm's comment in #2825215: Media initiative: Roadmap.
Comment #389
naveenvalecha@dawehner, Peter Wolanin, @xjm and I had a word regarding the restrict True on the delete and update any media permission. As these are the normal content entities and the node system does work this way so we don't need the strict restriction on the update any media and delete media permissions. Related issue #2862453: Make "update any media" a restricted permission, or remove the flag from "delete any media"
I have also addressed few minor things which I found.
We only have the two deprecated functions that we are using in the media module and we have the follow-up issue #2872159: Media: Replace deprecated entity_get_(form_)display() with replacement methods in EntityDisplayRepository for that.
//Naveen
Comment #390
xjmThis review is cross-posted at #2877346: Media Entity finalish review (disregard if you're not part of the Media Initiative; working around long issue node) since this issue node is practically unusable. That issue will be used as a scratch workspace for final work on this patch.
The following review and action items is the result of discussion between @catch, @alexpott, @seanB, @phenaproxima, and myself. Most of it is followup stuff. Enjoy!
Remove from patch into "must" or "should" followups
These things should be removed from the patch so that they can be reviewed on their own without blocking the issue. Each should be mentioned in the change record with a link to the followup issue.
In the case of some parts, we might reduce the scope and add the rest back later; in other cases, the functionality might not need to be in core. But we'll discuss each in a dedicated issue. Be sure to add the notes below to the applicable summaries.
Also, in particular:
This was copied over from node and may not be needed?
@catch also notes that the action itself is weird and might not be needed even if we add the media actions in core.
Media::preSave()and file a followup to discuss presave validation for Media and/or for entities in general ("Should").Other followups to file
<article>:Media::updateThumbnail()which has been discussed at length on the issue and at the DDD sprint. Mark the darn thing explicitly internal and file a followup to discuss it more ("Should"). Someone should go through the whole issue, the DDD spreadsheet, the DDD agenda/notes, this review's notes, etc. and collect all feedback about it and put it in the followup.Things to fix in the patch
So let's use "tempstore" for now.
Media:getThumbnailUri(). It gets the thumbnail's URI, not the media entity's. Also explicitly mark the method internal. @todo also check the issue for other discussion of this method.Media:sourceFileChanged()toMedia::has/isSourceFileChanged()(which?) and explicitly mark it @internal.In path_entity_base_field_info()fix thein_array()usage to be strict to avoid bugs.\Drupal\media\MediaAccessControlHandler::checkCreateAccess()to allow users with 'administer media' to create (and add tests).Constructs an ImageFormatter object.C&P error; should beMediaThumbnailFormatter.<article>referencing the followup to be added above.Comment #391
phenaproximaWe're on home stretch, everyone!
All feedback from #390 has been addressed in #2877346: Media Entity finalish review (disregard if you're not part of the Media Initiative; working around long issue node), and I have closed that issue as a duplicate. Attached is the final patch to emerge from that issue, and its interdiff against #389.
Here is #390, point by point. I reviewed the code changes and confirmed they are completed.
Removed and moved into its own patch at #2877378: Add token replacements for Media.
Removed and moved into its own patch at #2877383: Add action support to Media module.
Removed, and a follow-up has been filed: #2877397: Determine when and if media items should validate themselves
This was NOT done, because it breaks things for reasons that are not directly related to Media. @Wim Leers explained it beautifully in #2877346-25: Media Entity finalish review (disregard if you're not part of the Media Initiative; working around long issue node). In the meantime, Media will do what Taxonomy does, which is...provide a full view mode with tricky title handling. This problem should be solved generically in core, and we can discuss it in the follow-up: #2877400: Find a way to make title handling generic in entity templates.
Done: #2878110: [PP-1] Make revision log and "create new revision" configurable for media items
Done: #2878112: Fix Media JavaScript tests to account for changes in JavascriptTestBase
Done: #2878113: Find possible random errors in Media's JavaScript tests
Done: #2878115: Make the HTML wrapper tag for media items more semantically correct
Done: #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction
Done.
Done.
Done.
Done.
Done.
Done.
Done.
Done.
Comment #392
catchInterdiff looks great, thanks!
Comment #406
gábor hojtsyComment #418
gábor hojtsyComment #430
gábor hojtsyComment #443
gábor hojtsyComment #447
Anonymous (not verified) commentedGábor Hojtsy credited vilepickle.
Comment #451
gábor hojtsyComment #452
catchOK it's taken a long time to get here, but I've just committed 01621e5 and pushed to 8.4.x.
This is by far the single largest feature to go into 8.x directly as stable since 8.0.0 was released, so thanks for everyone's extremely hard work both on figuring out how to do it in the first place, and getting it done.
Looking forward to seeing everyone in the next issues around this.
Comment #453
catchOne extra comment is never not enough. Marking fixed this time :P
Comment #455
gábor hojtsyPublished the change record at https://www.drupal.org/node/2863992
Updated the experimental modules page: https://www.drupal.org/node/2657056/revisions/view/10355994/10491564
Comment #456
nevergoneWell, another content entity type in core. But where is related access layer? #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()
Comment #457
dbt102 commentedComment #458
gábor hojtsyLet's not forget to put this into the release notes ;)
Comment #460
joseph.olstadI believe that this change may have caused a regression preventing the 'contrib' version of 'media' 8.x from being uninstalled from 8.x
Comment #461
OnkelTem commentedThanks guys for your great work of adding this to the Core.
But where is media browser? From what I observe, it's not available in Drupal 8.4.2, there is no information about it both in this thread and on the Media module page. And honestly it's kind of odd that one of the most popular module had been first abandoned with promises it would reappear somewhere in future, and now when the future is here it's not even available in UI, and when you finally enable with drush Media browser is lacking...
Comment #462
oriol_e9g@OnkelTem the media browser is available in contrib https://www.drupal.org/project/media_entity_browser
Comment #463
michaelkoehne commentedIt would be good if the Media API offered similar functionality as the Node API:
hook_node_access_records => hook_media_access_records
hook_node_grants => hook_media_grants
Is there already a feature request for this?
Comment #464
mxh commentedHave a look at #2909970: Implement a query-level entity access API (currently at Entity API contrib module)