Problem/Motivation
#2831274: Bring Media entity module to core as Media module has landed Media entities in core, but they aren't of much use without any proper media source plugins.
We need to replicate current file field behavior as per our roadmap (#2825215: Media initiative: Roadmap). The first step is parity with file fields, which means supporting local documents/files.
Proposed resolution
Adapt the Document plugin from the Media entity document contributed module, and rename it to File.
Then, create a media source for files (called File), with sane default configuration. This media type will be included with the Standard profile as optional configuration.
Remaining tasks
bring in theFile
@MediaSource
plugin (formerly theDocument
@MediaType
plugin in contrib)create media type (bundle) for local files, and add it to the Media modulereview- commit
Follow-ups
- #806102: Locked fields can change the widget but not settings
- #2882801: Review and improve the media creation form
- #2883813: Move File/Image media type into Standard once Media is stable
- #2884379: Improve the administrative Media view
- #2884383: File media source should accept Open Document format(s)
Additionally, #2884202: Non-persistent fields become unpurgeable zombies without their target entity type was discovered here, but does not need to be fixed by/for the Media initiative. See #188.
Comment | File | Size | Author |
---|---|---|---|
#184 | interdiff-2831936-182-184.txt | 1.91 KB | phenaproxima |
#184 | 2831936-184.patch | 24.44 KB | phenaproxima |
#182 | interdiff-2831936-174-182.txt | 2.45 KB | phenaproxima |
#182 | 2831936-182.txt | 24.55 KB | phenaproxima |
#174 | interdiff-2831936-169-174.txt | 672 bytes | phenaproxima |
Comments
Comment #2
marcoscanoFixed typo from copy/pasting
Comment #3
marcoscanoWhat would be the best place for the document plugin to exist? Inside media_entity itself, as a submodule, or in a completely separate folder/namespace?
EDIT: The same question applies to the image plugin in #2831937: Add "Image" MediaSource plugin
Comment #4
marcoscanountested WIP
Comment #5
marcoscanoComment #6
marcoscanoTest steps:
0) Programmatically create a field to be used as source
1) Go to /admin/structure/media/add, make sure our type is available in the select
2) Create a bundle of our type
2.1) Fill in the label
2.2) Select our type on the drop down
2.3) (wait for ajax to finish, then) make sure the dropdown field "type_configuration[document][source_field]" is present
2.4) Make sure our meta-data fields are present (MIME and size)
2.5) Select our source field
2.6) Save the form
3) Make sure the current URL is /admin/structure/media
4) Check that our bundle is listed there
5) Go to admin/structure/media/manage/{bundle_name}/form-display, hide the media name from the form
6) Go to /media/add/{bundle_name}
7) Upload a file, save the form
8) Make sure the resulting URL is /media/{media_id}
9) Check whether the media name is the filename
10) Check that the thumbnail is there, using the generic icon. (an
<img>
is present, pointing to generic.png)Would anyone suggest additional tests, or anything that could be abstracted into a common test base?
Comment #7
marcoscano1) Go to /admin/structure/media/add, make sure our type is available in the select
2) Create a bundle of our type
2.1) Fill in the label
2.2) Select our type on the drop down
2.3) (wait for ajax to finish, then) make sure the dropdown field "type_configuration[document][source_field]" is present
2.4) Make sure our meta-data fields are present (MIME and size)
2.5) Save the form
3) Programmatically create two fields attached to this bundle:
- one field of a supported type
- one field of a non-supported type
4) Go to /admin/structure/media/manage/{bundle_name}
5) Make sure only the supported field type is shown in the dropdown
6) Save the form
7) Make sure the current URL is /admin/structure/media
8) Check that our bundle is listed there
9) Go to admin/structure/media/manage/{bundle_name}/form-display, hide the media name from the form
10) Go to /media/add/{bundle_name}
11) Upload a file, save the form
12) Make sure the resulting URL is /media/{media_id}
13) Check whether the media name is the filename
14) Check that the thumbnail is there, using the generic icon. (an
<img>
is present, pointing to generic.png)Comment #8
marcoscanoComment #10
marcoscanonow with full index line in the patch
Comment #11
marcoscanoComment #13
marcoscanoComment #14
phenaproximaGenerally this looks good to me. Nice test coverage :) I have a few comments...
Can this be "MIME type"?
Can be
return $file->getMimeType() ?: FALSE
for brevity's sake.0 is a valid file size. Not sure why anyone would upload a 0-byte file, but it's a valid size. We might want to change the return check to is_numeric().
Let's put two dashes between $mimetype[0] and $mimetype[1], since either of those could contain dashes as a matter of course. For example, application/octet-stream will become application-octet-stream.png, which is slightly confusing; application--octet-stream.png is clearer.
Can we get the icon_base as a variable so that we don't need to keep calling the config factory?
This if structure could be flattened more, for clarity's sake.
Nit: There's an extra line of white space.
Let use use $this->randomMachineName(). ;)
I think \Drupal::root() is exposed as a property of the test class ($this->root?); it would be preferable to use that.
Can we test with an actual document instead of an image, just to avoid stepping on the image plugin's toes? Maybe a .txt or a .pdf would suffice here.
Should be MediaTypeTestBase.
s/id/ID.
Comment #16
chr.fritschComment #17
marcoscano@phenaproxima thanks for the review!
Addressed all points in #14, except for:
#14-9: I couldn't find it, is it called something else perhaps?
This new patch also creates a default bundle for document. We are not yet including a test for that, because it will be better to wait until #2625854: Provide default source_field when creating new media entity bundles lands, so we can rely on the source field being automatically created.
Comment #19
marcoscanoThis should deal with the testbot complain
Comment #21
marcoscanotestbot I still love you
Comment #23
marcoscanoFor manual testing only (automated tests still failing, work in progress)
This patch needs to be applied after the patch #92 from #2831274: Bring Media entity module to core as Media module is applied.
Comment #24
marcoscanoThis is a conversion of #23, renaming "Document" into "File", which can then be used as a generalization and be extended by the image handler, for instance.
Comment #25
marcoscanoPatches:
- interdiff: The interdiff with the previous patch
- full: Against 8.3.x HEAD (including #2831274-92) to trigger all drupal tests
- only-plugin: The real work done here, to be applied on the top of #2831274-92
Comment #26
dawehnerI'm wondering whether file should be document instead? File seems really generic as image is also a file.
Nitpick: Using
$media->get($source_field)
is easier to read IMHOMaybe a few documentation words about the logic would be nice. Note: In an ideal world this method would have a corresponding kernel test
$field doesn't exist here, so we could remove this line
That's the only non UI code in this test, I think we should use the UI for this change as well. Mixing UI and non UI code in tests is tricky to deal with
Nitpick: this should be the other way round, as
$expected
should be the first parameterDo you mind opening up an issue for that? Do you know why this is the case? Note: There is
\Drupal\FunctionalJavascriptTests\JavascriptTestBase::assertJsCondition
for that kind of stuff as wellComment #28
pguillard CreditAttribution: pguillard commentedComment #29
pguillard CreditAttribution: pguillard commented@marcoscano :
I just applied the media_entity to media renaming (This patch is based on media 2831274-99.patch),
and changes according to Daniel's comments #3, #4, #6
Hope it is fine.
Comment #31
marcoscanoThanks Daniel for reviewing, and Philippe for starting to work on it.
The patch attached finishes the "renaming" process and also addresses most of the review in #26.
At this point the patches attached were both based on 2831274-99.
Some comments:
26-1: As commented with @dawehner on the sprint, the idea is that the Image handler extends this plugin, so this name is probably the most appropriate in this case. A "Document" plugin can be created also extending "File", if needed to expose specific UI improvements for documents.
26-2: As part of the work being done in the Image plugin, this is being refactored to use a getter in the base class.
26-3: OK, left the "ideal-world part" postponed until we get a little closer to that point :)
26-4: OK
26-5: OK
26-6: OK
26.7: Pending. I will ask @mtodor tomorrow about that because he is the one who whispered me this solution, so he may be able to help identifying the cause and filing an issue in that case.
NOTE: Sorry I am unable to provide a clean interdiff between 29 and 31 at this point. I had to update core due to the
RevisionableEntityBundleInterface
inclusion, and then messed up with the local branch I was working on.Comment #32
marcoscanotestbot please do your work
Comment #33
marcoscanoMissed the renaming in two spots, corrected now.
Comment #36
marcoscanoRebased after 2831274-120, and refactored accordingly.
Comment #37
marcoscanoRebased and refactored on top of 2831274-136
Comment #39
mtodor CreditAttribution: mtodor at Thunder commentedNice work! In my opinion, this is complete.
I have few nitpicks, but nothing crucial.
Comment should be corrected, since source field has to be present. Also there is no need to create new variable ($file), everything can be one expression: return $this->getSourceFieldValue($media)->getFilename();
This functionality should be documented, it's not so obvious, why it's here.
Comment #40
chr.fritschI rebased this patch on 2831274-147. Also i fixed comments from #39 and added the entity display and form display.
Comment #42
phenaproximaNit: 'mime' should be MIME.
MIME
Missing @throws annotation.
I think this part of the method should be moved into MediaHandlerBase. It's too useful not to have that everywhere. Then this handler can override and extend the base implementation with its additional logic.
Comment #43
seanBRebased #40 to the latest media patch #2831274-176. Fixed comments 1/2/3 from #42.
The last comment is not fixed, the getSourceFieldValue is specific for file based plugins. It fetches the file entity from the source field. The handler base does contain a getSourceValue method to fetch the main property for the source field (I think that's the file ID). Both functions are useful but since they have similar names this might be confusing. To fix this I renamed the getSourceFieldValue function to getSourceFile.
Most media plugins probably won't have file entities in their source fields anyway, so I don't think this is an issue. I did however change the media patch and added the check to getSourceValue in the MediaHandlerBase.
Comment #45
seanBJust saw the comment in https://www.drupal.org/node/2831937#comment-11829154 about the changed test. Updated so it will hopefully pass. Also moved some generic code needed by the file and image plugin tests to MediaHandlerTestBase.
Comment #47
seanBOk, forgot to rename something and there is also a function that could be removed since it is no longer used.
Comment #48
tedbowLooking good! just a couple thoughts
For an example mime type of "image/jpeg" this code will look for jpeg.png and image--jpeg.png but not image.png. Is this intentional? Seems like having a common icon for all images would be common.
Also do we think this type of icon look up would be common? If so maybe we should have general function in MediaHandlerBase like
function getIconFile($parts, $fallback = 'generic.png')
This would handle looking for all possibilities and concat '--' etc.
Then could all it here like
$this->getIconFile(explode('/',$file->getMimeType())
Should this somehow explain to the user they will not get media type specific features? For instance if I use this handler to upload an image instead of the Image handler I won't get the image thumbnail, correct?
If you have media before this might be obvious but from the description it would be easy to assume you could use it for any local files and get the image thumbnails.
Same comment about the description.
Comment #49
seanBThis could/should probably be added. Will look at that.
For now I think this is the only handler directly handling files and dealing with generic icons. The image handler uses the images for thumbnails and other handlers like youtube and instagram etc will also need to provide their own thumbnails. Since every handler should (imho) provide their own icon(s), it's probably best not to have this in MediaHandlerBase.
I don't think this is supposed to explain anything about media type specific features. The file media type is supposed to replace the file fields in core. There is no real explanation on the difference between file fields and image fields. In the current situation you also won't get a thumbnail when uploading an image in a file field. If you have any suggestions on how to improve this please let me know :)
Comment #50
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedIcons logic is still more or less the same as it is in contrib. Note that there is #2833768: Add media type icon SVG files for all the media types to core icons, which definitely influences this. I expect icon logic to change/adapt based on what is done there.
Comment #51
Gábor HojtsyYup, is media worse in some way then the existing difference between file and image fields?
Comment #52
tedbowThat is good point. It is the same problem so need to hold up this issue.
Regarding my icon comments in #48, explanations in #49 and #50 make sense and because my suggestion could easily be added later.
So regarding my comments in #48 I think the current state of the patch is good.
Comment #53
tedbowDo we actually have to test this? Isn't this standard core machine name element? If the machine name is not correct won't it fail later anyways in this function when we test for it's existence on /admin/structure/media
There is nothing to enforce that the File source field is required it doesn't make sense throw this expection if there is no source file.
I tested this and I can make the field not required. Then when I try to create a media entity without a file I get this hard error.
Comment #54
Wim LeersI typed an extensive review. Then lost most of it because Dreditor and Chrome. So this one is a bit more terse.
s/Files/File entities/
I'd also expect an
@see \Drupal\file\FileInterface
to stress the relationship.Omit "documents", because not relevant, nor mentioned anywhere.
Omit "business logic", because it doesn't help at all at
/admin/structure/media/add
Suggestion:
Allows local files to be used as media.
These keys should be constants on an interface.
These places would benefit from them being constants on an interface.
icon_base
has been renamed in #2831274: Bring Media entity module to core as Media module. So this is broken now.s/magically/automatically/
s/MIME information/MIME type/
This needs explicit test coverage.
The description sounds strange. And can even be omitted, because it's so trivial.
Gets File entity from given Media entity.
s/The/A/
s/present in/referenced by/
Nit: 80 cols.
s/media file/file media/
Test coverage is missing.
The message doesn't make sense (as Ted apparently already indicated in #53).
Test coverage is missing.
"handler plugin" again. It's either "media handler plugin" or "media handler", but never "handler plugin".
Again confirms my concerns in #2831274.
Eh… That's not a good description.
Let's at least be somewhat consistent: s/MediaFilePlugin/FileMediaHandler/
s/image/file/
Neither the comment nor the method name make sense. Rename the method to
hideMediaFieldWidget
and the comment can be removed :)"media item"… see discussion in #2831274.
Didn't know this assertion method, cool, thanks :)
s/created/displayed/
What about the
size
field?This comment doesn't make sense.
- media types?
- media handlers?
- media handler plugins?
I've seen it all, and now "plugin types". This really points to how problematic the current naming is, i.e. how problematic "media handler" as a plugin type is.
Nit: s/Create/Creates/
Even these are inconsistent… They should all be:
Nit: Omit
$config
, it's not used anywhere.s/field/field widget/
"Helper to" are not acceptable test method descriptions.
Nit:
s/hide/Hides/
s/from/in/
The
Test
suffix in this helper method is very strange/unnecessary/Nit: Is there really only one with this class here?
And why does this use jQuery?
Again further indication that "media type" config entity is almost always the same as the corresponding "media handler" plugin. Because this is not testing a type like the comment says, but a handler.
If even test authors are confusing this, what will it be like for end users?
Huh? This is modifying the
Standard
install profile?This doesn't make sense for two reasons:
I expected this to live in the non-optional configuration of the
media
module.Comment #55
Wim LeersI have prove of #54.5 being broken: it points to
http://d8/sites/default/files/styles/thumbnail/public//generic.png?itok=hWcY1sS1
, and hence looks like this:Comment #56
Wim LeersAlso, the issue summary explains #54.2:
And it again confirms the confusion/complexity/strangeness around "handler" vs "plugin" etc, that I mentioned numerous times in #54:
So it seems to have first been called "plugin" or "plugin handler", then just "handler". But it really is still a plugin, so this notation is wrong, so it's actually a "'bla bla handler' plugin".
And even the prefix for "handler" cannot be agreed upon: "media type" is written here, but "media" is what is in the actual annotation.
Hence this is a critical thing to fix before this or #2831274 are committed to core.
Comment #57
Wim LeersComment #58
Wim LeersAlso see #2831274-206: Bring Media entity module to core as Media module, which I was able to write thanks to reviewing this issue.
Comment #59
naveenvalechaThank you all reviewers.The patch in this issue is not in sync with the latest patch in #2831274: Bring Media entity module to core as Media module Postponing this on #2831274: Bring Media entity module to core as Media module so that there would not be back-forth b/w issues.
//Naveen
Comment #60
Wim LeersKeeping up with API changes at #2831274-246: Bring Media entity module to core as Media module.
Comment #61
Wim LeersKeeping up with API changes at #2831274-248: Bring Media entity module to core as Media module.
Comment #62
Wim LeersKeeping up with API changes at #2831274-253: Bring Media entity module to core as Media module.
Comment #64
drubbKeeping up with API changes at
Comment #65
drubbKeeping up with API changes at #2831274-286: Bring Media entity module to core as Media module.
Comment #66
seanBWorking on implementing API changes of #2831274: Bring Media entity module to core as Media module
Comment #67
seanBThis one is based on #332 of #2831274: Bring Media entity module to core as Media module. Just uploading for now, will look at the reviews later!
Comment #68
seanBSo I just addressed some of the comments from #54 that were not done yet. Still based on #332 of #2831274: Bring Media entity module to core as Media module but should work with #336 as well.
Still todo:
#54.8
#54.33
#54.34
#54.36
Done:
#54.6 Done
#54.7 Done
#54.8 TODO
#54.17 Done
#54.20 Done
#54.21 Done
#54.24 Done
#54.25 Done
#54.28 Done, renamed type name / machine name everywhere to type id.
#54.29 Done
#54.30 Done
#54.31 Done
#54.32 Done
#54.33 Yes, but since the method already exists in
MediaFunctionalTestCreateMediaTypeTrait
we can just remove it. Any suggestions?#54.34 TODO
#54.35 This is now called source, so I guess this is no longer an issue?
#54.36 Excellent point, I want to move it but maybe marcoscano can comment on the reason to do this?
Comment #69
seanBAdded missing patches...
Comment #70
seanBSorry, 54.2 is also still TODO!
Comment #71
marcoscanoRe #54.36, if I'm not mistaken the original idea was to follow more or less what the standard profile assumes for nodes.
In other words, just as the standard profile creates 2 content types, some fields, view/form displays, etc, the equivalent for media would also be created there. This would mean that sites that want to use media but not necessarily use these source plugins / fields / etc, wouldn't have them created just by enabling the module.
It is true that the upgrade paths would need to take care of, at least:
- Existing sites with the media_entity contrib module
- Existing sites without the media_entity contrib module, that upgraded core and then enabled the media module
There is probably some room for more discussion here, to look for alternatives that would better deal with all these scenarios.
Comment #72
mtodor CreditAttribution: mtodor at Thunder commentedIt's really nice to see media into core is moving forward! ::thumbsup::
I did quick check and found two things that would be nice to have.
I like this functionality and it would be really nice to add test that covers it (maybe there is already test for it, forgive me if I have missed it). Maybe it's possible just to clone generic.png into 'text-plain.png' and check is that icon used for new README.txt media entity.
This should not be used, instead fallback should be on parent, where annotation will be used
default_thumbnail_filename = "generic.png"
Comment #73
seanB54.2 Done
54.8 Done
54.34 Removed as suggested in #53
54.36 We discussed this today. As marcoscano already mentioned providing configuration for the media types is not something you should always enable. The standard profile is probably the best place.
#72.1 Done
#72.2 Done
Comment #74
Wim LeersVerifying that my #54 review is addressed. All points are addressed, except:
I'm explicitly reviewing the remarks in #54 that requested test coverage separately:
File
media source plugin thanks to architectural improvements in #2831274: Bring Media entity module to core as Media module :)#71: RE: #54.36: Aha, thanks for clarifying :)
However, this would AFAICT be the first and only case of configuration in the Standard install profile that is configuration that is optional.Apparently there already is optional configuration in the Standard install profile: image styles and responsive image styles. So this addresses #54.36.1.However, that still leaves the concern I raised in #54.36.2 for existing sites. I don't see why it's preferable to have this optional configuration in the Standard install profile instead of in the
media
module. This definitely needs further discussion.To be clear: I am okay with the current approach, I'm only saying we should consciously weigh "optional in Standard" vs "optional in
media
".#72.1: hah, that's also what I asked in #54.8 :)
#72.2: agreed.
And finally, a complete review:
As of #2831274-351: Bring Media entity module to core as Media module, the Media module's description is no longer but . @yoroy decided this was much better (and I think everyone will agree).
We should update this accordingly.
seems the logical consequence.
Still should be class constants, as I said in #54.3.
This is no longer used, can be removed.
We need a comment here: when can this happen?
Nit: I think all the newlines here are kind of atypical: we don't have any
\n
in mostswitch
statements.So are we creating a media source or a media type? Media type. Let's remove this comment.
s/mime/MIME/
s/attribute/attributes/
s/mime/MIME/
field type !== field storage type
$media_type_id
would be a better name than$type_id
.The
\n
there makes it seem as if the comment applies to the first two lines. But it actually applies to everything I quoted, and more (i.e. several lines thereafter). Let's remove the\n
for clarity.s/array()/[]/
The created media type.
So I already mentioned this
Test
suffix in #54.3. It's still here.Turns out it's here for good reason: it's also testing this!
So then I would suggest
doTestCreateMediaType()
as the method name. This would be consistent with for example\Drupal\block_content\Tests\BlockContentTranslationUITest::doTestBasicTranslation()
.s/page/form/
This matches what
\Drupal\file\Plugin\Field\FieldType\FileItem::defaultFieldSettings()
has, great!This is a superset of
\Drupal\file\Plugin\Field\FieldType\FileItem::defaultFieldSettings()
— this addspdf
."uploading local files" sounds fairly strange.
What about:
Comment #75
l0keDone.
#74.6 I've removed a newline before the first case and after
default:
but removing all newlines breaking the code standards. Therefore coreswitch
statements are 50/50 with/without newlines afterbreak;
. Even though we have areturn
statements it is doingbreak;
implicitly.So probably we should leave \n there.
Comment #77
l0keMy bad 2831936-75-full.patch haven't been a -full actually.
Correct set of patches.
Comment #79
seanBAwesome work l0ke, thnx!
#54.36
So when you have an new/existing site with a minimal install, we decided you shouldn't get our default configuration. Same as you don't get page/article node types when you install. You could argue that this is a different situation and I personally do not have a strong opinion on this. But we kind of compared our situation to the node types.
#54.34 The jQuery test for the machine name is also used in
Drupal\FunctionalJavascriptTests\Core\MachineNameTest
. Since it is tested there we might just remove this? This is in the main core patch too btw #2831274: Bring Media entity module to core as Media module#74.5 I told l0ke this shouldn't happen and he could just remove it, but in theory you could create your own field to a media type that is not required. So maybe it better to add a comment.
#74.6 l0ke already commented on this. I think it's fine now.
So I think this leaves us with:
- #54.34: Should we just remove the machine name test here?
- #74.5: Move back the check for $file and add a comment.
Comment #80
l0keThank you @seanB!
#74.5 Put condition back and add the comment.
#77 Add missing MediaSourceInterface import where used.
Comment #81
phenaproxima#2831274: Bring Media entity module to core as Media module has landed!
As laid out in #2825215: Media initiative: Roadmap, this issue is now the top priority for the Media team.
Comment #82
phenaproximaJust realized that this will likely need a reroll to account for architectural changes in Media.
Comment #83
chr.fritschHere is the reroll
Comment #84
chr.fritschRe #54.34
Since it is already tested in Drupal\FunctionalJavascriptTests\Core\MachineNameTest , i think it's fine if we remove that line. We are basically just using core functionality which is already tested in core.
But still, we have to wait until machine name is set. assertJsCondition does that implicitly.
As far as i see, all other comments are addressed, or is the currently something else open?
Comment #85
Wim LeersNo longer postponed :)
Indeed! So going over the patch once more…
I think this meant to say:
That's literally the only thing, and can clearly be fixed on commit.
Comment #88
phenaproximaThese do not belong on MediaSourceInterface. They are specific to files. I agree that they should be constants, but they should be constants of the File source, not the generic MediaSourceInterface. Can we move these?
Comment #89
Wim Leers#88 was surfaced in IRC by @phenaproxima, and I agreed:
Comment #90
seanBFixed the comments in #85 and #88. Please review!
Comment #92
phenaproximaNice. That looks perfect. I'd still like to do a complete read-through of the patch, but I doubt I'll find anything (it passed the @Wim Leers test, after all). Should be able to get to that today, then will restore RTBC if all looks good.
Comment #93
phenaproximaI don't think this is used...
I think both of these need
@var string
annotations.The documentation for MediaSourceInterface::getMetadata() dictates that NULL, not FALSE, should be returned if there is no value. https://api.drupal.org/api/drupal/core%21modules%21media%21src%21MediaSo...
We could just use entity_get_form_display() here.
Should we use entity_get_form_display() here and only save the changes if the display is not new?
These should use $assert_session.
Nit: We don't need the if (!empty()) check. If $provided_fields is empty, the foreach will just be skipped anyway.
I'm not sure what's gained by adding optional config to standard. Standard does not include Media, so unless I'm missing something, this will never be installed...?
Comment #94
chr.fritschFixed 93.1, 93.2 and 93.3
Not sure about 4 and 5 because entity_get_form_display is deprecated
Fixed 93.6, 93.7
93.8 If you install media installation, all the configs will be installed automatically. So the optional config makes totally sense.
Comment #95
phenaproximaThey were deprecated without anything to replace them. I have a patch (stalled) that moves their functionality into the entity display repository service and turns them into wrappers around those methods: #2367933: Move entity_get_(form_)display() to the entity display repository. Until that lands, it's OK to use those functions. :)
Comment #96
chr.fritschOk, adjusted comments 4 and 5 form #93
Comment #97
phenaproximaMy feedback, she is addressed. Restoring RTBC!
Comment #98
vijaycs85Just installed media module and tried to create a media type and found that it is not possible without this issue. Happy to see its already RTBC. By the look of it, without this patch, we wouldn't able to use media module at all(?).
I did a manual test with the patch in #96 and here is some findings:
Am I missing something or is it by design?
Comment #99
phenaproximaThat's not exactly intentional, but it is known. The reason is that the options you see are the available field storages, and that is how they label themselves ($field_storage->label()).
The field is intentionally locked, but if it means you can't change entirely reasonable settings, that is a problem. The only reason we need the field to be locked is so that it cannot be deleted through the UI, which would break the media type entirely. This is probably worth filing a related issue about.
Comment #100
vijaycs85thanks for the quick reply @phenaproxima.
Right. the field edit tab holds settings like "Allowed file extensions", directory etc. As you can see currently the field supports only txt extension. We can have follow up to discuss/implement option to edit settings (without delete option), but at least we should allow common extensions (png, gif, jpg, jpeg, pdf) for this issue IMO.
Comment #101
phenaproximaI agree...but let's not support image extensions in this issue. We'll do that in #2831937: Add "Image" MediaSource plugin, which directly extends the work being done here.
I propose we preconfigure the field with txt, doc, docx, and pdf extensions. So I'm kicking this back for that (with tests).
Comment #102
johnwebdev CreditAttribution: johnwebdev commentedI gave #101 an attempt. Not sure if you accept patches from strangers, but I was bored and if you aren't, i'm sorry!
Comment #103
johnwebdev CreditAttribution: johnwebdev commentedComment #104
phenaproximaThanks, @johndevman! We accept patches from anybody. Drupal is not a closed-door effort, and the Media initiative will take all the help it can get!
Comment #105
phenaproximaThe patch in #102 looks damn good to me. A few requests:
Rather than copy all the code from MediaSourceBase::createSourceField(), we can just do this:
return parent::createSourceField($type)->set('settings', ['file_extensions' => 'txt doc docx pdf']);
Nit: There needs to be a blank line after the opening brace of the class.
Asserting the placeholder HTML and actual list of extensions is a little overzealous, IMHO; let's loosen this a bit to:
$this->assertContains('Only files with the following extensions are allowed:', (string) $result->get(0)->getMessage());
Nit: There should be a blank line before the closing brace of the class, and we'll need a new line at the end of the file so Git doesn't complain. :)
Comment #106
vijaycs85Fixed all items in #105
Comment #107
naveenvalechaSorry I don't want to put it back to N/W but requesting few changes
While we're on this, please replace it short array syntax
Move this method to MediaKernelTestBase so that we could re-use it for other(Image) MediaSource Plugins
test.patch should be $filename so that we could reuse it for "test.patch" and "test.txt"
Comment #109
seanBI think that expanding the options for now is good enough. I do think we need to re-evaluate the locking of fields. For file/image fields changing the settings is not a problem. A entity reference source sometimes doesn't want to allow you to change the entity types and bundles you want to reference. Deleting a source field for a media type should never be allowed.
How about:
Not sure what the best way is to stop a field from being deleted, but always locking all settings is obviously not what we should want.
Comment #110
Wim Leers#99:
Hm, but I remember from the DrupalDevDays Seville reviews/analyses/discussions about Media that locking those field definitions was crucial. So I'm very much hoping we won't have to change that decision.
#98 + #100 + #101: hm, how come this is only allowing
txt
? Earlier, the patch was allowingtxt pdf
infield.field.media.file.field_media_file.yml
, but apparently we somehow lost that?#102 + #105: hm, we didn't need that before, it's a bit unfortunate that we now apparently need to override
createSourceField()
. But apparently this is something that was overlooked all this time?#109:
Well, not having it be locked also means it can be deleted? And only some settings can be safely modified, IIRC?
Indeed, but so then the real question IMO is:
Comment #111
johnwebdev CreditAttribution: johnwebdev commented#110 We do have 'MediaSourceFieldConstraintsInterface' interface which allows us to add constrains to the source field, but Implementing that interface seems redundant to me at a first glance for this source, as the constraint for file extension is already supported by the source field itself. I have not been around during the discussion of those interfaces so I'm not sure though.
Comment #112
seanBJust saw this is also in
MediaUiJavascriptTest
. We should probably change it there as well. Or not change it here, don't have a strong opinion on that#110
The base for this whole discussion was #291 and the solution summarized in #294 of the main patch issue. I think the solution I proposed (optional field locking and not allowing deletes for source fields) could be valid. Depends on if we can stop users from deleting a source field. But maybe stopping fields from being deleted is a separate issue which should be solved more generic (if not possible yet).
I don't think we overlooked this. It was discussed in the main patch, and I mentioned this in #294 and #295. Not sure if this is a bad thing, every media source is supposed to create a source field.
I think what I proposed above is still valid here. We could optionally lock a field and find a way to stop fields from being deleted. For file and image fields you can safely change everything (that's why it came up so late), but for entity reference source fields it could lead to issues. But allowing granular control over locked field settings sounds pretty nice and could definitely work as well.
Comment #113
Wim LeersI personally don't know the intricacies of field configurations well enough to approve or disapprove this approach. I'm mainly trying to ask critical questions that hopefully contribute to get us to a good place :) I think this definitely needs further discussion with the right people before it can be RTBC again.
Comment #114
seanBMaybe we should first allow some sane defaults for file extensions in this patch and discuss when and how to lock fields in a separate issue?
Comment #115
chr.fritschYes, i agree. We have "txt doc docx pdf" as defaults. Which is ok for now. I would also propose to move the discussion about field locking into a separate issue. These discussion should not block this issue.
FYI: I removed the locking in #2831274-161 and implemented hook_entity_access to prevent deleting a source field. Not sure when and why this was removed.
Comment #116
chr.fritschFixed all the comments from #107
Comment #117
naveenvalechadescribe the argument name to what it is. Change the argument name $name to $attribute_name
We already have an issue where the locked field could be altered via UI #2274433: Do not allow to alter Locked field via UI
Amen on having the txt, doc, pdf as defaults.
//Naveen
Comment #118
vijaycs85thanks @chr.fritsch. interdiff looks good.
Looks like we lost all assert lines(except the count). Is it intentional?
Comment #119
chr.fritschYes, because the second $media->validate() returns no errors. So there is nothing to check in the ViolationsList
Comment #120
phenaproximaThis patch looks great!! Only a couple of tiny nitpicks and it is RTBC from me.
Rather than return NULL if the MIME type is not known, we could inject the MIME type guesser service into this plugin and try to guess the MIME type :) But that's a follow-up, and not an essential one. Just thought I'd mention it!
Nit: This should be MediaTypeInterface.
This should be MediaTypeInterface.
Nit: There's an extra blank line.
Comment #121
marcoscanoAddressing #120
Comment #122
phenaproximaGorgeous. Pre-RTBC.
Comment #123
marcoscanoAnd now addressing the comment left from #117 that was not addressed yet. Sorry for overlooking it.
Comment #124
phenaproximaNice. Back to RTBC on the assumption that the tests will pass.
Comment #125
catchDoes this happen once when the file is uploaded or is it runtime?
This is just defaults right?
On the one hand this is good re-using the file, on the other the test will break if we change to README.md or something so might be better to have a .txt test file somewhere?
Is this really needed in the standard profile?
Comment #126
phenaproximaResponding to #125:
If I'm grokking things correctly, the answer is "maybe both".
getThumbnail()
will be called whenever the thumbnail_uri metadata attribute is requested; I think this attribute will be computed and saved when the file is uploaded, but then other code could also callgetMetadata('thumbnail_uri')
at any time, which would call that function.Yes. The source field definition is created once, then saved. It's saved as a locked field, but we're working on a related issue to ensure that those configuration values can be changed.
This seems like a bridge we can cross when and if we get to it. I'm speculating here, but it wouldn't hugely surprise me if there are other tests in Drupal that expect README.txt to exist and will break if it gets renamed.
Not sure where else it should live. If we put it all in Media, then we are not following standard practice (node types, etc. all ship with the profile). So IMHO it's in the right place...
Comment #127
johnwebdev CreditAttribution: johnwebdev commented#125:
I agree we should change this.
I agree with #126. If you use something else than Standard profile you are most likely setting these things up yourself anyway.
Comment #128
Wim Leers#125:
RE: thumbnail calculation happening during upload time or run time: see
\Drupal\media\Entity\Media::baseFieldDefinitions()
, specifically thethumbnail
field. That stores the thumbnail URI. Then, see\Drupal\media\Entity\Media::getThumbnailUri()
+\Drupal\media\Entity\Media::updateThumbnail()
. That shows the logic: new media items have their thumbnail generated, when the media item is changed, it's refreshed, and otherwise it's just returned from storage. We even have thorough test coverage for this in\Drupal\Tests\media\Kernel\MediaSourceTest::testThumbnail()
.Comment #129
Gábor HojtsyReviewed the code, only found a few things that were not raised above:
I think the code doc explanation is much more user friendly and concise. We don't need to repeat it is a media type, also we don't need to repeat the name of it, given the description is displayed with the name, etc.
These do not have corresponding fields attached to this media bundle. Is that a best practice?
The lack of editability of these settings was discussed above, #2274433: Do not allow to alter Locked field via UI was added as a related issue, but that deals with the opposite problem. Let's open the followup to discuss this because we lack that issue. @phenaproxima in #126 says "we're working on a related issue to ensure that those configuration values can be changed."
Is not supporting new revisions by default intentional or just a matter of accepting defaults as they were? I mean in terms of supporting workflows out of the box, etc.
Tested the user interface as well and found the following problems, some of which may need to be resolved here, some elsewhere:
/sites/default/files/styles/thumbnail/public/media-icons/generic/generic.png?itok=yWxZ5Pdm
which does not work of course, that prints "Error generating image.". And indeed, it looks like an odd combination of an image style path with a hardcoded image path.Reviewing the issue itself:
These are all the things I found, overall great job!
Comment #130
Gábor HojtsyComment #131
Wim LeersHowever, it is probably a good idea to have the sole/single/first media source's default configuration actually do this out of the box, because that will be necessary if you want to create a media library that can filter/sort by MIME type and size later on. I defer the specifics to Media initiative experts.
Great catch! :)
This is where media source metadata attributes can be mapped to a particular field.
Yes, I remember there being many problems with this: the root cause is somewhere deep in the routing/menu system. Screenshot of HEAD, without Media:
Wonderful review, Gábor — thanks!
Comment #132
Wim LeersForgot to attach the screenshot.
Comment #133
vijaycs85#129.3:
- Updated title of #2274433: Do not allow to alter Locked field via UI to avoid the confusion
- Real follow up of what we want here is at #806102: Locked fields can change the widget but not settings (I guess) not sure if we want to port to D8 or ok to create a separate issue for D8
Comment #134
Gábor HojtsyI think #806102: Locked fields can change the widget but not settings is the right issue for that, updated its version and added it as related here.
Comment #135
phenaproxima#129.1: Fixed by @Wim Leers. Thanks!
#129.2 and #131.2: I don't think we need that. Adding fields to store MIME type and size is redundant -- the media type already wraps around a file, which has that information anyway. So we'd effectively be double-storing that information, which means we'd need to keep it in sync if the source field changes, and so forth. It's a can of worms we shouldn't open. Wim raised the point that you might want to create a media library that filters by MIME type -- but you could simply do that via a Views relationship to the wrapped file, rather than mess around with specialized MIME type and size fields.
#129.3: Follow-up confirmed.
#129.4: I see no reason why we should disable revisions out of the box. Let's fix this.
Given Wim's comment in #131, it sounds like this is out of scope for us to fix in this issue. Is there an issue already open to address this? /admin/content/media is provided by a view, packaged with Media already. Why isn't it appearing?
No. I'll open one.
Uh-oh. Let's fix that here and add test coverage.
Comment #136
Gábor HojtsyI set up two more test sites and neither had the Media tab problem. I had my testing site as a fresh site as well, so don't know why was that different honestly, but it was. But statistically we can ignore that since it worked on the other two attempts on the same patch.
Comment #137
BerdirThat's likely a known module install/caching issue actually, if you had the local tasks on that page cached before and then install the module without any cache clear then they're not invalidated.
See #2783791: Module install doesn't invalidate render cache
Comment #138
phenaproximaFiled #2882801: Review and improve the media creation form to address this point from #129:
Comment #139
phenaproximaAdding #2783791: Module install doesn't invalidate render cache as a related issue, as per @Berdir's comment in #137.
Comment #140
phenaproximaAdding #2836153: Improve metadata mapping UI on Media type form here, since it came up in IRC discussion today. Specifically, this comment by @seanB:
Comment #141
seanBFollow ups were created for all other issues. Only the CR and related issues for an upgrade path from media_entity_document need to be created.
Comment #142
Wim LeersWe've had this problem in the past too. Some git setups seem to corrupt PNG files. Possibly automatic
CRLF
fixing.Are you saying that sites using https://www.drupal.org/project/media_entity_document should migrate to the File@MediaSource
plugin?The answer is in the IS:
So that was simply the original code. I think I'm hearing we don't want to block this issue on such an upgrade path?
This implementation is now violating the API: it's returning
NULL
. It must at minimum doreturn [];
.Do we need test coverage for this?
Comment #143
Gábor HojtsyI don't think we want to block this on an upgrade/migration solution as we did not do that for the core modules either, but it needs to be mentioned in the change notice and issue opened to work on it eventually so we can prioritize it.
Comment #144
Wim LeersThere is an important difference though: this is for a 8.3->8.4 migration. D8 sites that currently exist and use Media will need to update to 8.4 to remain secure. Therefore, their media data needs to migrated too.
We didn't require a 7->8 migration for core modules, but that's different, because those sites can choose to remain on D7 for much longer than the 6 months of support for each Drupal 8 minor.
Comment #145
marcoscanoIs there anything in core 8.4.x that would prevent sites from continuing to use the contrib
media_entity
in its 1.x branch? (I don't think so, but I'm not 100% sure) If that's the case, existing sites could always opt to upgrade drupal without moving to the new "media-in-core" solution (even if this would not be the recommended path).In practice though, this is something I believe will happen anyways in many cases, because existing sites will be using a bunch of contrib plugin/source modules (not only documents + images), so until all of them have stable 2.x branches compatible with the new API, they will not be able to switch.
Comment #146
phenaproximaNot exactly :) The data model is essentially unchanged between Media Entity and core Media. The modules just need to upgrade to the new API, and that's it. Media will include an update hook to do the requisite config renames and whatnot, but that's the extent of it.
Comment #147
seanBI guess there should be an upgrade path in media_entity_document to move to the file source plugin in core. That's out of scope for this issue, but is definitely be a must have to consider this "stable". Same as the upgrade path for the media_entity module.
Comment #148
Wim LeersWell, happy to be wrong! :) But that makes me a bit scared about the upgrade path in general. Where can I read about that? Zero mentions of it in the overarching plan issue: #2801277: [META] Support remote media assets as first-class citizens by adding Media Entity module in core. I think it might be a good idea to document the plan/recommendations that I see being written here in that issue, so that it's A) clear, B) findable.
Comment #149
Gábor HojtsyYeah all I asked is the upgrade path from the contrib module considered (in this case, the plugin name is not even the same + metadata will be "lost", etc.) and an issue opened at the appropriate place (contrib module or core depending on where it will be implemented) and related to this issue, so people reviewing the change will be aware of the upgrade implications. (Also it does help that *we* discuss the upgrade implications a bit without need to resolve it outright of course).
Comment #150
phenaproximaPer @Wim Leers' request in #148, I have documented the plan for the upgrade path over at #2801277-18: [META] Support remote media assets as first-class citizens by adding Media Entity module in core.
Comment #151
Gábor HojtsySuperb. Let's open that issue too for the contrib media document module (or core as appropriate) for upgrade path to this core feature from contrib. Comments in other issues does not work to follow progress on specific tasks for people who need this upgrade path. They also don't work for prioritizing work, etc. :)
Comment #152
phenaproximaAn upgrade path issue has been opened for Media Entity Document (#2883449: Provide upgrade path for media_entity_document config changes to Drupal core). Thanks, @seanB!
Comment #153
phenaproximaDraft change record written: https://www.drupal.org/node/2883488
Comment #154
seanBDrupal\Tests\media\Functional\MediaUiFunctionalTest
. This is not something specific for the file source?Comment #155
phenaproximaI think all feedback from Gàbor's review in #129 has been addressed. Let's land this!
Comment #156
andypostit needs follow-up to add support "odt" extension!
Comment #157
vijaycs85@andypost #806102: Locked fields can change the widget but not settings is the follow up trying to allow to edit settings of locked fields.
Comment #158
catchThanks for the links to previous discussion, my concern is a bit different though:
At the moment, media is marked experimental, obviously we're trying to make it stable before 8.4.0 is released (otherwise we'll probably have to remove it from the 8.4.x branch and immediately put it back into 8.5.x to try again).
To do that, we need to keep media self-contained, putting the config into standard (even though it's optional) undermines that self-containment.
So even though that feels like the right place for it to live eventually, either we should open a follow-up to add the configuration, or add it to media for now with a follow-up to move it to standard when media's ready to go stable. That's one extra issue/patch, but worth it to keep things in one place for me.
Discussed this with @xjm and she had a similar opinion.
Comment #159
vijaycs85+1 to #158 Here it is.
Comment #161
naveenvalecha#158 Follow-up issue #2883815: Move Media Entity configuration from media module to standard profile
Comment #162
vijaycs85looks like the PNG file in the patch changed after I applied locally, although not showing as diff :(
Comment #163
phenaproximaRe #161: Thanks, @naveenvalecha! Added that issue to the roadmap at #2825215-71: Media initiative: Roadmap.
Comment #164
phenaproximaNever mind -- swapped in @vijaycs85's more complete #2883813: Move File/Image media type into Standard once Media is stable instead.
Comment #165
vijaycs85Same patch in #159 with
--binary
argument when generating the patch (phenaproxima++). diff file in #159 is valid of course.Comment #167
andypostin #156 I mean to add
.odf
open document format support because file field defines onlt.txt
extensionComment #168
vijaycs85Comment #169
phenaproximaOkay. This should fix the broken tests.
It turns out that the reason they were failing is because they were pretty much written on the assumption that the default configuration would be part of the profile, not the module. Which is a totally reasonable assumption, and one that will be true once we land #2883813: Move File/Image media type into Standard once Media is stable, as we intend to do. Therefore, I decided to take the approach of deleting the default config in setUp(). This might look dubious at first glance, but it clears the way for the tests to pass as written, and will allow them to continue to pass once the config is moved into the profile. All we'll need to do is remove those lines from setUp(), and I added the requisite TODO comments so that we'll know that.
I also had to do a little grooming on the default configuration in order to get DefaultConfigTest to pass, which annoyed the hell out of me, but whatever -- I did what I had to do!
Comment #170
tstoecklerThe
source_field
configuration value should be added to the schema.Comment #171
phenaproximaRe #170: It already is defined as part of media.source.field_aware, also in media.schema.yml.
Comment #172
tstoecklerOh, wow, that's a really sweet solution! Sorry for the noise and thanks for the quick response.
Comment #174
phenaproximaOkay. This should fix the problem with InstallUninstallTest.
It turns out that Media itself did not break InstallUninstallTest, but it did expose a pre-existing condition in the Field module that was manifested under the conditions produced by InstallUninstallTest.
So the changes I've introduced may seem dubious, but believe me...they were necessary in order to get the test to pass. Please see #2884202: Non-persistent fields become unpurgeable zombies without their target entity type for the details of the bug I'm working around in this patch. I'd rather not block this issue on that one, seeing as how I was able to find a workaround and Media is on a deadline. But, I defer to the committers' collective decision.
Comment #175
seanBNice find phenaproxima. Then I guess we are done here! Back to RTBC.
Comment #176
Wim Leers#150: RE: upgrade path: yay! Updated that issue's IS with your info.
#153: RE: change record: yay! Fixed some nits.
#154: RE: test coverage for
new_revision
change: if we already have test coverage, then why was #141 able to change this setting, without needing to update any test expectations?#158: Hah! That makes sense :)
#174:
Eh?! Why this change? Won't this make it impossible to use the
File
media source?Or are you doing this because
media
still depends onimage
, which itself depends onfile
, and therefore this is a no-op change in practice? But if that's the case, then why do it?(I went through the trouble of manually testing this, so manually uninstalling all the things that caused the
file
module to be required. Apparently even theupdate
module requires thefile
module, for no reason AFAICT!)This is my only remaining question for now.
This is weird, but it's to work around a core bug for which there is an issue — this is fine.
I also repeated the manual UI testing that @Gábor Hojtsy did in #129.
/admin/content
now indeed appears.I additionally noticed that the "Media" view is inconsistent with the "Content" and "Files" views:
<thead>
that those other views do show.Generally, we should make the "Media" view as consistent with the "Content" view as possible, since that's the canonical/most used one.
We should fix this, but that could be done in a follow-up.
And Gábor also had two remarks about the issue itself:
Comment #177
andypostCode looks good except default set of supported extensions
It needs follow-up to discuss the set to add 'odf' and cover with test this defaults
Comment #178
phenaproximaI was trying to figure out what the hell was breaking InstallUninstallTest. I had a theory that the dependency on File was causing it, so I removed it from the info file. It turned out not to be at fault, but I decided to leave the change in place because as you pointed out, Image depends on File, and Media depends on Image (in order to provide the thumbnail base field). So there is no reason for Media to depend explicitly on File.
I will post the requested follow-ups and re-roll the patch with --binary to fix the icon. Can we please, please move to SVG icons (follow-up!) as a matter of course so that we never have to deal with this BS ever again?
Comment #179
phenaproximaAdding #2884379: Improve the administrative Media view as a related issue, to address @Wim Leers' points in #176 about the Media view being inconsistent with other administrative content views.
Comment #180
phenaproximaAdding #2884383: File media source should accept Open Document format(s) as a related issue.
Comment #181
Wim Leers#177 + #180: added #2884383 to IS.
#178: But the
File
@MediaSource
plugin does explicitly depend on thefile
module. So I think it'd be good formedia
to continue to declare a dependency onfile
. In any case, it feels out of scope to be changing that in this issue. It's eyebrow-raising, and minimizing committer-eyebrow-raising is always a good thing to do :)#179: added #2884379 to IS.
RE: #154: test coverage, discussed that during today's Media meeting in IRC. Core committer @xjm agreed with my explanation/justification in #176: that changing that config should have caused some test to fail. This is not about testing the UI change.
I was expecting something along the lines of:
Because it makes a world of difference whether revisioning is enabled or disabled by default for an entity type or bundle! It affects all media entities of that bundle that will be created! Hence the request for this to be tested, so that we can certain that updating them indeed creates new revisions.
Comment #182
phenaproximaOkay. This patch adds the test coverage requested by the final point in #142 and detailed in #181. I also re-added an explicit dependency on File, per #181.
For the record, this patch was rolled with --binary, so the PNG should be intact.
Comment #183
Wim LeersNot a media type, we want to test the
file
media type that the default config is creating. i.e. test coverage for the default configuration.Comment #184
phenaproximaHow's this?
Comment #185
Wim LeersI think ideally this would be explicitly called
MediaDefaultConfigTest
or something like that.To ensure our expectations/assumptions wrt
media
module's default configuration are actually verifiably true. Of course we don't need to test everything, but this is a good example of something that's worth the burden of test coverage.Comment #186
Wim LeersClarified things I said in #176 (for which a follow-up was created) with screenshots: #2884379-3: Improve the administrative Media view.
Comment #188
catchI don't mind that as a workaround, field purging is painful and the other issue looks potentially critical.
Committed/pushed to 8.4.x, thanks!
Comment #189
Wim LeersYay, this means #2831937: Add "Image" MediaSource plugin is now unblocked! As well as all the follow-ups listed in the IS, of course:
Comment #190
Wim LeersBut note that #2884202: Non-persistent fields become unpurgeable zombies without their target entity type is a follow-up that is not really a Media initiative follow-up. It's a bug surfaced by the Media initiative, for which fortunately a decent work-around is available. See #188.
IS updated.
Comment #191
Wim LeersI:
#806102: Locked fields can change the widget but not settings is apparently also an issue that long predates the Media initiative but might be a blocker for the Media module to become stable.
So, I've done as much "push follow-ups forward" as possible.
Comment #192
phenaproximaRe-tagging.