[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:

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.

CommentFileSizeAuthor
#95 file_entity-catch_all_type-1292382-95.patch455 bytesazinck
#91 1292382-file_entity_custom_types-91.patch50.8 KBParisLiakos
#82 file_entity-1292382-82.patch50.98 KBtim.plunkett
#82 interdiff.txt5.61 KBtim.plunkett
#78 file_entity-1292382-78.patch50.38 KBtim.plunkett
#77 1292382_custom_file_types_77.patch50.39 KBaaron
#73 1292382_custom_file_types_73.patch49.26 KBslashrsm
#72 1292382_custom_file_types_71.patch48.24 KBslashrsm
#69 1292382_custom_file_types_69.patch48.25 KBslashrsm
#62 ctools-export-file-types-1292382-62.patch46.17 KBaaron
#60 ctools-export-file-types-1292382-60.patch46.17 KBaaron
#59 ctools-export-file-types-1292382-59.patch45.41 KBaaron
#58 ctools-export-file-types-1292382-58.patch44.01 KBaaron
#55 ctools-export-file-types-1292382-55.patch43.97 KBaaron
#53 ctools-export-file-types-1292382-53.patch41.03 KBaaron
#50 ctools-export-file-types-1292382-50.patch40.32 KBaaron
#45 ctools-export-file-types-1292382-45.patch39.72 KBaaron
#42 ctools-export-file-types-1292382-42.patch39.85 KBaaron
#41 ctools-export-file-types-1292382-41.patch39.75 KBaaron
#38 ctools-export-file-types-1292382-38.patch39.81 KBelliotttf
#36 ctools-export-file-types-1292382-36.patch39.21 KBaaron
#35 ctools-export-file-types-1292382-35.patch39.21 KBaaron
#30 1 - file type mimetype config ui.png144.56 KBbecw
#28 ctools-export-file-types-1292382-28.patch39 KBeffulgentsia
#24 ctools-export-file-types-1292382-24.patch38.4 KBeffulgentsia
#23 ctools-export-file-types-1292382-23.patch33.41 KBeffulgentsia
#20 ctools-export-file-types-1292382-20.patch26.79 KBrobeano
#19 ctools-export-file-types-1292382-19.patch26.82 KBaaron
#17 ctools-export-file-types-1292382-16.patch27.49 KBaaron
#15 ctools-export-file-types-1292382-15.patch28.41 KBaaron
#14 ctools-export-file-types-1292382-14.patch1.44 KBaaron
#13 ctools-export-file-types-1292382-13.patch1.44 KBaaron
#12 ctools-export-file-types-1292382-12.patch27.89 KBaaron
#11 ctools-export-file-types-1292382-11.patch27.95 KBaaron
#10 ctools-export-file-types-1292382-10.patch24.62 KBaaron
#9 file_type_ctools.patch24.11 KBJacobSingh
#8 create-file-type-table-1292382-6.patch27.54 KBaaron
#5 create-file-type-table-1292382-5.patch18.55 KBaaron
#3 create-file-type-table-1292382-3.patch4.5 KBaaron
#2 create-file-type-table-1292382-2.patch2.89 KBaaron
#1 create-file-type-table-1292382-1.patch2.89 KBaaron
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaron’s picture

Title: Better implementation of file_types » Create file types table
Status: Active » Needs review
FileSize
2.89 KB

this creates the file type & file type mimetypes mapping tables. required before we can create/implement the file type mappings.

aaron’s picture

use type for field name for consistency in file_type_mimetypes

aaron’s picture

Title: Create file types table » Create file types table and file_type_save()
FileSize
4.5 KB

fixes the indices for file_type_mimetypes and adds file_type_mimetypes().

aaron’s picture

Assigned: Unassigned » aaron
aaron’s picture

JacobSingh’s picture

Assigned: aaron » JacobSingh

I'll take a look.

Dave Reid’s picture

I 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.

aaron’s picture

JacobSingh’s picture

Assigned: aaron » JacobSingh
Status: Needs review » Needs work
FileSize
24.11 KB

WIP patch with ctools

aaron’s picture

wip

aaron’s picture

Status: Needs work » Needs review
FileSize
27.95 KB
aaron’s picture

aaron’s picture

aaron’s picture

aaron’s picture

Status: Needs review » Needs work

The last submitted patch, ctools-export-file-types-1292382-15.patch, failed testing.

aaron’s picture

Status: Needs work » Needs review
FileSize
27.49 KB
aaron’s picture

status change

aaron’s picture

robeano’s picture

Poor @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.

robeano’s picture

I ran drush updatedb and did not see any errors.

When I loaded the home page after the update, several PHP warnings were displayed:

    Warning: Attempt to assign property of non-object in _ctools_export_get_defaults() (line 638 of /Applications/MAMP/htdocs/media/sites/default/modules/ctools/includes/export.inc).
    Warning: Attempt to assign property of non-object in _ctools_export_get_defaults() (line 638 of /Applications/MAMP/htdocs/media/sites/default/modules/ctools/includes/export.inc).
    Warning: Attempt to assign property of non-object in _ctools_export_get_defaults() (line 638 of /Applications/MAMP/htdocs/media/sites/default/modules/ctools/includes/export.inc).
    Warning: Attempt to assign property of non-object in _ctools_export_get_defaults() (line 638 of /Applications/MAMP/htdocs/media/sites/default/modules/ctools/includes/export.inc).

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.

effulgentsia’s picture

Assigned: aaron » effulgentsia
Status: Needs review » Needs work

The CTools integration can use some cleanup. Working on it.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
33.41 KB

Here's an updated patch. I'm not sure if it's complete yet, but posting the work in progress.

effulgentsia’s picture

Priority: Critical » Major
FileSize
38.4 KB

Ok, 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!

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Priority: Major » Critical

Upping 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.

Priority: Major » Critical
Status: Needs review » Needs work

The last submitted patch, ctools-export-file-types-1292382-24.patch, failed testing.

Dave Reid’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

File 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.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
39 KB

The 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.

becw’s picture

Assigned: Unassigned » becw

Reviewing.

becw’s picture

Status: Needs review » Needs work
FileSize
144.56 KB
  1. The UI for managing file type mime types is hard to use (see attached) -- it provides a free-text entry field, and lists 300+ allowed values below. Preferably this would only allow users to select available mime types instead of allowing users to enter values that will throw an error. However, making a 300+ item list of checkboxes or multiselect usable is hard, so I'm ok with the patch as it is as long as there's a followup issue where we can solicit advice on how this UI should work.
  2. When viewing file previews with this patch, preview thumbnails don't appear for non-image files, and I get this PHP notice:
    Notice: Undefined index: #file in theme_media_thumbnail() (line 245 of /Library/WebServer/Documents/media/sites/default/modules/media/includes/media.theme.inc).
  3. Even though file types are CTools exportables, we should still document the return values that we expect from hook_file_default_types() in file_entity.api.php
  4. With the removal of hook_file_type_info() (line 171 and others), how does file_entity match up file types with their default view callbacks? (related to hook_file_type_TYPE_default_view()?) It looks like the default formatter fallback has been removed from file_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.
  5. line 293: this patch adds code related to file_display exportables--it's not related to this patch, but these are minor changes and might as well stay where they are.
  6. line 425: there's logic around figuring out whether this file type is being renamed. The UI doesn't allow changing the machine names of file types. Should the 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.
  7. line 457: use db_merge() instead of wrapping update/insert in an if statement, and always wipe and reinsert the file type's mimetypes.
  8. Are there constraints on using the same mimetype on multiple file types? -- nope: the when figuring out a file type based on a mimetype, we just use the first file type we find that uses the mimetype.
  9. line 576: does this mean that when media is installed, any existing files will get the default 'undefined' file type?
  10. line 605, 618: I think that even successful update functions should return something saying what they've done.
  11. line 751: the comment says that administrators can add custom file types via admin/config/media/file-types, but there's UI for this. I suggest just changing the comment :)
  12. line 1076: yay, minuses! It looks like there were no tests using this code anyways.

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?

becw’s picture

p.s. this is a huge patch, both in functionality and in lines, and I'm really excited to see it this far along!

aaron’s picture

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.

oEmbed 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.

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?

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.

aaron’s picture

The UI for managing file type mime types is hard to use (see attached) -- it provides a free-text entry field, and lists 300+ allowed values below. Preferably this would only allow users to select available mime types instead of allowing users to enter values that will throw an error. However, making a 300+ item list of checkboxes or multiselect usable is hard, so I'm ok with the patch as it is as long as there's a followup issue where we can solicit advice on how this UI should work.

Agreed.

aaron’s picture

When viewing file previews with this patch, preview thumbnails don't appear for non-image files, and I get this PHP notice:

Notice: Undefined index: #file in theme_media_thumbnail() (line 245 of /Library/WebServer/Documents/media/sites/default/modules/media/includes/media.theme.inc).

I believe that needs to be a follow-up patch for media.

Even though file types are CTools exportables, we should still document the return values that we expect from hook_file_default_types() in file_entity.api.php

Okay.

With the removal of hook_file_type_info() (line 171 and others), how does file_entity match up file types with their default view callbacks? (related to hook_file_type_TYPE_default_view()?) It looks like the default formatter fallback has been removed from file_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.

Perhaps we should always fall back to a display of a link to the file?

line 293: this patch adds code related to file_display exportables--it's not related to this patch, but these are minor changes and might as well stay where they are.

Okay.

line 425: there's logic around figuring out whether this file type is being renamed. The UI doesn't allow changing the machine names of file types. Should the 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.

I agree, we should never allow the file type to be renamed. However, will this break ctools logic?

line 457: use db_merge() instead of wrapping update/insert in an if statement, and always wipe and reinsert the file type's mimetypes.

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.

Are there constraints on using the same mimetype on multiple file types? -- nope: the when figuring out a file type based on a mimetype, we just use the first file type we find that uses the mimetype.

Okay.

line 576: does this mean that when media is installed, any existing files will get the default 'undefined' file type?

Currently, yes.

line 605, 618: I think that even successful update functions should return something saying what they've done.

Why? We already say what they're going to do in the comments, which get printed before the update is run.

line 751: the comment says that administrators can add custom file types via admin/config/media/file-types, but there's UI for this. I suggest just changing the comment :)

Huh?

line 1076: yay, minuses! It looks like there were no tests using this code anyways.

Okay.

aaron’s picture

Status: Needs work » Needs review
FileSize
39.21 KB

This 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... .

aaron’s picture

aaron’s picture

what's with the test bot?

elliotttf’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, ctools-export-file-types-1292382-38.patch, failed testing.

Dave Reid’s picture

aaron’s picture

I have made some progress on this patch. Here is the latest; it doesn't apply yet, as I need to finish it.

aaron’s picture

Status: Needs work » Needs review
FileSize
39.85 KB

whee!

Status: Needs review » Needs work

The last submitted patch, ctools-export-file-types-1292382-42.patch, failed testing.

tsvenson’s picture

This is very important so going to make this the #1 patch I will help working on (testing mainly) during the sprint.

aaron’s picture

Status: Needs work » Needs review
FileSize
39.72 KB

here we go again

tsvenson’s picture

Applied 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.

aaron’s picture

tsvenson: did you run the updates?

paulmckibben’s picture

The 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!

tsvenson’s picture

OK, 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';

aaron’s picture

Thanks, got that. I also added the file type to file listing the administration page, rather than using the mime type.

tsvenson’s picture

That 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.

tsvenson’s picture

Status: Needs review » Reviewed & tested by the community

When 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.

aaron’s picture

here it is with the bread crumbs in place.

tsvenson’s picture

aaron’s picture

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

I had forgotten entirely about including stream wrappers before. This patch includes that now.

tsvenson’s picture

Status: Needs review » Reviewed & tested by the community

Patch 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, ctools-export-file-types-1292382-55.patch, failed testing.

aaron’s picture

Assigned: becw » aaron
Status: Needs work » Needs review
FileSize
44.01 KB

chasing head

aaron’s picture

New and improved! Now with tests!

aaron’s picture

This is also moving over some tests from the media module, although I commented out one of them.

Status: Needs review » Needs work

The last submitted patch, ctools-export-file-types-1292382-60.patch, failed testing.

aaron’s picture

Status: Needs work » Needs review
FileSize
46.17 KB

Sorry about that, this one should work

Nick Lewis’s picture

Nick Lewis’s picture

My attempt to update resulted in the same failures, but my media install is all jacked up.

tsvenson’s picture

I'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.

tsvenson’s picture

Issue summary: View changes

Updated issue summary.

aaron’s picture

http://drupal.org/node/1506424 is what we need to get media working with this patch, so that we can get this sucker in.

Dave Reid’s picture

+++ b/file_entity.installundefined
@@ -9,6 +9,10 @@
+  $schema['file_type'] = _file_entity_schema_file_type_7201();
+  $schema['file_type_mimetypes'] = _file_entity_schema_file_type_mimetypes_7201();
+  $schema['file_type_streams'] = _file_entity_schema_file_type_streams_7201();

Let's follow core best practices and put the raw schema array in here and not have a helper function for it.

+++ b/file_entity.installundefined
@@ -140,12 +148,6 @@ function file_entity_install() {
-  // Update all files with empty types to use the first part of filemime.
-  db_update('file_managed')
-    ->expression('type', "SUBSTRING_INDEX(filemime, '/', 1)")
-    ->condition('type', '')
-    ->execute();
-
   // Set permissions.

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?

+++ b/file_entity.installundefined
@@ -317,3 +323,151 @@ function file_entity_update_7200() {
+function file_entity_update_7201() {
+  db_create_table('file_type', _file_entity_schema_file_type_7201());
+  db_create_table('file_type_mimetypes',  _file_entity_schema_file_type_mimetypes_7201());
+  db_create_table('file_type_streams',  _file_entity_schema_file_type_streams_7201());

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.

+++ b/tests/file_entity.testundefined
@@ -23,6 +23,22 @@ class FileEntityTestHelper extends DrupalWebTestCase {
+  protected function createType($overrides = array()) {

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.

tsvenson’s picture

Title: Create file types table and file_type_save() » Make it possible to create any number of custom File Types

Changed title to better reflect what this patch is about.

slashrsm’s picture

Attached 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?

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?

We can use file_type_determine(), which is now implemented, to determine types for existing files. Would that work?

mauritsl’s picture

mauritsl’s picture

Added some discussion points on file_type_determine() and hook_file_type() here:

http://drupal.org/node/1637382#comment-6122396

slashrsm’s picture

file_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.

slashrsm’s picture

Added API docs for new hooks.

gmclelland’s picture

Issue tags: -sprint, -Media Initiative

#73: 1292382_custom_file_types_73.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +sprint, +Media Initiative

The last submitted patch, 1292382_custom_file_types_73.patch, failed testing.

gmclelland’s picture

I think #73 needs a reroll for the latest dev version.

aaron’s picture

Status: Needs work » Needs review
FileSize
50.39 KB

it was one of the tests that needed to be changed

tim.plunkett’s picture

FileSize
50.38 KB

Rerolled with some coding style fixes.
Hopefully I'll have some time to fully test this tomorrow.

grendzy’s picture

This 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).

aaron’s picture

Status: Needs work » Needs review

grendzy, 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.

ryan.gibson’s picture

Status: Needs review » Needs work

Tested 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!

tim.plunkett’s picture

Addressed the concerns in #79 and #81.

ryan.gibson’s picture

Status: Needs review » Reviewed & tested by the community

Alright this looks good to me, thanks for making those changes tim.plunkett. As I noted in 81, the functionality works great. Marking rtbc.

Jackinloadup’s picture

Installed the patch on a fresh install and didn't get any errors while playing around. Though I didn't create any custom types.

Nitebreed’s picture

Works fine, haven't created custom filetypes but added mimetypes to existing ones and they work great!

michielnugter’s picture

Works great, +1 for including this patch in the next release!

aaron’s picture

#82: file_entity-1292382-82.patch queued for re-testing.

jojonaloha’s picture

Looks 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!

ParisLiakos’s picture

Issue tags: -sprint, -Media Initiative

#82: file_entity-1292382-82.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Media Initiative

The last submitted patch, file_entity-1292382-82.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
50.8 KB
ParisLiakos’s picture

Ok, 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 :)

ParisLiakos’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

azinck’s picture

Status: Closed (fixed) » Active
FileSize
455 bytes

This 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.

ParisLiakos’s picture

Status: Active » Closed (fixed)

To 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

chirhotec’s picture

Is 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.

slashrsm’s picture

UI is almost ready (just needs tests). Check #1260050: Provide administrative UI for adding/removing file types.

chirhotec’s picture

Ok, 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?)

azinck’s picture

Yes, there are only 4 default types in the latest 2.x versions of File Entity.

azinck’s picture

Issue summary: View changes

Updated issue summary.