Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
[ToDo: Write a better issue summary]
This patch will make it possible to create custom file types that even contains mixed files (for example images, videos and auto).
It will then make it possible to create file types of the type "Logotypes" that has fields tailored for the needs of logos, while another is called "Products" and contains images and videos of products with a totally different set of fields.
Issues depending on this one:
- #1260050: Provide administrative UI for adding/removing file types
- #1553094: Alt and Title support for Images
Original issue: currently, media_type_get_types() does most of the heavy lifting for file types. this needs to be moved over to file_entity and removed from media.
Comment | File | Size | Author |
---|---|---|---|
#95 | file_entity-catch_all_type-1292382-95.patch | 455 bytes | azinck |
#91 | 1292382-file_entity_custom_types-91.patch | 50.8 KB | ParisLiakos |
#82 | file_entity-1292382-82.patch | 50.98 KB | tim.plunkett |
#82 | interdiff.txt | 5.61 KB | tim.plunkett |
#78 | file_entity-1292382-78.patch | 50.38 KB | tim.plunkett |
Comments
Comment #1
aaron CreditAttribution: aaron commentedthis creates the file type & file type mimetypes mapping tables. required before we can create/implement the file type mappings.
Comment #2
aaron CreditAttribution: aaron commenteduse type for field name for consistency in file_type_mimetypes
Comment #3
aaron CreditAttribution: aaron commentedfixes the indices for file_type_mimetypes and adds file_type_mimetypes().
Comment #4
aaron CreditAttribution: aaron commentedComment #5
aaron CreditAttribution: aaron commentedComment #6
JacobSingh CreditAttribution: JacobSingh commentedI'll take a look.
Comment #7
Dave ReidI think overall this begs the question if we should make file types exportable with CTools. I think we end up having spaghetti code with some properties I don't agree are good for content types:
'custom' => 0,
'modified' => 0,
'locked' => 1,
'disabled' => 0,
'is_new' => 1,
Having a hook for file types and then CRUD for file types just kinda screams CTools exportables.
Comment #8
aaron CreditAttribution: aaron commentedComment #9
JacobSingh CreditAttribution: JacobSingh commentedWIP patch with ctools
Comment #10
aaron CreditAttribution: aaron commentedwip
Comment #11
aaron CreditAttribution: aaron commentedComment #12
aaron CreditAttribution: aaron commentedComment #13
aaron CreditAttribution: aaron commentedComment #14
aaron CreditAttribution: aaron commentedComment #15
aaron CreditAttribution: aaron commentedComment #17
aaron CreditAttribution: aaron commentedComment #18
aaron CreditAttribution: aaron commentedstatus change
Comment #19
aaron CreditAttribution: aaron commentedComment #20
robeano CreditAttribution: robeano commentedPoor @aaron! @davereid kept committing changes to file_entity. This latest patch should work with 7.x-1.x and needs more testing.
[edit]
Please run update.php after applying the patch.
Comment #21
robeano CreditAttribution: robeano commentedI ran drush updatedb and did not see any errors.
When I loaded the home page after the update, several PHP warnings were displayed:
I attempted to go to Admin -> Configuration -> File Types -> Edit File Type (for Images types) or admin/config/media/file-types/manage/image/edit but I get an access denied error as an administrator. As I kept debugging this issue, eventually the Access Denied went away as well as the Warnings above. There's more to test here, but I need a break.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedThe CTools integration can use some cleanup. Working on it.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedHere's an updated patch. I'm not sure if it's complete yet, but posting the work in progress.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedOk, here's one that I think is ready to be committed (but someone needs to review it and set it to RTBC).
As a summary, this patch gets us part of the way towards what's outlined in #1260050-3: Provide administrative UI for adding/removing file types. It doesn't yet provide a UI for enabling/disabling module-defined default types, or for reverting an admin-defined type to its module-defined default settings, or for adding/removing new admin-defined types that aren't defined by any module. All those fun CTools exportables capabilities will be needed, but I think should be done as follow-ups, so as not to make this patch any bigger. This patch does, however, provide a UI for the administrator to customize the settings of the default image, audio, video, and document types.
Also, this patch doesn't add a UI for adding files from admin/content/files (the Media project does, but arguably, a simple implementation of that belongs in File Entity). When such a UI is added, it will need to include a confirmation step if someone is adding a file whose mimetype is supported by more than one file type, so the administrator can choose which file type to assign to the new file. But again, that's a separate task that doesn't need to hold up this patch. In the meantime, you can test the logic for how file types get assigned by uploading files via file or image fields on nodes.
This patch also doesn't implement the media widget simplification shown in #1260050-3: Provide administrative UI for adding/removing file types. That will be a job for the Media project once this patch is committed to File Entity.
This patch does, however, include a forward port of #1268378-28: Empty file type causes exceptions in Drupal 7.8, which wasn't initially needed in the File Entity project when file types could be absolutely determined based on a fixed and complete mimetype mapping. However, now that the mimetype mapping can be customized, that forward port was needed.
Anyway, let's get this committed quickly, so we can continue with the above follow-ups. Thanks!
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedUpping to critical, since this is a necessary issue to solve before doing the rest of #1260050: Provide administrative UI for adding/removing file types, which is a critical issue.
Comment #27
Dave ReidFile entity has switched to a 7.x-2.x branch and the 7.x-1.x branch is no longer used. Please make sure to update your Git clones.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedThe token tests that failed in #24 are due to them expecting a file type of 'text', which this issue intentionally changes to 'document', so this patch adjusts those tests.
Comment #29
becw CreditAttribution: becw commentedReviewing.
Comment #30
becw CreditAttribution: becw commentedhook_file_default_types()
in file_entity.api.phphook_file_type_info()
(line 171 and others), how does file_entity match up file types with their default view callbacks? (related tohook_file_type_TYPE_default_view()
?) It looks like the default formatter fallback has been removed fromfile_view_file()
. This makes sense, because it removes the potential for a default formatter that is not available in the UI, but it means that a file might appear if there is no formatter configured for it, which is potentially baffling.file_display
exportables--it's not related to this patch, but these are minor changes and might as well stay where they are.file_type_save()
allow this? I've seen this pattern before, but it strikes me that the machine name is used in many places, and it's not practical to change it in all those places when a file type is updated. This means that any "rename" operation is essentially a delete + create new; perhaps we should force anyone trying to do this to to it explicitly.One major issue with this new mimetype handling is that existing media with mimetypes outside of the core list break in the media browser widget. For sites using oEmbed, this will be very common--oEmbed uses custom mimetypes like "video/oembed". We should make sure that we respect the file types on existing files instead of expecting we can always extract it from the mimetype--first of all, the file type assigned to a mimetype may change over the lifetime of a site, and second, this update should not change anything for existing content.
Will there be a way for oembed to automatically declare what file type its media sources are? Or will it need to declare the mime types, and then alter them into the default file type configurations? How will this affect default file type configurations vs. file type configurations in the database?
Comment #31
becw CreditAttribution: becw commentedp.s. this is a huge patch, both in functionality and in lines, and I'm really excited to see it this far along!
Comment #32
aaron CreditAttribution: aaron commentedoEmbed should use hook_file_mimetype_mapping_alter. I think it's reasonable to expect modules to follow Drupal's standards. Additionally, we don't expect to be able to extract the file type from a mimetype. For instance, we may have several file types mapping to image/jpeg mime types.
Yes, oEmbed should be able to automatically declare a filetype. And as I pointed out above, it will also need to declare the mime types.
Comment #33
aaron CreditAttribution: aaron commentedAgreed.
Comment #34
aaron CreditAttribution: aaron commentedI believe that needs to be a follow-up patch for media.
Okay.
Perhaps we should always fall back to a display of a link to the file?
Okay.
I agree, we should never allow the file type to be renamed. However, will this break ctools logic?
I agree in principle, however, the ctools module expects SAVE_INSERT or SAVE_UPDATE to be returned. I'm not sure what will happen if we change that.
Okay.
Currently, yes.
Why? We already say what they're going to do in the comments, which get printed before the update is run.
Huh?
Okay.
Comment #35
aaron CreditAttribution: aaron commentedThis patch documents the required return values for hook_file_default_types() and also changes the update function numbers to 720 and 7201, per http://api.drupal.org/api/drupal/modules--system--system.api.php/functio... .
Comment #36
aaron CreditAttribution: aaron commentedOops.
Comment #37
aaron CreditAttribution: aaron commentedwhat's with the test bot?
Comment #38
elliotttf CreditAttribution: elliotttf commentedI've re-rolled the patch from #36 against the latest 7.x-2.x branch. I made a couple of minor changes in the tests, but otherwise it should be the same thing.
Comment #40
Dave ReidComment #41
aaron CreditAttribution: aaron commentedI have made some progress on this patch. Here is the latest; it doesn't apply yet, as I need to finish it.
Comment #42
aaron CreditAttribution: aaron commentedwhee!
Comment #44
tsvenson CreditAttribution: tsvenson commentedThis is very important so going to make this the #1 patch I will help working on (testing mainly) during the sprint.
Comment #45
aaron CreditAttribution: aaron commentedhere we go again
Comment #46
tsvenson CreditAttribution: tsvenson commentedApplied the patch in #45, cleared the cache and got the following warning:
Warning: Invalid argument supplied for foreach() in file_entity_field_extra_fields() (line 389 of /home/thomas/www/media-2-dev/sites/all/modules/file_entity/file_entity.module).
Continuing to test now.
Comment #47
aaron CreditAttribution: aaron commentedtsvenson: did you run the updates?
Comment #48
paulmckibbenThe patch applies cleanly for me.
One minor issue: as user 1, after updating the database, I got "access denied" at admin/structure/file-types/manage/image/edit
However, that problem went away after I cleared all my caches.
I was able to edit one of the file types (I added image/tiff to the image type), and nothing else appeared to be broken. I am in favor of committing the patch.
Thanks for the good work!
Comment #49
tsvenson CreditAttribution: tsvenson commentedOK, patch applies cleanly and after dbupdate presents a new UI making it easy to edit file types.
I edited the image fily type added a new mime type, that worked. But when saving I got redirected to admin/config/media/file-types and not admin/structure/file-types which is the new home.
I believe
$form_state['redirect'] = 'admin/config/media/file-types';
needs to be changed to$form_state['redirect'] = 'admin/structure/file-types';
Comment #50
aaron CreditAttribution: aaron commentedThanks, got that. I also added the file type to file listing the administration page, rather than using the mime type.
Comment #51
tsvenson CreditAttribution: tsvenson commentedThat did the trick.
Would it be possible to add the file type name to the breadcrumbs too? When on the Manage tabs there is no way to tell for which file type I am on.
Comment #52
tsvenson CreditAttribution: tsvenson commentedWhen file type name breadcrumb is added I say this patch, together with #1496624: Provide an upgrade path for media types to file_entity types, is RTBC.
Comment #53
aaron CreditAttribution: aaron commentedhere it is with the bread crumbs in place.
Comment #54
tsvenson CreditAttribution: tsvenson commentedWorks great.
This, together with #1496624: Provide an upgrade path for media types to file_entity types is RTBC now.
Comment #55
aaron CreditAttribution: aaron commentedI had forgotten entirely about including stream wrappers before. This patch includes that now.
Comment #56
tsvenson CreditAttribution: tsvenson commentedPatch in #55 doesn't apply:
error: patch failed: file_entity.install:247
error: file_entity.install: patch does not apply
That after making git pull to get up-to-date with repo.
Comment #58
aaron CreditAttribution: aaron commentedchasing head
Comment #59
aaron CreditAttribution: aaron commentedNew and improved! Now with tests!
Comment #60
aaron CreditAttribution: aaron commentedThis is also moving over some tests from the media module, although I commented out one of them.
Comment #62
aaron CreditAttribution: aaron commentedSorry about that, this one should work
Comment #63
Nick Lewis CreditAttribution: Nick Lewis commented#62: ctools-export-file-types-1292382-62.patch queued for re-testing.
Comment #64
Nick Lewis CreditAttribution: Nick Lewis commentedMy attempt to update resulted in the same failures, but my media install is all jacked up.
Comment #65
tsvenson CreditAttribution: tsvenson commentedI've been thinking about how to best tackle the issue with that there is currently no default viewing of images using the "file/[fid]" path out-of-the-box. In my opinion this is not so good UX as most users will expect something to be shown as default. Now its easy to instantly think the installation is broken as nothing is shown.
Since the core image-module comes with a couple of styles, we can easily use for example the "large" (48+px wide) style as default. All we need to do is to enable Image and Image Style: Large for Default in the Manage File Display tab for the file type image.
Comment #65.0
tsvenson CreditAttribution: tsvenson commentedUpdated issue summary.
Comment #66
aaron CreditAttribution: aaron commentedhttp://drupal.org/node/1506424 is what we need to get media working with this patch, so that we can get this sucker in.
Comment #67
Dave ReidLet's follow core best practices and put the raw schema array in here and not have a helper function for it.
Since we're removing the 'apply default file type on install to existing files' functionality, how does this happpen now? All existing files get assigned 'undefined' with no way to fix it?
Same here. Get rid of the helper functions. It's much easier to follow in the code if the schema arrays are actually in the update functions. Yes, it's "more" code but it's easier to follow.
Should probably be createFileType to better self-document.
As an overall comment I'm hesitant to add too many assumptions that file types are exportable with CTools. I don't believe that node types or vocabularies are planned to be exportable with CMI in D7, so it would be good if the exportable support was either removed for now, or was made optional, otherwise I feel that we're going to have to go backwards to rip it out when we port the module to merge with Drupal 8.
Comment #68
tsvenson CreditAttribution: tsvenson commentedChanged title to better reflect what this patch is about.
Comment #69
slashrsm CreditAttribution: slashrsm commentedAttached patch fixes some things mentioned in #67 (scheme helper functions, test function name).
I also added another hook, which is used when we try to determine which bundle should be assigned to a new file entity. It currently keeps the same behaviour as before, but it allows other modules to change this behaviour.
Regarding export feature I'm more into having this optional than removing it. People will most likely use this even in D7, so we should provide them ability to export their bundles. Should we move export-related code to another submodule and make our bundle implementation non ctools dependent by itself?
We can use file_type_determine(), which is now implemented, to determine types for existing files. Would that work?
Comment #70
mauritsl CreditAttribution: mauritsl commentedProbably a requirement:
#1637382: type of file is overwritten in file_entity_file_presave().
And issue depending on this:
#1553114: Adding files should be a multi-step process
Comment #71
mauritsl CreditAttribution: mauritsl commentedAdded some discussion points on file_type_determine() and hook_file_type() here:
http://drupal.org/node/1637382#comment-6122396
Comment #72
slashrsm CreditAttribution: slashrsm commentedfile_type_determine() is actually doing the work of file_get_type(), which was removed by this patch. New patch renames file_type_determine() to file_get_type(). If we compare new file_get_type() to old one it basically does the same thing, but it does it on a different way.
Comment #73
slashrsm CreditAttribution: slashrsm commentedAdded API docs for new hooks.
Comment #74
gmclelland CreditAttribution: gmclelland commented#73: 1292382_custom_file_types_73.patch queued for re-testing.
Comment #76
gmclelland CreditAttribution: gmclelland commentedI think #73 needs a reroll for the latest dev version.
Comment #77
aaron CreditAttribution: aaron commentedit was one of the tests that needed to be changed
Comment #78
tim.plunkettRerolled with some coding style fixes.
Hopefully I'll have some time to fully test this tomorrow.
Comment #79
grendzy CreditAttribution: grendzy commentedThis is my first media patch review, so please forgive me if some feedback is ill-informed.
Visiting node/1/edit produces:
Fatal error: Call to undefined function file_info_file_types() in drupal/sites/all/modules/media/media.module on line 1096
I don't see a way to disable default types (or is that planned for #1260050: Provide administrative UI for adding/removing file types ?)
Will Media have a way to explicitly choose the type when creating a file? When more than one than one MIME type matches it seems to pick the last one in the list.
Do users need this information in type descriptions?
Audio files are an electrical representation of sound.
It's a little... metaphysical? Instead, perhaps the default descriptions could summarize the included file types (such as MP3, WAVE, Ogg).
Comment #80
aaron CreditAttribution: aaron commentedgrendzy, you need the patch at #1496624: Provide an upgrade path for media types to file_entity types as well for the first error. EDIT: Rather, you should probably use #1506424: allow thumbnails to work after applying the file types patch file entity.
disabling types is planed elsewhen.
the media feature request (choose the type) will go in that queue. it will need some ux design.
the descriptions are from wikipedia, fpo. sorry. I like your idea.
Comment #81
ryan.gibson CreditAttribution: ryan.gibson commentedTested the patch in #78 and it worked great.
One request, could some documentation be added to explain how to use the hook? It took some searching through the patch and module to realize that I had to use hook_ctools_plugin_api() in order to get hook_file_default_types() in my custom module noticed. I think just some documentation to explain that hook_ctools_plugin_api() needs to be added or appended to if already existing would suffice. I'm not sure of the best wording or if it should be added to file_entity.api.php or elsewhere so I won't take a swing at that.
Thanks a bunch!
Comment #82
tim.plunkettAddressed the concerns in #79 and #81.
Comment #83
ryan.gibson CreditAttribution: ryan.gibson commentedAlright this looks good to me, thanks for making those changes tim.plunkett. As I noted in 81, the functionality works great. Marking rtbc.
Comment #84
Jackinloadup CreditAttribution: Jackinloadup commentedInstalled the patch on a fresh install and didn't get any errors while playing around. Though I didn't create any custom types.
Comment #85
NitebreedWorks fine, haven't created custom filetypes but added mimetypes to existing ones and they work great!
Comment #86
michielnugter CreditAttribution: michielnugter commentedWorks great, +1 for including this patch in the next release!
Comment #87
aaron CreditAttribution: aaron commented#82: file_entity-1292382-82.patch queued for re-testing.
Comment #88
jojonaloha CreditAttribution: jojonaloha at Metal Toad commentedLooks good to me. I created a custom module and implemented:
- hook_ctools_plugin_api()
- hook_file_default_types()
Everything from /admin/structure/file-types worked as expected for my new filetype. Also tested a hook_file_default_types_alter() implementation.
Very excited to get this in!
Comment #89
ParisLiakos CreditAttribution: ParisLiakos commented#82: file_entity-1292382-82.patch queued for re-testing.
Comment #91
ParisLiakos CreditAttribution: ParisLiakos commentedreroll after #1296268: Add Preview and Teaser view modes landed
Comment #92
ParisLiakos CreditAttribution: ParisLiakos commentedOk, this issue is one year old and i think it is time to commit, a lot of positive reviews, i did a quick check on gui, everything good and code looks good.
Lets get additional issues for cleanups, additions this patch is already 50kb!
good job and thanks everyone, commited :)
Comment #93
ParisLiakos CreditAttribution: ParisLiakos commentedComment #95
azinck CreditAttribution: azinck commentedThis change means there's no longer a "catch-all" file type for types that don't match recognized mimetypes. It used to be that such files were assigned type "application". Now they're undefined which prevents you from using the media widget to upload such files, even if it's an allowed extension.
Here's a sort of proof-of-concept patch to assign all such files to type "document". If the general approach is favored then perhaps the best solution would be a setting to determine which file type should serve as the catch-all.
Comment #96
ParisLiakos CreditAttribution: ParisLiakos commentedTo be honest i would rather introduce a new type...maybe application? than using document.Lets take this to a new issue though
#1920350: Provide a "catch all" default application file type
Comment #97
chirhotec CreditAttribution: chirhotec commentedIs there a UI for adding new file types? Or updated documentation on how to create the file types in code? The only documentation I can find that lists the file types suggest there are 6 (Application, Audio, Text, Image, Video, Other), and yet when I installed the latest version of the module there are only four: Audio, Document, Image & Video.
Comment #98
slashrsm CreditAttribution: slashrsm commentedUI is almost ready (just needs tests). Check #1260050: Provide administrative UI for adding/removing file types.
Comment #99
chirhotec CreditAttribution: chirhotec commentedOk, thanks, I'll check it out it and add my notes to that ticket.
But am I correct about the change from 6 to 4 default types? (or is that an artifact from migrating from an old 1.x version of Media / File Entity?)
Comment #100
azinck CreditAttribution: azinck commentedYes, there are only 4 default types in the latest 2.x versions of File Entity.
Comment #100.0
azinck CreditAttribution: azinck commentedUpdated issue summary.