Closed (fixed)
Project:
D7 Media
Version:
7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
14 Sep 2010 at 14:15 UTC
Updated:
30 Sep 2010 at 23:20 UTC
Jump to comment: Most recent file
Comments
Comment #1
JacobSingh commentedThis is actually kinda hairy.. :(
I'm looking at the code from http://drupal.org/files/issues/file_usage_19.patch in file.field.inc.
The media field's implementation is not nearly as robust w.r.t. revisions on entities. I'm wondering if the best approach is to switch the media field to being a widget of filefield. This presents its own issues of course.
I wonder if http://drupal.org/project/filefield_sources is a good solution. This of course is a radical departure of the media module architecture, but is a nice similar module. I guess my reservation here is that one of media's goals is to have the same interface everywhere, for WYSIWYG, for fields and for custom implementations...Anyway, I digress...
Comment #2
JacobSingh commentedComment #3
JacobSingh commentedfilefield sources doesn't have a D7 version though, and porting it would be a kinda crazy endeavor.
Comment #4
effulgentsia commentedThe "delete no longer works" part of this issue title is a separate issue: #911830: Can't delete any media item, because all are supposedly in use.
But for this issue, we do need to implement media_field_(insert|update|delete)() to call file_usage_(add|delete)(), much like file_field_(insert|update|delete)() does.
I don't think revisions has anything to do with this. Files are not revisionable. From the perspective of file usage, media entities are just files, so media entities not being revisionable is not an issue. The media field should work when attached to a revisionable entity (since attaching media fields to nodes is one of the main use-cases), so the new media_field_(insert|update|delete)() functions should be written to work when the field is attached to a revisionable entity, but hey, the file_field_*() functions already figured this out, so we can copy them. It does suck duplicating code though. It's interesting that the image_field_(insert|update|delete)() functions just call the file_field_*() ones, so bonus points if media field can be made to do the same: that may require submitting a core patch to file.module to not hard-code assumptions it shouldn't make, but such a patch could be a good thing for other contrib field modules that are kind of like file field but aren't file field.
As per above, even image field is its own field type and not a widget of file field. So, I don't think it's necessary to shy away from media field continuing to be its own field. It should be possible for contrib modules to define field types that work with files without duplicating 100 lines of file.module code. If that's not possible, we need to be submitting patches to improve file.module while we still can. I can see the appeal of wanting to merge file field and media field into one though, given that we simply want all files to be entities. But I don't think we can do such a thing successfully at this time, because we still need the freedom to experiment and try out code in media.module that we won't be able to get into file.module until D8.
I'd love it if we could organize an opportunity to chat with quicksketch and other developers interested in filefield_sources and related modules. Given the state of media for D6, those modules were necessary, but ideally, D7 media could solve everything those modules did, so rather than spending time porting those modules, maybe we can attract their developers to help out on media. Given D7's entity system, I'm just not sure why there's a reason to have a non-entity-based file reuse architecture, which is what D6 filefield_sources is. Why wouldn't D7 filefield_sources want to use entities? And do we really need two totally different entity-based file reuse architectures, or can we combine efforts here?
Anyway, I digress. The focus of this issue is to implement media_field_(insert|update|delete)() to call file_usage_(add|delete)().
Comment #5
effulgentsia commentedWell, here's an attempt towards that: #864800-3: FileField Sources: D7 version.
Comment #6
JacobSingh commentedThis doesn't do a usage check on delete, but implements the file_usage management stuff.
Comment #7
effulgentsia commented90 lines of nearly identical code to what's in file.field.inc. So sad. But looks good, and the duplication is needed until we can make the core functions more flexible. Committed #6 with the addition of this comment: