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.
Talked with KarenS about this today and we're going to look at create a file field for core at the DrupalConDC code sprint on Saturday.
Comment | File | Size | Author |
---|---|---|---|
#90 | file_field.patch | 106.72 KB | quicksketch |
#89 | file_module.patch | 106.72 KB | quicksketch |
#86 | file_module.patch | 106.91 KB | quicksketch |
#85 | file_module.patch | 106.9 KB | quicksketch |
#84 | file_module.patch | 107.38 KB | quicksketch |
Comments
Comment #1
jhedstromSubscribing.
Comment #2
KarenS CreditAttribution: KarenS commentedHere's a shell module to use for this that can be used as a starting point, hacked up from one of the other core fields. It has no file API code in it, just the basic Field API stuff.
Comment #3
drewish CreditAttribution: drewish commentedThings I need to figure out:
* Is file_reference.module really the best name for this? I was kind of hoping to have file.module as an admin module and this syncs up with node and user reference.
* How to find all the table names that a field is stored in so I can implement hook_file_usage() and hook_file_delete().
* Do I need to implement hook_field_load()? It seems like that's where I'd need to call file_load().
Comment #4
yched CreditAttribution: yched commented"How to find all the table names that a field is stored in so I can implement hook_file_usage() and hook_file_delete()."
Hm, interesting. According to the 'swappable storage engine' mechanism, there's currently no real way to do that.
"Do I need to implement hook_field_load()? It seems like that's where I'd need to call file_load()."
Yes, sounds like the right place. You want to populate the actual {file} data into $object->field_foo[delta] items, right ? You'll need to return the new values of the $object additions :
Comment #5
yched CreditAttribution: yched commentedExpanding a little: Field.module doesn't know where fields are stored. The storage engine knows. Currently field_sql_storage.module doesn't publish the information to the outside world (the only approaching function is _field_sql_storage_tablename($name), just an internal helper func).
The approach in #368674: Implement hybrid storage for Fields in Core complexifies this since a little more since it hijacks the storage engine for some fields.
[edit : fixed the link above]
About hook_file_usage() :
Is the solution that filefield stores file usage in its own dedicated table ?
About hook_file_delete() :
Do we need to cleanup file field data when a field is deleted ? Noderef doesn't do so when a node is deleted... but maybe the case of files is different ?
Comment #6
drewish CreditAttribution: drewish commentedah... yeah chx explained that there's not a way to query for a value he just opened up #392494: Fields API needs to allow searching of scalars. i'm looking to avoid the complication so it's got me thinking that #353458: hook_file_references() was not designed for a highly flexible field storage would be good to get finished up so that i can avoid the limitation.
Comment #7
drewish CreditAttribution: drewish commentedyched, for hook_file_delete(), i honestly think that user/node_reference should clean up values when they're deleted but if we move to the table based appraoch it's not really necessary.
Comment #8
yched CreditAttribution: yched commented"user/node_reference should clean up values when they're deleted"
Hmm, referential integrity. Tough issue :-)
Comment #9
aaron CreditAttribution: aaron commentedwe need to be careful about file reusability when cleaning up the table as well; one file might be referenced in three different filefields, then as a file attachment on six different nodes, and maybe a user pic to boot. that's without opening the can of inline worms...
Comment #10
grendzy CreditAttribution: grendzy commentedsubscribing
Comment #11
drewish CreditAttribution: drewish commentedlittle bit further... posting this so i can hand it off to quicksketch
Comment #12
jhedstromThe very simple tests now work (a field is created, and an instance is created).
Comment #13
jpetso CreditAttribution: jpetso commentedsubscribing
Comment #14
quicksketchSubscribe
Comment #15
catchsubscribing.
Comment #16
yched CreditAttribution: yched commentedRemarks :
- hook_field_validate has changed since #369964: Refactor field validation and error reporting was committed. Forget the awful $item['_error_element'] :-)
- hook_field_load() needs to *return* the field values, not alter $items by reference. We need that so that field values can be cached.
also, the function confuses $items and $item in several places.
Comment #17
SeanBannister CreditAttribution: SeanBannister commentedsub
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe.
Comment #19
kevinhammer CreditAttribution: kevinhammer commentedsubscribe
Comment #20
mitchell CreditAttribution: mitchell commentedsubscribe
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone willing to run with this? Less than 3 months until code freeze.
Comment #22
drewish CreditAttribution: drewish commentedi'm off in the contrib weeds and should really focus on some lower level stuff for core. i'll review any patches or answer questions on the file api half.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe. i'm taking up some file-related stuff, so i'll take a look at this.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedDries has asked for this in his blog - buytaert.net/drupal-7-fields-in-core-status-update-and-next-steps
Comment #25
rickvug CreditAttribution: rickvug commented"file_reference.module" does sound like an odd name. What about just "file" or "file field"? Will this be replacing upload.module? In that case, the module name could be kept since it is familiar.
Comment #26
catchYeah upload.module seems like the natural place for this - a patch to add the field itself, then a patch for the upgrade path and removing the legacy code. I'm bumping this to critical - if we ship core with regular old upload module when we have fields in core available, then it's going to be extremely confusing.
Comment #27
drewish CreditAttribution: drewish commentedI think upload would be a great place for this. I'd really rather avoid the confusion of having this collide with the file.inc namespace.
Comment #28
sun#513096: The Future of Image in Drupal 7
Unlike the image namespace, the file namespace is completely safe to use.
Comment #29
quicksketchI'm doing a straight-port of FileField to Drupal 7. I'm really glad to see all the duplicate code that can be removed thanks to the new File API (nice work drewish). However there are going to be several items that need forward-porting from FileField. Here are a couple obvious ones:
#59750: Required flag on file forms breaks on validation
#516768: Image validator has inaccurate message
This patch will be a prerequisite to putting ImageField in core, which will extend FileField and just provide a widget. More details in the issue sun posted above in #513096: The Future of Image in Drupal 7.
Comment #30
gordon CreditAttribution: gordon commentedSubscribe
Comment #31
eojthebraveYes please! Quicksketch, let me know if/where you need help on this one.
Comment #32
mcrittenden CreditAttribution: mcrittenden commentedSubscribe.
Comment #33
quicksketchOkay, the first port of FileField is now ready for testing! I've claimed the "file" project space for this work. And you can download it (as soon as the packaging script runs tonight) from the File project page. Please post bug reports to the File project issue queue.
There's a big long list of patches that need to be applied for it to work. Here's the list again so that those patches might get a little extra attention and clear the way for further development.
#520664: file_get_mimetype() should not be case-sensitive
#457744: node.module misses calls to field_attach_delete() / field_attach_delete_revision()
#520620: Delete revision and prepare translation field hooks are called incorrectly (or not at all)
#520774: Default value field shown even if module specifies to use NONE/CUSTOM default value
#520778: Let a complex widget return a single value
There are also some general bugs with Field module that I haven't made/found issue for. Like you can't delete a field once it's been added, or a multivalue field with an exact number of values doesn't save the first value. So please research whether the problem is a CCK/Field problem or a File problem before filing.
Comment #34
bjaspan CreditAttribution: bjaspan commentedNate, please explain "you can't delete a field once it's been added."
Comment #35
webchick"you can't delete a field once it's been added" - This is probably a CCK bug rather than a FileField bug. I noticed this while testing the taxonomy term patch as well. It also holds true for text fields.
Comment #36
quicksketchI marked #220944: Handle File Uploads in Form API as a duplicate.
Besides just providing a file field for Field module, this provides a completely independent FAPI element called a "managed_file". It has the following attributes:
This FAPI element then handles the entire upload process, and the return value from it is a single $fid that can be used to insert into the module's table for tracking files. The widget uses #attached_css and #attached_js to provide a self-sufficient AJAX-enabled, progress-enabled widget that can be used anywhere in Drupal (site logo and user pictures are obvious targets for this) The Field version of this widget is actually this same element type, only it's extended by an additional #process function to add the "description" and "list" fields.
This advanced FAPI element is the reason for #520778: Let a complex widget return a single value, in case further explanation was needed for that issue.
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedThat FAPI element is so so fine. Kudos.
Comment #38
aaron CreditAttribution: aaron commentedgreat work! has anyone put thought into how this will affect #227232: Support PHP stream wrappers? i'll try to take a look at it all this weekend.
Comment #39
jmstacey CreditAttribution: jmstacey commentedsubscribe
Comment #40
quicksketchI've basically finished porting ImageField to Drupal 7 also, though the web of patches required to get everything working is completely overwhelming and it's really hard to make patches. Here's the image.field.inc file, even though it's not functional without additional patches to image.module, but at least you can see how Image module will extend File. The best thing to get this unblocked is to finish up the ImageCache UI patch, which is the largest chunk of changed code in the way (besides the CCK UI of course).
Comment #41
yched CreditAttribution: yched commentedInitial remarks after a quick read
- image_field_widget_process()
This should not be needed, field_read_fields() and field_read_instances() ensure that all expected field, instance, widget and formatter settings come with all expected values present, using defaults if needed. So
$settings = $instance['widget']['settings'];
should be enough.- image_field_widget_info()
Most of the settings sound more like field or instance settings than widget settings. For some of them which are more 'image' related, it's probably a consequence of the fact that there's no 'Image' field type, only image-specialized widgets and formatters for 'File' fields.
'file_directory', 'max_filesize_per_field', 'max_filesize_per_file' do really sound like instance settings to me, though.
- theme_image_widget()
nitpick:
<div class="form-managed-file ...">
- what's in there is more than just a simple form-managed-file element, right ?- The patch currently does not make use of formatter settings - I would have expected imagefield to be a great case for those.
I'd tend to think the three formatters could be joined in one, with option 'link' = no_link|link_content|link_file ?
I guess there can be different scenarios here (including imagecache styles)
- The image_link_content formatter raises an interesting issue: we currently have no generic, reliable way to generate the url of an 'object' (node, user, comment, taxo term, contrib fieldable entity). The patch assumes "{$object_type}/{$object_id}", which happens to work for nodes, users and comments, but is obviously not future proof.
I opened #525622: 'as link' formatters need a generic way to build the url of an 'entity' for this.
- More generally, all formatters for all field types could benefit from generic 'as link' options. Even cooler with a 'custom path' option similar to the one present in Views 2 for views fields. Only pipe dream for now, and obviously not a job for this patch, but I just though I'd share :-)
Comment #42
webchickJust an update to note that all of the dependent issues in #33 are committed to HEAD now.
Comment #43
quicksketchThanks yched! That's all tremendously helpful information. I based most of the code off of existing CCK UI, which seems to do
$defaults = field_info_widget_settings($instance['widget']['type']);
in several places. Maybe that's been fixed since I was looking at it.I didn't even know formatters could have settings now, that's fantastic news.
As for the "widget" vs. "instance" settings, this is almost entirely because the current CCK UI segregates the instance settings from the widget settings, making the ordering of the form really funky, since it's not possible to put all the fieldsets down at the bottom and the common settings at the top. But it's more a problem that we should fix in CCK UI, rather than working around it here.
Comment #44
yched CreditAttribution: yched commentedYes, in D6 field type modules do not expose default values for their settings. We added that in D7 to get rid if painful (if isset()) code in field hooks.
Formatters can have settings, but the current UI (CCK HEAD or #516138: Move CCK HEAD in core) does not reflect that for now :-/. Tricky UI pattern, kind of begs for AHAH subforms... That's one of the reasons why I hope Field UI gets more attention that just 1 or 2 people ;-)
More generally, the breakdown and organisation of 'field edit subforms' in the UI still needs to be mapped out. I'm leaning towards separating into smaller subforms (field settings separated from widget settings).
Comment #45
drewish CreditAttribution: drewish commentedRe #40:
The comment seems odd...
Also wanted to note that #370537: Allow suppress of form errors (hack to make "more" buttons work properly) is a prerequisite.
Comment #47
deekayen CreditAttribution: deekayen commentedbad trial run of new testing bot
Comment #49
deekayen CreditAttribution: deekayen commentedmisbehaving bot again
Comment #50
plachComment #51
eojthebraveThese notes were taken over the course of a couple of evenings so might not make total sense. Let me know if you have questions. Also, I don't know where you're at with this currently but let me know if you need help implementing any of this. And/Or where a good spot to jump in would be.
This is strictly a code review. I haven't even attempted to install the module yet. I'll try and do that in the next day or two and will surely have more notes then.
----------------------------------------------
file.field.inc
----------------------------------------------
- file_field_schema
Should fid be allowed to be set as null?
- file_field_sanitize
Remove extra newline at top of function
line #140, checks for existence of function and then goes ahead and tries to use the function wether it exists or not.
line #183, "deleting" should be "delete"
line #211 could this
return empty($item['fid']) || (int) $item['fid'] == 0;
be
return empty($item['fid']) || $item['fid'] === 0;
line #215, I think "Determine" would be better here than "Return", and what about "node" are files always listed in conjunction with a node or will it be possible to list files with other fieldable objects?
- file_field_listed
phpdoc needs @return
line #323
" * A CCK field array." seems wrong
line #351, file_field_widget_associate needs @param docs
line #357, needs a period
line #366, file_field_widget_file_path, you never use the $account param for anything. Presumably this is for token replacement stuff and there is a to do for that. So either implement that, or remove this argument for now.
line #455, theme_file_widget has no comments
line #460 and #461 have improper spacing for concat at the end of each line.
line #479, dsm()
----------------------------------------------
file.admin.inc
----------------------------------------------
_file_generic_settings_file_directory_validate and _file_generic_settings_max_filesize both need documentation
----------------------------------------------
FILE.MODULE
----------------------------------------------
Needs some DBTNG love.
line #58, Implementation should be Implement
line #133-135, this should be all on one line.
line #170
// You don't have permission to view the node this file is attached to.
would prefer something like// The user does not have permission to view the node this file is attached to.
line #194
* Menu callback; Shared AHAH callback for uploads and deletions.
add "file" between "for" and "uploads" and this comment block needs re-wrapping.file_theme() returns theme functions for image.field.inc. Those should probably go in image.module, especially since the functions are not even defined in file.module.
phpdoc for file_ahah_js needs to be re-wrapped
line #214,
$form_build_id = $form_build_id;
don't think this is needed.line #219, can we have an error message that is a little more user friendly. I doubt the end user cares that this form was missing form the server cache.
file_progress() should probably document the $key param
line #284,
* Determine which upload progress implementation to use, if any available.
should be* Determine which upload progress implementation to use, if any are available.
line #287, needs to use new static variables API
line #304 and #312, Implementation should be Implement
line #315, file_file_delete function needs to do something or be removed.
line #315, missing code, though you're probably aware of that.
line #318, extra newline
line #385, re-wrap comments
line #408 and #415, why all caps for key names?
line #426
// And finally, the file upload field itself.
maybe change to something like// Add the file upload field.
line #490,
could probably be consolidated to a single line
line #523, don't think you need to pass the $form variable by reference.
file_managed_file_save_upload should document a @return value.
line #589 and #601, why not just delete this line? $output = '' isn't necessary here.
line #647.
return '<img class="file-icon" alt="" title="' . $mime . '" src="' . $icon_url . '" />';
could you use theme('image') here instead.line #720, file_icon_map needs documentation
line #868,
// Filter down the list just to file fields.
is a little weird. Maybe something like// Filter the list to just file fields.
line #898,
foreach ($results as $type => $type_results) {
could just beforeach ($results as $result) {
line #964 file_get_attached_files, I think $entity_id should be $object_id for clarity.
I really like the new managed_file element, but it needs documentation and lots of it. Should we consider adding the managed_file element to FAPI so that other modules can use it without depending on file.module?
Comment #52
int CreditAttribution: int commented#516138: Move CCK HEAD in core has committed..
Comment #53
int CreditAttribution: int commentedadd tag.
Comment #54
webchickCould we get an update on what (if any) blocking issues are here? Is it just time? Is there a way someone else could jump in and help you?
I think I speak for everyone when I say that we would LOVE this to be part of D7. :)
Comment #55
webchickx
Comment #56
aaron CreditAttribution: aaron commentedI'm happy to jump in next week and look at this, in particular to review the impact on streams (if any) on this patch. Probably Tuesday or Wednesday; marking it on my calender now.
Comment #57
quicksketchHey all. FileField is 100% up-to-date and working with both the new AJAX system AND stream wrappers. It's just in the File project http://drupal.org/project/file. Just download the latest version and try it out! I'm waiting on posting it as a core patch until I update my tests, which I'm in the process of completing.
There are currently NO blocking issues though. Yay!
Comment #58
alippai CreditAttribution: alippai commentedmaybe this is related: #452446: Select multiple files to upload
Comment #59
joshmilleradding tag...
that is good news quicksketch!!
Comment #60
quicksketchI nearly got the tests working but then I CVS updated core and everything broke due to #497118: Remove the registry (for functions) and #367595: Translatable fields. I updated file.module as far as I could but I hit a major, major problem regarding finding which file contains the original form that contains any file. For node module this is node.pages.inc, user.module is just user.pages.inc, plus any of the hundreds of potential files that could contain a managed_file element in the future.
I'm stumped on how to fix this efficiently. Moshe made a good suggestion of trying to use the Reflection class provided by PHP 5 (apparently we're already using this in update.php), which would look something like this:
Of course we don't want to do this every time a file field is shown, but we need to actually get the file name during the building of the form and recording it somewhere, that way we can include that file again when we rebuild the form during the AJAX uploading.
So generally we're still looking pretty good, updating for all the recent core changes hasn't been too bad, but this is a bit of a brutal problem that I didn't need to consider before when the function registry was doing this for us.
Comment #61
webchickHm. I didn't quite follow that. Could you copy/paste the relevant bits from file module that might help someone jump in here with alternate suggestions?
Comment #62
webchickAdditionally, do we really need this flexibility? Can't we just force the form definition and AJAX callback functions to be defined in the same file? You can't split hook_menu() definitions from their access callbacks, either.
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commentedAlso, how many different callbacks are we talking bout per page? I think a few calls to reflection API will be fine 100 calls might be bad. I would proceed with reflection and then see how we do on benchmarks. My .02.
Comment #64
quicksketchAll forms will always use the same AJAX callback when a file is uploaded. Take the node form for example, which is located in node.pages.inc (the node_form() function). When the file_ajax_js() function is called to handle the AJAX upload, it needs to rebuild the original form to handle the upload. However, if this function is in an include file, we don't have any way of knowing what file contains the form function, so when drupal_get_form('node_form') is fired, we get a function not found error and the whole thing bombs out.
While we could hard-code a few paths like the node form, it won't be a good long-term solution since we need to have file uploads in all kinds of locations, like user-profile pages, taxonomy terms, site logo upload, and other places, nearly all of which are forms that are part of an .inc file.
Comment #65
quicksketchAnother option that I'm trying now is to record the menu entry when the form is originally being built. Then in the AJAX request we can check this include file property and pull it in. Unfortunately this only works for the first time the form is built, since subsequent buildings are done by the AJAX request.
Comment #66
yched CreditAttribution: yched commentedIt's not specific to File field's own 'add more', this exact same issue is breaking #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'].
- ajax_form_callback() doesn't find the callback node_form(), and the the $form is not fully built.
- field_add_more_submit() relies on a $form['#builder_function'] callback (e.g node_form_submit_build_node() ), which it doesn't know how to load.
re webchick #62: Well, Field API is all about adding form elements on forms it doesn't control and doesn't really want to know about...
Was not an issue in D6 without the registry, because CCK knew it worked on node forms.
Comment #67
catchThe easiest thing to do here would be to move these forms back into .module - since they're now API functions which can be called from anywhere, then document this as one of the necessities for making entities fiedlable.
Comment #68
rfayThe same issue is breaking #556438: AJAX/AHAH 'callback' support only works for 'submit' and 'button' elements - Should work for all triggering elements. Thank you all for pioneering the way here.
Comment #69
quicksketchHmm, thanks yched, catch, and rfay for your input. I didn't even think about this affecting other AJAX behaviors (like the "Add more" button), but of course they are all equally affected. I'm rather bummed about this development, since file.module actually makes a full FAPI element that will work anywhere in Drupal (or more specifically in any .module file apparently). This just seems like a bizarre restriction to introduce, especially since this will affect a lot of forms we we switch them to using #type => managed_file (which I think we should).
- User picture upload
- Logo upload
- Favicon upload
- Any file field on nodes, taxonomy terms, or users
- Field configuration for default file value
That's a lot of forms to move into modules, but I don't have a better solution that works.
Comment #70
quicksketchLet's pick up the broken AJAX functionality in #559526: All AJAX Behaviors Broken for Forms in .inc files, where I've proposed an alternative to fix this problem.
Comment #71
quicksketchAfter much, much catching up with changed APIs and fixing of tests, I'm happy to present a patch! This patch is an exact copy of CVS HEAD for file.module. So if it's too much work to apply, just try out the latest version of File module (preferably from CVS).
You'll need to apply this patch then expand the icons and put them into /modules/file/icons.
Comment #72
quicksketchDoh, first flub. I didn't include the tests! Same patch with tests.
Included tests:
- Revision tests (upload a file, create new revisions, delete revisions, check files are not deleted when in use)
- Upload location tests (with token support!)
- File validation tests (Extension check, Size check, Required check)
- Formatter display test
Comment #73
quicksketchFor bonus points, you can also apply the Image Field patch and get an "Image" widget! Apply this patch then head over to #560780: Add Image Field to image.module and apply that one to Image module for a better image uploading experience.
Comment #74
Dries CreditAttribution: Dries commentedOh, behave, baby! :)
Comment #75
cburschkaI'm not sure whether the right way here would be './' or DRUPAL_ROOT . '/modules/file/', but the dirname(__FILE__) thing is not used anywhere else in core.
(A minor nitpick - "pull in" isn't a term I've seen applied to loading a code file before.)
Beer-o-mania starts in 4 days! Don't drink and patch.
Comment #76
cburschkaSee what happens when I go away for three hours while reviewing. :P
Comment #77
catchOnly got about 10% through then ran out of time, but a few minor things I spotted to save others doing so:
This is already in system_requirements() as of D7, no need to duplicate.
Won't the warnings for upload progress flag a 'problem with your Drupal installation' message on every admin page as REQUIREMENT_WARNING - seems a bit much for a progress bar, and it's something that people might not be able to affect on shared hosting.
Not 100% but I don't think these are needed any more after registry rip?
Don't think it matters having this filled in, but would be more standard with // $id$
Should be module_load_include(), or if this is always going to be loaded anyway, they could move back into file.module without the need for the separate include.
Assuming this is OK for a followup patch.
Will try to look more later on, and actually try it out.
I'm on crack. Are you, too?
Comment #78
catchfixing tags again. Drupal 8 needs to handle comment cross-posts out of the box.
Comment #79
mcrittenden CreditAttribution: mcrittenden commentedRe-tagging.
EDIT: oops, too late!
Comment #80
jrglasgow CreditAttribution: jrglasgow commentedWhen trying to upload a file with the javascript uploader I get the error
It does upload the file when submitting the node form.
Also when viewing the node I don't see the files listed anywhere. The display settings for the field are "Generic File", but all I have is the body. I also tried "table of files" and it did nothing in the node view.
Comment #81
yched CreditAttribution: yched commented"Also when viewing the node I don't see the files listed anywhere. The display settings for the field are "Generic File", but all I have is the body. I also tried "table of files" and it did nothing in the node view."
Could be [#http://drupal.org/node/553292] - needs a reroll, couldn't get back to it in the last couple days.
Comment #82
jrglasgow CreditAttribution: jrglasgow commentedmy problem in #80 was solved by simply clearing the cache (a problem in and of itself), apparently I forgot after applying the patch
Comment #83
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #84
quicksketchIn general
dirname(__FILE__)
is more flexible because it allows the module to be anywhere (for example I'm developing in sites/all/modules/file, not modules/file). However this isn't the place to fix that problem so I've switched to using DRUPAL_ROOT.We can't use module_load_include() during installation, so I'm using DRUPAL_ROOT as recommended by Arancaytar. I think splitting them still makes total sense, since it's a very clean split between "general" file handling hooks and "field" hooks. Field module does something similar, since 2000+ line files get a bit daunting.
Yeah... I think I was just expecting the requirement to get downgraded to 5.1. We're going to make a LOT of unhappy RHEL users (who are stuck on 5.1 because RedHat likes to
supportrip off people for 5 years at a time). Anyway I've removed it even though I think we'll ultimately have to downgrade to 5.1 for Drupal, and the progress bar capability was added in 5.2.Downgraded to REQUIREMENT_INFO.
They're still needed for *class* registry, but I'm not clear on where these are needed and not. Every other module is still listing all files so I figured I match for the time being.
This patch accommodates for all the above changes.
Comment #85
quicksketchI removed some references to #include_files that I had in place until #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] was committed that fixed the same problem with include files not being pulled in.
Comment #86
quicksketchWell now I broke the DRUPAL_ROOT change... this is the trouble with trying to move contrib to core. :-/
Comment #87
ksenzeeI applied the patch and banged on it, and didn't run into any problems, so I'm reduced to actually reviewing the code. :) I'm no Field API guru, but here's my review, fwiw.
Is an optional module the right place for the managed_file field? I'm wondering if other places in core might want to be able to depend on it.
It might make sense to extend FAPI to allow multiple
#value_callback
s, now that there's a use case for it in core. That seems like an enhancement, though, not a showstopper.I believe this is a per-instance setting, not a per-field setting.
s/loose/lose
After looking through the regexes, I think I get what's going on here, but "reduce the replacement by one parent" is kind of confusing. Maybe something like "Adjust the AJAX settings so that on upload and remove, this element's parent (i.e. the entire group) gets replaced."?
Looking forward to seeing this in core. I think it belongs there.
I'm on crack. Are you, too?
Comment #88
catchStill haven't made it all the way though, sorry for piecemeal, nitpicky comments, on the plus side I'm either missing serious issues or there aren't any. And RHEL users can stay on Drupal 6 or change operating system IMO.
Leftover?
Should be drupal_process_form() so it gets highlighted.
Why?
Should be a defgroup.
Why would $field ever be set, but not a string?
4 days to code freeze. Better review yourself.
Comment #89
quicksketchksenzee feedback:
I think it's very likely that File will become a required module if we make use if it for user pictures. Otherwise I'm a bit nervous about jamming 600 lines into system module to get the managed_file functionality.
Sure enough. And it turns out we can't specify it in hook_field_info() at all so I removed it.
Fixed
I went with this:
catch feedback:
It was actually intentional to keep that debug() in there so that it was easy to manually test and see the results of uploading through the test form, but since you're the 3rd person to think it is accidental and we won't need to manually test it anymore anyway, I removed it.
I think this was caused by some bug early in the render() patches. It seems to work fine again without rendering the form first, so I removed the code comment and extra line rendering the form first.
I've been using file_get_field_list() as a way of getting a consistent data structure to loop over, so actually within file.module the $field value is always a field array (or NULL). However I figured that this would be more helpful to other developers as a string. So it's for efficiency within file.module that we don't retrieve the field array again, since we already have it. I've updated the PHPdoc to clarify that either a string or array is allowed.
Comment #90
quicksketcharianek found a bug where if "Number of values" is set to some specific number (not 1 or unlimited) then the field is always marked as required. This patch corrects that problem. It also puts "File" in the normal "Core" category on the modules page.
Comment #91
arianek CreditAttribution: arianek commenteddid a load of testing, here be my report!
- when you set a default file + make the field req'd = it does not count the default file as one, and says req'd field must be filled out! (this is something that needs to be made clearer in the UI if you don't want people to do this, but is a user error more than a code error)
- when not a req'd field + multiple field allowed (3 for eg) prompts that the field is req'd regardless of having uploaded (quicksketch has fixed this already)
- i set file upload limit to 40KB and just uploaded a 2.6MB one (quicksketch reports this is just for user1)
- the field label field accepts a lot of chars, which leads to theming fail! on /admin/structure/node-type/article/fields long field name = http://skitch.com/arianek/b7fe8/article-localhost.drupal7 and node view: http://skitch.com/arianek/b7fe1/testing-more-localhost.drupal7 (submitting this as an issue on field_ui.module)
with those fixes: thumbs up!
Comment #92
ksenzeeI second the motion. This is ready to go in. No offense, upload.module, but the time has come.
Comment #93
catch#563000: Provide upgrade path to file
Comment #94
Dries CreditAttribution: Dries commentedAwesomeness. Committed to CVS. Great work quicksketch and drewish.
Comment #95
Dries CreditAttribution: Dries commentedAwesomeness. Committed to CVS. Great work quicksketch and drewish.
Comment #96
quicksketchLooks like the icons from #71 weren't added.
Comment #98
webchickHuh. I can't commit these images:
I'm guessing I know who "second party" is. ;)
Comment #99
int CreditAttribution: int commentedI supost that you have to remove the CVS directory inside the zip file..
Comment #100
cburschkaIsn't CVS lovely? ;-)
And yeah, icons don't seem to be in the repository yet. int's right, the CVS folder inside the zip is probably interfering.
(It'd be cool if we could include images as base64 or hex in diff. They're not very large files...)
Comment #101
Dries CreditAttribution: Dries commentedCommitted the icons to CVS HEAD.
Comment #102
quicksketchYay, thanks Dries. :D
Comment #105
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedMarked #14192: Help wording confusing for edits to attachments as duplicate.