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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Subscribing.

KarenS’s picture

FileSize
3.58 KB

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

drewish’s picture

Status: Active » Needs work
FileSize
12.09 KB

Things 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().

yched’s picture

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

array(
  'field_foo' => array(
    0 => array('fid' => '...', 'filename' => '...', ...),
    1 => ...,
  ),
);
yched’s picture

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

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

drewish’s picture

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

drewish’s picture

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

yched’s picture

"user/node_reference should clean up values when they're deleted"
Hmm, referential integrity. Tough issue :-)

aaron’s picture

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

grendzy’s picture

subscribing

drewish’s picture

Assigned: drewish » Unassigned
FileSize
11.9 KB

little bit further... posting this so i can hand it off to quicksketch

jhedstrom’s picture

FileSize
11.63 KB

The very simple tests now work (a field is created, and an instance is created).

jpetso’s picture

subscribing

quicksketch’s picture

Subscribe

catch’s picture

subscribing.

yched’s picture

Remarks :
- 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.

SeanBannister’s picture

sub

moshe weitzman’s picture

subscribe.

kevinhammer’s picture

subscribe

mitchell’s picture

subscribe

moshe weitzman’s picture

Anyone willing to run with this? Less than 3 months until code freeze.

drewish’s picture

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

Anonymous’s picture

subscribe. i'm taking up some file-related stuff, so i'll take a look at this.

moshe weitzman’s picture

Dries has asked for this in his blog - buytaert.net/drupal-7-fields-in-core-status-update-and-next-steps

rickvug’s picture

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

catch’s picture

Priority: Normal » Critical

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

drewish’s picture

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

sun’s picture

#513096: The Future of Image in Drupal 7

Unlike the image namespace, the file namespace is completely safe to use.

quicksketch’s picture

Assigned: Unassigned » quicksketch
Issue tags: +ImageInCore

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

gordon’s picture

Subscribe

eojthebrave’s picture

Yes please! Quicksketch, let me know if/where you need help on this one.

mcrittenden’s picture

Issue tags: +upload, +FileField

Subscribe.

quicksketch’s picture

Status: Needs work » Needs review

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

bjaspan’s picture

Nate, please explain "you can't delete a field once it's been added."

webchick’s picture

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

quicksketch’s picture

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

$form['file'] = array(
  '#type' => 'managed_file',
  '#default_value' => $fid, // Any File ID.
  '#progress_indicator' => 'throbber', // or 'bar', automatically adds progress indication if server supports it.
  '#progress_message' => t('Please wait...'), // Message is overridden by progress indication if enabled.
  '#upload_validators' => array( // Same as the current #upload_validators for 'file' types.
    'file_validate_size' => array($size),
    'file_validate_extensions' => array('png gif jpg'),
  ),
  '#save_directory' => file_directory_path() . '/uploads',
);

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.

moshe weitzman’s picture

That FAPI element is so so fine. Kudos.

aaron’s picture

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

jmstacey’s picture

subscribe

quicksketch’s picture

FileSize
6.9 KB

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

yched’s picture

Initial remarks after a quick read

- image_field_widget_process()

  $defaults = field_info_widget_settings($instance['widget']['type']);
  $settings = array_merge($defaults, $instance['widget']['settings']);

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

webchick’s picture

Just an update to note that all of the dependent issues in #33 are committed to HEAD now.

quicksketch’s picture

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

yched’s picture

Yes, 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).

drewish’s picture

Re #40:

  // Check if using the default title and replace tokens.
  $default_title = (!$settings['title_field'] || (empty($item['status']) && empty($item['data']['title'])));

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

deekayen’s picture

Status: Needs work » Needs review

bad trial run of new testing bot

Status: Needs review » Needs work

The last submitted patch failed testing.

deekayen’s picture

Status: Needs work » Needs review

misbehaving bot again

plach’s picture

eojthebrave’s picture

Status: Needs review » Needs work

These 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,

$return = empty($element['#extended']) ? $fid : $extended;
return $return;

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 be foreach ($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?

int’s picture

int’s picture

Issue tags: +Fields in Core

add tag.

webchick’s picture

Could 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. :)

webchick’s picture

Issue tags: +Drupal 7 priority

x

aaron’s picture

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

quicksketch’s picture

Status: Needs work » Needs review

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

alippai’s picture

joshmiller’s picture

adding tag...

that is good news quicksketch!!

quicksketch’s picture

Status: Needs review » Needs work

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

  // Record the original file containing the form function so we can include it
  // during AJAX requests.
  $args = $form_state['args'];
  $forms = module_invoke_all('forms', $form_id, $args);

  if (isset($forms[$form_id]['callback']) && !function_exists($forms[$form_id]['callback'])) {
    $func = new ReflectionFunction($forms[$form_id]['callback']);
    debug($func->getFileName());
  }

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.

webchick’s picture

Hm. 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?

webchick’s picture

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

moshe weitzman’s picture

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

quicksketch’s picture

Can't we just force the form definition and AJAX callback functions to be defined in the same file?

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

quicksketch’s picture

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

  // Because this file field may be shown on forms that are not in module files,
  // we need to keep record of where this form originated so that we can pull
  // in the same include file during AJAX uploads.
  $menu_item = menu_get_item();
  if (!empty($menu_item['file'])) {
    $element['#include_file'] = $menu_item['file'];
  }
yched’s picture

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

catch’s picture

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

rfay’s picture

quicksketch’s picture

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

quicksketch’s picture

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

quicksketch’s picture

Status: Needs work » Needs review
FileSize
6.53 KB
76.54 KB

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

quicksketch’s picture

FileSize
106.56 KB

Doh, 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

quicksketch’s picture

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

Dries’s picture

Issue tags: +Favorite-of-Dries

Oh, behave, baby! :)

cburschka’s picture

Issue tags: -Favorite-of-Dries
+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,968 @@
+require dirname(__FILE__) . '/file.field.inc';

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

cburschka’s picture

Issue tags: +Favorite-of-Dries

See what happens when I go away for three hours while reviewing. :P

catch’s picture

Issue tags: -Favorite-of-Dries

Only got about 10% through then ran out of time, but a few minor things I spotted to save others doing so:

+++ modules/file/file.install	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,62 @@
+    $php_52 = version_compare(phpversion(), '5.2.0', '>');

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.

+++ modules/file/file.info	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,10 @@
+files[] = file.module
+files[] = file.field.inc
+files[] = file.install
+files[] = tests/file.test

Not 100% but I don't think these are needed any more after registry rip?

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,968 @@
+// $Id: file.module,v 1.44 2009/08/27 02:18:13 quicksketch Exp $

Don't think it matters having this filled in, but would be more standard with // $id$

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,968 @@
+require dirname(__FILE__) . '/file.field.inc';
+

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.

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,968 @@
+/**
+ * Implement hook_file_delete().
+ */
+function file_file_delete($file) {
+  // TODO: Remove references to a file that is in-use.
+}

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?

catch’s picture

Issue tags: +Favorite-of-Dries

fixing tags again. Drupal 8 needs to handle comment cross-posts out of the box.

mcrittenden’s picture

Re-tagging.

EDIT: oops, too late!

jrglasgow’s picture

When trying to upload a file with the javascript uploader I get the error

The page at http://localhost says:
An HTTP error 0 occurred.
Path: /drupal-7/file/ajax/field_file/zxx/0/form-523e78ea1f6fe135c9b03c6924f571e

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.

yched’s picture

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

jrglasgow’s picture

my problem in #80 was solved by simply clearing the cache (a problem in and of itself), apparently I forgot after applying the patch

bjaspan’s picture

subscribe

quicksketch’s picture

FileSize
107.38 KB

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

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

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.

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.

This is already in system_requirements() as of D7, no need to duplicate.

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 support rip 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.

REQUIREMENT_WARNING - seems a bit much for a progress bar,

Downgraded to REQUIREMENT_INFO.

Not 100% but I don't think these are needed any more after registry rip?

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.

quicksketch’s picture

FileSize
106.9 KB

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

quicksketch’s picture

FileSize
106.91 KB

Well now I broke the DRUPAL_ROOT change... this is the trouble with trying to move contrib to core. :-/

ksenzee’s picture

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

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+ * Defines a "managed_file" Form API field and a "file" field for Field module.

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.

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+      // Because FAPI does not allow multiple #value_callback values like it
+      // does for #element_validate and #process, this fills the missing
+      // functionality to allow File fields to be extended through FAPI.
+++ modules/file/file.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,922 @@
+      'default_value_function' => FALSE,

It might make sense to extend FAPI to allow multiple #value_callbacks, now that there's a use case for it in core. That seems like an enhancement, though, not a showstopper.

+++ modules/file/file.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,922 @@
+      'default_value_function' => FALSE,

I believe this is a per-instance setting, not a per-field setting.

+++ modules/file/file.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,922 @@
+    // Checkboxes loose their value when empty.

s/loose/lose

+++ modules/file/file.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,922 @@
+  // Adjust the AJAX settings to reduce the replacement by one parent.

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?

catch’s picture

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

+++ modules/file/tests/file_module_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,52 @@
+function file_module_test_form_submit(&$form, &$form_state) {
+  debug($form_state);
+}

Leftover?

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+  // This call recreates the form relying solely on the form_state that the
+  // drupal_process_form set up.

Should be drupal_process_form() so it gets highlighted.

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+  // drupal_render() MUST be run before the status messages.

Why?

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+
+/*******************************************************************************
+ * Public API functions for File module.
+ ******************************************************************************/

Should be a defgroup.

+++ modules/file/file.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,956 @@
+    if (is_string($field)) {

Why would $field ever be set, but not a string?

4 days to code freeze. Better review yourself.

quicksketch’s picture

FileSize
106.72 KB

ksenzee feedback:

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.

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.

+      'default_value_function' => FALSE,

I believe this is a per-instance setting, not a per-field setting.

Sure enough. And it turns out we can't specify it in hook_field_info() at all so I removed it.

+ // Checkboxes loose their value when empty.

Fixed

Maybe something like "Adjust the AJAX settings so that on upload and remove, this element's parent (i.e. the entire group) gets replaced."?

I went with this:

  // Adjust the AJAX settings so that on upload and remove of any individual
  // file, the entire group of file fields is updated together.

catch feedback:

+function file_module_test_form_submit(&$form, &$form_state) {
+ debug($form_state);
+}
Leftover?

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.

+ // drupal_render() MUST be run before the status messages.
Why?

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.

+ if (is_string($field)) {
Why would $field ever be set, but not a string?

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.

quicksketch’s picture

FileSize
106.72 KB

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

arianek’s picture

Status: Needs review » Reviewed & tested by the community

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

ksenzee’s picture

I second the motion. This is ready to go in. No offense, upload.module, but the time has come.

catch’s picture

Dries’s picture

Awesomeness. Committed to CVS. Great work quicksketch and drewish.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Awesomeness. Committed to CVS. Great work quicksketch and drewish.

quicksketch’s picture

Status: Fixed » Reviewed & tested by the community

Looks like the icons from #71 weren't added.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Reviewed & tested by the community

Huh. I can't commit these images:

cvs add: `icons/application-octet-stream.png' added independently by second party
cvs add: `icons/application-pdf.png' added independently by second party
cvs add: `icons/application-x-executable.png' added independently by second party
cvs add: `icons/audio-x-generic.png' added independently by second party
...

I'm guessing I know who "second party" is. ;)

int’s picture

I supost that you have to remove the CVS directory inside the zip file..

cburschka’s picture

Isn'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...)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed the icons to CVS HEAD.

quicksketch’s picture

Yay, thanks Dries. :D

Status: Fixed » Closed (fixed)

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

drupal_was_my_past’s picture