This patch builds on #391330: File Field for Core and adds an "Image" widget to the file field type. The changes are fairly minimal, since file.module does the huge bulk of the work. I should provide a good usability increase for uploading images (with the image preview) and also demonstrate a solid approach to extending file.module (as may be used by video or audio modules in D7).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Issue tags: +ImageInCore

Of course you need to apply #391330: File Field for Core first for this to do anything. Visit admin/structure/types and add a "File" type field, and select the "Image" widget.

A quick preview of the Image field in action:

imagefield-d7-preview
SeanBannister’s picture

Sub, will test it this weekend at the Brisbane Code Sprint

Dries’s picture

Issue tags: +Favorite-of-Dries

Tracking and tagging. :)

cburschka’s picture

I'm getting warm fuzzies from this.

dman’s picture

Immanentize the Convergence!
Io! Io! Io!

yched’s picture

First : awesome. I have no real code remarks on this patch, I'll try to review the Filefield patch asap.
Also: see #525622: 'as link' formatters need a generic way to build the url of an 'entity' to enhance 'image with link to original content' formatter on non-node entities.

The thing that bugs me is the implementation of Imagefield as an image 'widget' on a Filefield.
This model is already in place in D6 and it seems no-one died so far, so this is probably no blocker, and I assume quicksketch gave it a good thought already, but I'd like to see if there's a different way.

Pros:
- (probably the initial reason) This does save a lot of code duplication - ideally, the right answer for this would be field types as classes, where a field type can subclass and specialize another one, but obviously, we won't be there in D7.

Cons:
- It's a hack in the field type / widget / formatters model, forcing some settings to be widget settings, while they should be instance settings : (alt|title|description)_field, (min|max)_resolution, and some workaround code in image_field_create_instance() to adjust settings (won't work if an 'image' widget is applied later on an existing file field, though).
- We get a set of 'image' formatters that apply to generic File fields while they don't make sense there.
- For the end users in Field UI, it means they don't add an 'Image field', they add a 'File field', and have to set it up with 'Image widget'.
Also, Field UI cannot get rid of the current 'edit field' form soup where 'widget settings' and 'instance settings' are presented in a giant form without structure, because the 'image widget' settings mentioned above would confuse the users if they actually showed up in a 'widget settings' section.

So I'm wondering :
- are there other reasons for this model other than saving code duplication ?
- alternate model 1: could we implement a true 'Image Field' type, building its hook_field_* implementations on Filefield's own versions of the hooks ? image_field_widget() does that in the current patch, calls file_field_widget().
- alternate model 2: could we add 'image_specialization = TRUE / FALSE' as an instance setting on a File field, and move all the arguable 'widget settings' above to be instance settings as well ? #557272: FAPI: JavaScript States system could be used to present only relevant settings on forms.

jrglasgow’s picture

I am having a couple of problems getting this to work

  • I had a problem uploading the image using the javascript uploader
  • I set the field display settings to "Image" for both the teaser and the full node, yet I didn't have any of my images displayed in the node

Even though I couldn't get the javascript uploader to work, the images were still saved when I submitted the node form. Multiple images work the same way, the thumbnails look great.

I was a little disappointed that I didn't see a way to restrict the image size (resolution - max/min)

Other than that I think it is ready.

quicksketch’s picture

I was a little disappointed that I didn't see a way to restrict the image size (resolution - max/min)

Well that's a good point! This patch does have support for min/max image size, but seems that I cut out the UI for entering the values. I'll put those in and repost.

As for Image-as-Field vs. Image-as-Widget: We tried this in both Drupal 5 and in an early version of Drupal 6. Then end-result was that we found ImageField and FileField shared too much code to be separate types. If ImageField were to be implemented as a field, all of these functions would need to be reimplemented:

hook_field_info
hook_field_schema
hook_field_settings_form
hook_field_load
hook_field_sanitize
hook_field_insert
hook_field_update
hook_field_delete
hook_field_delete_revision
hook_field_is_empty
hook_field_formatter_info_alter (to make Images work with generic file formatters)
hook_file_download
hook_file_references
hook_file_delete

Even though file.module in D7 will provided the abstracted "managed_file" type (which cuts down on a massive amount of duplicated widget code), this is still a lot of code to duplicate/abstract. We could do this sort of thing all over the place:

function image_field_schema() {
  $schema= file_field_schema();
  $schema['alt'] = $additions;
  unset($schema['description']);
  return $schema;
}

But we'd need to add a bit of abstraction in all the places where we check for the "file" type. Things like hook_file_references() were always breaking when we had ImageField separate from FileField.

So the bottom line here is that it would definitely be easier to make an "Image" type separate from "File" than it was in the past, since all of our AJAX uploading code is abstracted into a general FAPI field. However at the same time we'd still need to duplicate a dozen hooks that will be nearly (but not quite) identical.

cburschka’s picture

Minor point: I'm not sure if I've understood Field dependencies correctly, but if image.module adds a widget for a field in file.module, does this mean image.module depends on file.module? Should be in the .info if so.

bjaspan’s picture

subscribe

quicksketch’s picture

FileSize
13.07 KB

Patch now with Image resolution checking UI added.

quicksketch’s picture

I'm not sure if I've understood Field dependencies correctly, but if image.module adds a widget for a field in file.module, does this mean image.module depends on file.module?

Image module will provide a File widget if File module is available (I think it's likely that File will become required, honestly). But this doesn't mean that Image module is dependent on File module, since you can still use Image module for thumbnail generation without File module.

quicksketch’s picture

Just a note that #553292: Formatter settings lost when saving a field helps significantly with displaying uploaded images properly instead of using the generic file display for new image fields.

yched’s picture

re quicksketch #8:

ImageField and FileField shared too much code to be separate types. If ImageField were to be implemented as a field, all of these functions would need to be reimplemented: (...)

OK - so we agree that the ideal plan would be "class ImageField extends Filefield" ? I really wish we were OO already, but getting there in the first iteration of 'fields in core' was a bit daunting...

Agreed that #6 'alternate model 1' and the snippet you posted in #8 leads to convoluted and fragile code. What about 'alternate model 2' ?

quicksketch’s picture

I'm not sure method 2 is very viable either:

- alternate model 2: could we add 'image_specialization = TRUE / FALSE' as an instance setting on a File field, and move all the arguable 'widget settings' above to be instance settings as well ? #557272: FAPI: Dependency system could be used to present only relevant settings on forms.

In Drupal 6, FileField is extended by a lot of other modules, not all of which are image-specific. It's likely that video and audio modules (among others) are going to extend File in D7 to save the exact same trouble that we'd have with images.

ksenzee’s picture

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,265 @@
+  // Add maximum and minimum resolution settings.
+  $form['max_resolution'] = array(
+    '#type' => 'textfield',

Elsewhere in core we're now using separate width x height fields - #523478: WIDTH x HEIGHT text. That would make sense here too.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,265 @@
+  return theme('image', $element['#item']['filepath'], $element['#item']['data']['alt'], $element['#item']['data']['title']);

$element['#item']['filepath'] should be $element['#item']['uri'].

theme_image() chokes on this line anyway, at least on my dev setup, but that's due to #561708: theme_image() chokes if getimagesize() fails. If anyone else notices they get no output from the formatter, try applying the patch in that issue.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,265 @@
+  l(theme('field_formatter_image', $element), file_create_url($element['#item']['filepath']));

Same here - s/filepath/uri

I also experienced the problem noted in #13. Otherwise it's working fine for me. Re the architecture question, I will offer the opinion that the price we pay for not being OO is occasionally ugly code. If that means we implement alternate model 1 but have some brittle code, but the user experience is better, to me it seems like an okay tradeoff. Just that much more impetus for OO fields in D8. :)

I'm on crack. Are you, too?

jrglasgow’s picture

As ksenzee said in #16

+++ modules/image/image.field.inc 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,265 @@
+  return theme('image', $element['#item']['filepath'], $element['#item']['data']['alt'], $element['#item']['data']['title']);

$element['#item']['filepath'] should be $element['#item']['uri'].

When you fix this the image now shows up on node view.

I also noticed that when you set the image max/min widthxheight the #default_value is not being pre-populated properly.
image.field.inc line 24 needs to be changed from

    '#default_value' => !empty($widget['max_resolution']) ? $widget['max_resolution'] : 0,

to

    '#default_value' => !empty($settings['max_resolution']) ? $settings['max_resolution'] : 0,

and line 34 from

    '#default_value' => !empty($widget['min_resolution']) ? $widget['min_resolution'] : 0,

to

    '#default_value' => !empty($settings['min_resolution']) ? $settings['min_resolution'] : 0,

This fixes all the functionality problems that I am seeing.

Dries’s picture

I also experienced the problem noted in #13. Otherwise it's working fine for me. Re the architecture question, I will offer the opinion that the price we pay for not being OO is occasionally ugly code. If that means we implement alternate model 1 but have some brittle code, but the user experience is better, to me it seems like an okay tradeoff. Just that much more impetus for OO fields in D8. :)

I like how she thinks! I've to agree with that. If we are going OO, it might be better to establish the model and the architecture, even if its a bit ugly. Plus, with that said, it seems to make sense to optimize for the user experience.

quicksketch’s picture

FileSize
15.5 KB

This patch fixes the above feedback including:

- Height x Width fields are now separate and have better validation on them.
- Display of images through the provided formatters is now fixed.
- The default values for min/max size values are prepopulated correctly.

And I also made one other small change where Image fields are not allowed to upload files other than png, jpg, and gif files (since that's all we currently support). There's been some discussion about this over in #269337: Support for more image types (PDF, TIFF, EPS, etc.) about making this list available for other parts of Drupal, which we can switch to if that gets implemented. If not, it'll be easy for a contrib module to modify this restricted list of extensions through hook_form_alter().

cburschka’s picture

Status: Needs review » Needs work

This looks great now, so I'll focus on points of code style.

+++ modules/image/image.module	29 Aug 2009 19:29:53 -0000
@@ -6,6 +6,9 @@
+// Load all Field module hooks for Image.
+require(DRUPAL_ROOT . '/modules/image/image.field.inc');

The safe command to use for code files is require_once. Technically, the chance that this code file will be included elsewhere is almost non-existant, but consistency and certainty is a plus.

Also, it's spelled "require_once FILE" without parantheses everywhere in core.

+++ modules/image/image.module	29 Aug 2009 19:29:53 -0000
@@ -126,6 +129,7 @@
+    // image.module.
     'image_style' => array(

At a glance, I'm not totally sure what this inline comment means (and that's what it should be for ;) ).

Is image.module the file that the theme_image_style function lives in? Saying that in so many words might be clearer.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,327 @@
+  $form['max_resolution'] = array(
+    '#title' => t('Maximum image resolution'),

Sorry for asking something stupid, but are form elements allowed not to set a #type? I think you end up with 'markup' or maybe 'fieldset' by default, but I'm sure specifying it explicitly can't hurt.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,327 @@
+    '#default_value' => $max_resolution['0'],

'0' and 0 in PHP are equivalent array keys. So $max_resolution[0] works at least as well as $max_resolution['0'] and has the advantage of not looking odd.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,327 @@
+    '#theme_wrappers' => array(),

Sorry for another dumb FAPI question, but does an empty #theme_wrappers array() do anything?

1 days to code freeze. Better review yourself.

1 day? Ohcrapohcrap.

arianek’s picture

Status: Needs work » Needs review

Alright super user testing phase 2! Here you go:

1. CRITICAL: changes to the field settings ARE retroactive - ie. if i have it set to allow 3 uploads, then change it to allow 1, any nodes with content uploaded, files 2 and 3 disappear with no warning (and if i return it to 3 they do not reappear)

2. image field inherits filefield weirdness as far as setting the field as required, but providing a default image - the default is not considered as a provided file, so it still prompts you for a value (sort of a user error though, but this should be clarified).

3. just fyi user1 has unlimited file size upload, this is inherited from filefield i think.

4. as USER1 it does not allow me to load below the dimension limits (rightly so), but the error that comes up when i do this appears above the field's fieldgroup, which (since i had set it to allow 3 uploads) is above my position in the screen. so all that happened was the file upload failed and i could not see why until i scrolled up (as i was looking for the error.) http://skitch.com/arianek/b781h/create-article-localhost.drupal7

for AUTH user it actually was different, the same error showed in a different position at the top of the page http://skitch.com/arianek/b78uu/create-article-localhost.drupal7

5. as USER1 it *does* allow me to load images above the dimension limits as (so a little inconsistent since i can load above the upper limit but not below the lower limit set)

6. the error message does not disappear when i attempt a subsequent upload after triggering the below dimensions error as user1, even if it is successful.

7. first image i test uploaded for no apparent reason uploaded turned sideways - i double and triple checked that the file was not originally like that on my copy, but it could just be me, as i could not replicate this.

8. random error http://skitch.com/arianek/b78ug/edit-article-test-localhost.drupal7 when going to edit node (which has no files uploaded, only a default set)

9. not a bug, but seems to me if you hit upload without browsing for a file there should be a "you have not selected a file" type message.

10. can't figure out where the image field's description field appears, should double check it's displaying somewhere?

11. as auth user (not user1) after uploading a file, i jump back up to the top of the page, which is sorta confusing usability-wise.

arianek’s picture

Status: Needs review » Needs work

not sure why it snapped back to needs review, putting back to needs work

cburschka’s picture

1. CRITICAL: changes to the field settings ARE retroactive - ie. if i have it set to allow 3 uploads, then change it to allow 1, any nodes with content uploaded, files 2 and 3 disappear with no warning (and if i return it to 3 they do not reappear)

Wasn't that one by design? Fields generally warn you that if you remove or decrease any of its storage settings, existing records will be lost or truncated...

arianek’s picture

fair but there was no warning shown.

yched’s picture

re @arianek #21

1. that's actually issue #560434: Field UI allows updating fields with data. You shouldn't be allowed to alter field cardinality once there's data in there.

6. That's a generic bug after the new ajax framework, I think there's an issue open already.

arianek’s picture

Status: Needs work » Needs review

so... yched - with those 2 assumptions, the rest is small beans/bug fix territory really, yes? anyone vote this an RTBC in the next day???

rfay’s picture

Status: Needs review » Needs work

In D6 with imagecache and imagefield, the selects on the "display fields" page allow you to select what imagecache preset you want to use for the teaser and the full content.

I don't see the ability to set the "image style", the new term for imagecache preset, with this patch. I assume this is a bug? Or is it something that's moved?

quicksketch’s picture

I'll do at least a basic re-roll with all the other fixes. However there are a number of things that aren't specifically caused by Image field, such as weirdness with the AJAX framework or the Field UI. Thanks for the excellent reviews!

quicksketch’s picture

I don't see the ability to set the "image style", the new term for imagecache preset, with this patch. I assume this is a bug? Or is it something that's moved?

The ability to use an Image Style will be added at some point, but right now we don't have the ability to configure a formatter. In Drupal 6 we just have a reaaaally long list of formatters, but in Drupal 7 we have the API in place to have settings for formatters, but the UI does not yet support it. So we'll probably hold off on the ability to use an image style in formatters until the UI supports these configurable options.

ksenzee’s picture

I take it alternate model 1 is off the table? The "field as widget + formatter" thing still weirds me out a bit. :(

cburschka’s picture

FileSize
15.61 KB

Well, yched withdrew the first alternative in #14 because of the code duplication/inheritance problem.

Which of arianek's user problems need to be fixed for this to be ready, and how?

I see that 1) is a field_ui bug, while 2) and 3) are inherited from filefield and should be fixed separately (3 is about size of file, not image dimensions).

4) is image specific and rather pesky for usability, so we should look at that one. Particularly interesting is the discrepancy in the display between root and auth user. 5) is an inconsistency; min dimensions are enforced but max dimensions aren't.

6) Was identified as an Ajax bug.

7) Sounds very weird. Did the same image upload normally on image hosting sites? Was it resized by Drupal?

8) is a notice-level error for an uninitialized variable.

function theme_field_formatter_image($element) {
  return theme('image', $element['#item']['uri'], $element['#item']['data']['alt'], $element['#item']['data']['title']);
}

The 'data' key has to be either initialized or checked with isset before being used.

9) is a file-field usability issue

10) should indeed be checked.

11) sounds like a Javascript problem which might be file-field specific. It should definitely be fixed (sending an asynchronous request should not reset the page scroll) but possibly not in this patch.

So to summarize, I think we're left with definite fixes needed for 4, 5, 8, further investigation for 7, 10 11.

The patch I've attached below addresses only the code-style I examined in #20. The '#type' => 'markup' declaration was called superfluous by the docs, so I left it out, but the empty #theme_wrappers => array() I tentatively removed, and changed the require() and $dimensions['0'] stuff.

ksenzee’s picture

Well, yched withdrew the first alternative in #14 because of the code duplication/inheritance problem.

After that, in #18, Dries said he liked the idea, and then I heard no more about it. I think the code duplication is less of a problem than the UX weirdness we'll get when image style formatters are added. ("Why can't I display this PDF filefield as a 20x20 thumbnail? It's one of the listed options. Why isn't it working?") I'm not trying to derail the issue. I just want to know if there was some IRC conversation where option #1 was summarily dismissed.

cburschka’s picture

Whoops, I missed Dries' comment in #18. I haven't heard the idea addressed or dismissed anywhere, no; thanks for bringing it up.

/me just wants this to hit in time for code freeze...

quicksketch’s picture

7. first image i test uploaded for no apparent reason uploaded turned sideways - i double and triple checked that the file was not originally like that on my copy, but it could just be me, as i could not replicate this.

In a set of odd circumstances, this might happen if you're resizing an image that has a thumbnail in the EXIF data that is larger than the size you're scaling down to. Sounds like a problem (if we can reproduce it) with our GD implementation.

#theme_wrappers => array() I tentatively removed

This was an intentional line that keeps theme_form_element() from wrapping extra DIVs (which in turn causes fields not to display inline).

"Why can't I display this PDF filefield as a 20x20 thumbnail? It's one of the listed options. Why isn't it working?"

You actually can display PDF filefields as a 20x20 thumbnail if you have ImageMagick. But we're not really enforcing that you use Formatters that make sense that's true.

int’s picture

Issue tags: +Exception code freeze

add tag

int’s picture

Status: Needs work » Needs review
ksenzee’s picture

FileSize
15.22 KB

Here is a reroll with #theme_wrappers => array() replaced.

quicksketch’s picture

Just an update on this patch since we haven't seen any movement in over a week. Last I spoke with ksenzee, she was talking about rerolling so that "Image" would be its own field type rather than just a widget that extends FileField.

The current patch is nearly RTBC (I'm not sure if there are any real issues with it), but it continues to use the same approach used by the D6 filefield/imagefield combo, where "Image" is simply a widget extension for the "File" field type. This approach the disadvantages of being less clear in the UI when it comes to adding a new field, and it is an "impure" implementation since Images store different data than a normal file. It has the advantage in that it requires much less code (hence why this patch is 15K instead of 50K).

I'm supportive of moving Imagefield to its own Field type like ksenzee suggests, but I'm afraid that it is unlikely to happen. Everyone involved (yched, ksenzee, and myself) will not hold up any attempts to finish the current or proposed patch though, though the current patch seems more likely to reach completion in time to meet the Oct 15th deadline.

ksenzee’s picture

What quicksketch said. :) The reroll is coming along. I have a "work on core" day coming up on Wednesday of next week, and I'll use it to finish the reroll. If all goes well I'll have something for review on 10/1. If all doesn't go well, we can go with this patch, which has only minor nitpicks left to resolve. I think the reroll would be a significant UX improvement, which is why I'm really trying to get it done, but the existing patch is fine, and is a million times better than leaving imagefield in contrib.

drewish’s picture

don't know how i missed this patch. subscribing.

NaheemSays’s picture

while the rewrite is happening, I will chime in that I have tested the patch in #37 and it works well. rtbc IMO if that approach is taken instead of the one that is still working on.

(subscribe)

ksenzee’s picture

Status: Needs review » Needs work
FileSize
26.31 KB

Ok, this is totally work in progress, but I'm resisting the urge to be Perfectionist Pat. I think the hard stuff is done:

- Image field is a separate field, with its own widget and formatters.
- Both image and file fields work correctly, including hook_file_references(). (Although boy, could we use some tests for this.)

To do:
- implement hook_field_formatter_info_alter per quicksketch's suggestion in #8.
- Review field vs. instance vs. widget settings.
- Move instance settings around in the form so they make any sense whatsoever.
- Write tests!! I could really use help here. (This needs to happen regardless of which version of the patch goes in.)

Questions:
- Should imagefield be its own module? I ask because it needs to enforce a dependency on file.module now, which before was not the case. The easiest way I can think of to do that is to separate the field stuff into its own module within modules/image.
- Any reason we can't move all the existing field settings to instance settings?

ksenzee’s picture

Status: Needs work » Needs review
FileSize
27.83 KB

Nothing new in this patch but some doxygen, but I'm bumping it to needs review for the testbot. I'll have more time to work on this in the next several days (thanks Acquia!).

ksenzee’s picture

FileSize
37.85 KB

Ok. Now this actually is ready for review.

For reviewers: Essentially what this patch does, in comparison with the almost-RTBC one from #37, is make image its own field type. It moves around field, widget, and instance settings as described by yched in #7, for both the file and image fields. All the field settings but one, and all the widget settings but one, are now instance settings. (See #7 for why on earth we would bother doing this.)

We still need tests and image style formatters, regardless of which patch goes in. I'll work on tests next week. We could probably make adding image style formatters a separate issue after this goes in.

I also still have the question about whether we can keep image.module from depending on file.module, but make imagefield dependent on file.module, without breaking the imagefield stuff out into a separate module. I'd like someone else's opinion on that before I go ahead and create the separate module.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Awesome, thanks for working on this, ksenzee ! I'm not where I can apply and actually tests, here are a few remarks out of visual inspection.

+++ modules/file/file.field.inc
@@ -43,7 +44,7 @@ function file_field_schema($field) {
-      'display' => array(
+      'display_file' => array(

Is this change really needed ? It makes the code a little harder to grasp IMO. 'display' makes the relation between $instance['settings']['display_field'] and $item['display'] a bit clearer (the 'display_field' setting controls the presence of the 'display' data element)

+++ modules/file/file.module
@@ -830,15 +830,17 @@ function file_icon_map($file) {
+ * @param $module
+ *   (optional) A module name on which to filter the list.
  */
-function file_get_field_list($bundle_type = NULL, $field = NULL) {
+function file_get_field_list($bundle_type = NULL, $field = NULL, $module = 'file') {
   // Build the list of fields to be used for retrieval.

Param name $module doesn't seem right, this actually limits on the field type, right ? (also applies to function file_get_file_reference_count)
+ if so, it seems this file_get_field_list() function is not file-or-image specific anymore, and could be helpful as a generic helper function in field.info.inc.

+++ modules/image/image.field.inc
@@ -0,0 +1,521 @@
+function image_field_widget_settings_form($field, $instance) {
+  $widget = $instance['widget'];
+  $defaults = field_info_widget_settings($widget['type']);
+  $settings = array_merge($defaults, $widget['settings']);
+
+  // Use the file widget settings form as a basis.
+  $form = file_field_widget_settings_form($field, $instance);
+
+  return $form;
+}

It looks like the first 3 lines in the function are not needed anymore ?

+++ modules/image/image.field.inc
@@ -0,0 +1,521 @@
+/**
+ * Implement hook_field_create_instance().
+ */
+ /*
+function image_field_create_instance($instance) {
+  // If creating a new image widget, set some matching formatters.
+  if (isset($instance['widget']['type']) && $instance['widget']['type'] == 'file_image') {
+    if (!isset($instance['display'])) {
+      $instance['display']['full']['type'] = 'image';
+      $instance['display']['teaser']['type'] = 'image';
+    }
+    if (!isset($instance['settings']['file_extensions'])) {
+      $instance['settings']['file_extensions'] = 'png jpg gif';
+    }
+    field_update_instance($instance);
+  }
+}

Is this still needed ? Last time I checked, this was required by the 'image as widget' approach.

This review is powered by Dreditor.

ksenzee’s picture

FileSize
39.58 KB

Thanks yched!

Is this change really needed ? It makes the code a little harder to grasp IMO.

Agreed... but now that it's an instance setting, we have to change the name to something. $instance['display'] is a Field API internal, so we can't use that.

Param name $module doesn't seem right, this actually limits on the field type, right ?

Doh. Yes, it's field type. You would think if ($field['type'] != $module) { might have clued me in.

it seems this file_get_field_list() function is not file-or-image specific anymore

I agree, but I wouldn't have known where to put it. :) After your comment, I moved it to field.info.inc, but then I got to thinking that it should be merged into field_info_fields(), so I did that instead.

It looks like the first 3 lines in the function are not needed anymore ?

Nope, good point.

Is this still needed ? Last time I checked, this was required by the 'image as widget' approach.

Oops... I commented it out and then forgot to delete it.

Attached patch addresses the above, plus fixes the file tests that were failing because the display field setting is now a display_file instance setting.

ksenzee’s picture

Status: Needs work » Needs review

fixing status.

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

Status: Needs work » Needs review
FileSize
40.19 KB

Hm. file.test still had a [display] instead of [display_file]. Try this instead, bot.

quicksketch’s picture

This looks like a really great improvement so far. The more I review the patch the better I like it and the approach. So let's keep rolling with this and get it done before our freeze.

Some things that need improvement:
- We should remove settings that make little sense for images and revert back to the nice ImageField/FileField split of D5, including:
- Removing the "description" field for images, especially since description isn't printed out *anywhere* in the default theming (nor does it have a place).
- Removing the "list" field for images, since we don't need to worry about porting upload.module to image fields. This presents a challenge for contrib users upgrading from ImageField, but I don't think it's a substatial problem. Usually for image fields you'll either set display = "Hidden" because you're doing it entirely inline or you're using an ImageCache preset and manually placing the images (such as in a gallery).

Separately we should also make the following changes:
- Move "Image preview style" to a widget setting, since it's not guaranteed that every image upload widget will support showing a thumbnail through Image styles (such as a Flash-based image uploader).
- With the removal of a lot of file-only options, it makes sense to eliminate as many collapsed fieldsets as possible, since there will only be one or two fields left in most of them. Less fieldsets means faster configuration, since most users will open fieldsets anyway to look inside of them. Without token filling up tons of space (so far), there's no need to shove them into fieldsets.

Right now I also found just a straight up bug:
- Enabling title and alt fields doesn't actually enable title or alt fields.

So I'll set this to needs work. I've been working on a reroll of some of these changes, but it's too late to finish them tonight. Hopefully I'll be able to post it before Thursday.

quicksketch’s picture

Status: Needs review » Needs work

Another thing I found this morning:

-function file_field_displayed($item, $field) {
-  if (!empty($field['settings']['display_field'])) {
-    return (bool) $item['display'];
+function file_field_displayed($item, $instance) {
+  if (!empty($instance['settings']['display_field'])) {
+    return (bool) $item['display_file'];
   }
   return TRUE;
 }

The "display_field" option *must* be a field-level setting, not an instance setting. Take the following situation:
- The instance setting is set have "display_field" in one instance
- Another instance as "display_field" disabled.
- Then a listing of all files is shown within Views (from both instances). Now it's impossible to tell which files should be displayed based on their instance settings, since you don't know which instance was used to upload the files.

yched’s picture

re ksensee #47 :

but now that it's an instance setting, we have to change the name to something. $instance['display'] is a Field API internal, so we can't use that.

nope, I'm fine with $instance['settings']['display_field'], it's $item['display_file'] (thus the ''display_file' column in the schema) that makes me tick. I'd vote to keep it as 'display' as currently.
"$instance['settings']['display_field'] controls the presence of the $item['display'] element" works mentally for me.
"$instance['settings']['display_field'] controls the presence of the $item['display_file'] element" doesn't work that well. But that's nitpick, really.

re quicksketch #52: good catch. Apart from stuff that directly affect the db schema, the case of cross-node-type Views listings is the second criteria for field vs instance level settings.

mcrittenden’s picture

Sub.

stuffitt’s picture

Subscribing

ksenzee’s picture

Webchick says quicksketch is out for the next day or so, so I'm going ahead with a reroll. Will have something in a few hours.

sun’s picture

Priority: Normal » Critical
Issue tags: +Fields in Core

Magic subscribe

ksenzee’s picture

Status: Needs work » Needs review
FileSize
36.55 KB

This patch fixes #52 and #53. (I wasn't aware of the cross-node-type Views listing criteria for field settings -- thanks. Do we have docs on that?) I also added a test for hook_file_references. More to come hopefully.

Starting work on quicksketch's points from #51 now, but setting to needs review so the testbot can have a whack at it.

ksenzee’s picture

FileSize
38.64 KB

I think I addressed most of #51:
- removed the "description" field for images
- removed the "list" field for images
- moved "Image preview style" to a widget setting

I didn't really address the order of any of the settings on the admin form. Input welcome. Also, I can't reproduce the bug with enabling title and alt fields.

I think this is in pretty decent shape at this point. It could really stand some banging around in the UI, since we're still short on tests.

webchick’s picture

Yoink! Sitting down with this one now.

webchick’s picture

I will say that on the whole I totally prefer this approach. It's always confused me when

For whatever reason, the "Image" field didn't show up in the field selection at first. I tried clearing caches about 50 times, and finally disabling/re-enabling the image module seemed to fix it. Weird.

Anyway!

UI-wise:
- "alt" and "title" seem to be working for me fine, as well. However, I get this when editing a node with an image that does not have alt/title filled in, even though the fields are checked off:

     *  Notice: Undefined index: alt in theme_field_formatter_image() (line 463 of /Users/webchick/Sites/core/modules/image/image.field.inc).
    * Notice: Undefined index: title in theme_field_formatter_image() (line 463 of /Users/webchick/Sites/core/modules/image/image.field.inc).

- Let's get "Upload destination" out of the collapsed fieldset on the field settings page. I think this is supposed to have a big list of available tokens, but that doesn't currently exist. Once it does, the token UI can take care of whatever form_altering is necessary.

- We seem to have lost the ability under admin/structure/types/manage/XXX/display to show an image style as the formatter. It would be terribly tragic for us to ship Drupal 7 with imagefield and imagecache, but NOT the ability to use them together. Oh, I see there's a TODO about this. ;) In #29 Nate mentions that we're holding off on this for the ability to configure formatters through the UI. However, 1.5 months later, we still do not have such an ability, and I don't like the idea of shipping D7 without the ability to use image styles, even if the UI is sub-optimal. It's possible this configurable formatter stuff could be added during the UI freeze, but I'll need to discuss with Dries.

- Nate also has a patch that re-weights things on the settings page to generally make it make more sense. I see him on IRC now, so hopefully this will be forthcoming soon! If not, this is definitely something we can fix it during the UX polish phase.

I'm going to stop reviewing this now because Nate's back. I'll peek in again tonight.

quicksketch’s picture

Status: Needs review » Needs work

Well like all patches in the queue, we now need to reroll after #572618: All theme functions should take a single argument to make preprocess sane and meaningful. Hopefully I can post an update before hoping on the plane, otherwise I'll send in an update tonight.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
40.87 KB

I'm sitting on a plane but trying to get this up as quick as possible. Here's a reroll with a few changes:

- Now works properly with private files.
- Reroll for the theme changes.
- Adds back in formatters for image styles.
- Reorganizes field widget/instance settings forms.
- Several fixes to description field storage.

This patch is not based on ksenzee's from #60 since I was in the middle of a lot of changes when she did the reroll, but it has all the same changes as she mentioned (removed description and list from image field types).

Status: Needs review » Needs work

The last submitted patch failed testing.

arianek’s picture

hey - i told ksenzee i'd end user test this again before the end of the weekend, but i can't really tell which patch i should be using - going to check back later this afternoon and see if i can catch someone (quicksketch?) online. will be out tonight, but around sunday morning/afternoon for a bit as well so will keep checking in till i snag someone who knows what's up! or if anyone can post here and clear things up, i can follow up tomorrow. thanks!

quicksketch’s picture

Status: Needs work » Needs review
FileSize
40.91 KB

An updated reroll that should apply and actually work. arianek, right now I think this is the only patch that will even slightly work, since the changes to the theme system broke just about every patch in the core queue.

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

I did the reroll well after [572618] went in, so my patch would work fine, but it makes more sense to use quicksketch's patch since he's added the image style formatters back in. Otherwise I'm guessing the patches are very similar. I only jumped in with mine because I hadn't heard from him and I wanted to make sure this didn't die of neglect.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
41.03 KB

A few more changes:

- Patch from the Drupal root (dur).
- Should pass tests.
- Adds necessary RTL styling for image widgets.

Status: Needs review » Needs work

The last submitted patch failed testing.

arianek’s picture

gotcha - about to run out to (canadian) thanksgiving dinner so i'll have to get to it tomorrow, but will do a thorough end user testing round on quicksketch's patch and see what i can dig up. ;-)

quicksketch’s picture

Status: Needs work » Needs review
FileSize
42.22 KB

I finally managed to track down a problem with the tests that were causing them to fail (the testing instance needed a cache reset). This version should hopefully pass test bot.

quicksketch’s picture

FileSize
41.99 KB

The last patch included a segment of debugging code that needed to be removed.

webchick’s picture

+++ modules/image/image.module	12 Oct 2009 02:59:57 -0000
@@ -6,6 +6,9 @@
+// Load all Field module hooks for Image.
+require_once DRUPAL_ROOT . '/modules/image/image.field.inc';

We should probably use module_load_include() here, for consistency with the rest of core.

EDIT: Actually, Nate pointed out that module_load_include() requires a database connection, and so doing this would mean we could never use image module in an install profile. Sad panda.

+++ modules/image/image.module	12 Oct 2009 02:59:57 -0000
@@ -600,17 +642,20 @@
+ * @param $uri
+ *   The path to the image. If referencing an image in an existing
+ *   schema, that URI should be used, such as "public://example.png".
+ *   A normal file path may also be used to create derivatives of
+ *   images that are not within the files directory or any schema,
+ *   such as "modules/image/example.png".

This documentation needs a bit of work. I had to read it 3 times and I still don't know what I'm supposed to pass in here and when.

+++ modules/file/file.field.inc	12 Oct 2009 02:59:57 -0000
@@ -136,26 +133,28 @@
+    '#description' => t('Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. If left empty the file sizes will be limited only by PHP\'s maximum post and file upload sizes (current limit <strong>%limit</strong>).', array('%limit' => format_size(file_upload_max_size()))),

This could be written without the \' so let's do so.

EDIT: This is just moving around from a different spot. Can fix in a follow-up.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,481 @@
+/**
+ * Implement hook_field_load().
+ */
+function image_field_load($obj_type, $objects, $field, $instances, $langcode, &$items, $age) {
+  file_field_load($obj_type, $objects, $field, $instances, $langcode, $items, $age);
+}

I know there was concern about code duplication of this kind, but I actually kind of like having this wrapper around filefield. It means if we need to we can also insert some custom image-only logic.

+++ modules/image/image.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,481 @@
+/**
+ * Theme the display of the image field widget.
+ */
+function theme_image_widget($variables) {
...
+/**
+ * Theme function for 'image' file field formatter.
+ */
+function theme_field_formatter_image($variables) {
...
+/**
+ * Theme function for 'image_link_content' file field formatter.
+ */
+function theme_field_formatter_image_link_content($variables) {
...
+/**
+ * Theme function for 'image_link_file' file field formatter.
+ */
+function theme_field_formatter_image_link_file($variables) {

Let's get some documentation for the $variables parameter for these theme functions.

They should also be in @ingroup themeable.

EDIT: These functions are documented consistently with other formatters (e.g. http://api.drupal.org/api/function/theme_field_formatter_number_unformat...), so fixing this would be in a follow-up.

However! The docs are incorrect since image is not part of filefield anymore.

--

And goll dang it, that's all I can find to nit-pick about the code. :\ Onward with the UI review!

This review is powered by Dreditor.

quicksketch’s picture

FileSize
41.72 KB

This documentation needs a bit of work. I had to read it 3 times and I still don't know what I'm supposed to pass in here and when.

I simplified this to simply:

+ * @param $uri
+ *   The URI or path to the image.
However! The docs are incorrect since image is not part of filefield anymore.

These now properly say what field type they belong to ('image', not 'file'):

+ * Theme function for 'image_link_content' image field formatter.

All the rest of webchick's nitpicks were exceptions or rationalized, so this should meet all the code style requests.

webchick’s picture

When clicking around, I noticed I get these notices from a node with a default image:

    *  Notice: Undefined index: alt in theme_field_formatter_image() (line 451 of /Users/webchick/Sites/core/modules/image/image.field.inc).
    * Notice: Undefined index: title in theme_field_formatter_image() (line 452 of /Users/webchick/Sites/core/modules/image/image.field.inc).
quicksketch’s picture

FileSize
43.29 KB

When clicking around, I noticed I get these notices from a node with a default image:

This situation was caused by the following configuration:
1) A default file was uploaded in the field configuration
2) But no image was uploaded when creating the node
3) Then view the node (and reload to see the error)

To fix this problem, I think we should solve another minor nitpick: It doesn't make sense to have a "default file" for anything but an image. At least it doesn't make sense in the context of a general file upload widget. So this version of the patch removes "default_file" from file.field.inc and adds "default_image" specifically to image.field.inc (and fixes the notices).

webchick’s picture

Status: Needs review » Fixed

All righty then. COMMITTED TO HEAD!!! :D Sorry, Ariane. But we knew you were busy, and hopefully now you banging on it and finding all kinds of horrible bugs will be even easier! :)

Only change was a CHANGELOG.txt entry for ImageField, and also one for FileField since we discovered it was missing. :)

I don't actually think there is anything here to document, so I'm marking this baby fixed!

Great push, everyone!!!

webchick’s picture

Oh. Also wanted to ping everyone with this critical follow-up: #601966: Tests for image field

jhedstrom’s picture

I added #602752: Image field depends on the file module as it is currently possible to disable the file module while image is still enabled.

arianek’s picture

Great work you guys and gals, if the newbs don't keep me too busy at the code sprint on the weekend, I will ping webchick and see what my time can be useful for, maybe I will do some testing on this yet if it's still worthwhile. Anyway *highfives*!

mattyoung’s picture

.

dopry’s picture

Status: Fixed » Needs review

I know late to the party.... however you completely missed the reason for merging filefield and imagefield... Reducing code duplication was just a boon. The real reason for integrating the two modules and their code base was to have a unified user interface for managing files, regardless of file type.

This is a horrid regression of two years of design and input from the community of people developing media management tools for drupal. It's a massive step backwards in usability. Now I feel I need to have seperate fields to handle images, video, audio, pdf files etc. I'm very sad, baby jesus is crying for you lack of foresite, and the concept of an abstract reusable representation of files no long has a user interface.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

How about instead of slinging around insults, you provide a code example of what you're trying to do and can't given the way this was implemented? I still fundamentally believe that the approach that was committed is better for two reasons:

1. Users choose between File or Image as the choice of field, rather than having to infer that images, videos, etc. etc. are all of a type of "File". That makes sense from a developer perspective, but not from a user perspective.

2. Moreso though, this implementation direction cleaned up a lot of stuff in FileField module. "Alt" and "Title" are now native fields, and not crammed into a ghetto serialized data column. This means that if you want to display a view of images and their titles (or videos and their height/width or encoding...), you can. We moved the concept of a "default" if nothing's uploaded yet to imagefield, where it makes sense, rather than filefield, where it doesn't.

It was my understanding that the 'managed field' element was going to encapsulate all of this in a central place, but I've been told on IRC that this isn't the same thing, because it applies only to the Form API, and not the Field API. That sounds fair enough. So what would Field API need in order to support the kinds of things you're trying to do?

mcrittenden’s picture

Status: Needs work » Fixed

@dopry a bit too late to be making significant changes, not that code freeze/slush is well past. If you'd like, create a new issue against D8 about it.

dopry’s picture

webchick mentioned this should be written somewhere... I have the unfortunate position of thinking this vision was well understood, since amongst the people working on media module it is...

So vision first, and this vision is based on one on one feed back from developers, site administrators, end users, and my issue queue over several years.

There should be one field for handling all types of files. One simple interface to upload an image, video, pdf, mp3. The original vision was just the single upload field we're all used to... When the file is uploaded any embedded data, such as iptc tags, id3 tags, etc are sniffed by a module via the file layer and stored by media type specific modules... Then the file is displayed in the form in a row appropriate to it's file type with the appropriate 'meta' fields..

So given an concrete example...

I spend my day out shooting with my camera.. I grab a few videos and images I like at the end of the day that I want to upload to my website. I go create a new gallery node on my website... (The gallery node type has multivalue file field.) I enter my title 'Fun on 10/23 halloween shopping!'. I go click on the browse button, select an image. I hit the upload button... I see a new row in my filefield form with the file I just uploaded with alt an title fields contain the IPTC headline tag from image. I got to upload another file, it's a video, I upload it in the same upload field I uploaded my image into. It shows up in the next row of my table with it's media type and duration. I repeat with several other images and video files.. I save my node...
Looking at my node page I see a selection of thumbnails since I've configured the filefield to display images and video as thumbnails 120 x 120 thumbnails...

I hope that suffices for vision...

What you get when you switch to having an imagefield and a filefield.... Twice as much screen real estate consumed, frustration when I try to upload the wrong file type to the wrong field... required to support multiple field type modules and user interfaces for files...

Now the technological pieces that needed to come together for this to happen....
1) File hooks (Done)
2) Unify FileField and ImageField's underlying field storage mechanism and abstract the user interface parts into the widgets. (WAS done in D6)
3) Make field formatters more like widgets with settings and instances (Thought this was in the CCK or Field pipeline, but never appeared)
4) Unified file uploads in FormAPI (Done)
5) Make a supersmart widget for handling all the field types choosing the appropriate sub widget for filefield (Media Sprinters are working on this)
6) Filefields needed a place to store random instance item specific data.. (I added a serialized 'data' field in D6 as a part of the field merge in D6)

Then comes fancy stuff like remote uri references and file reuse which is also on the roadmap for media module, but baby steps first...

So the outcome in splitting the fields...

Fortunately you haven't blocked the work the Media team is still planning... We basically now have to fork filefield (the descent back into how do I handle files with so many options hell) to get back to where I was at in D6, or do some sort of crazy scope creepy mess of schema_alter and form_alter element_info_alter to make filefield behave as it used to...

dopry’s picture

Status: Fixed » Needs work

webchick mentioned this should be written somewhere... I have the unfortunate position of thinking this vision was well understood, since amongst the people working on media module it is...

So vision first, and this vision is based on one on one feed back from developers, site administrators, end users, and my issue queue over several years.

There should be one field for handling all types of files. One simple interface to upload an image, video, pdf, mp3. The original vision was just the single upload field we're all used to... When the file is uploaded any embedded data, such as iptc tags, id3 tags, etc are sniffed by a module via the file layer and stored by media type specific modules... Then the file is displayed in the form in a row appropriate to it's file type with the appropriate 'meta' fields..

So given an concrete example...

I spend my day out shooting with my camera.. I grab a few videos and images I like at the end of the day that I want to upload to my website. I go create a new gallery node on my website... (The gallery node type has multivalue file field.) I enter my title 'Fun on 10/23 halloween shopping!'. I go click on the browse button, select an image. I hit the upload button... I see a new row in my filefield form with the file I just uploaded with alt and title fields containing the IPTC headline tag from image. I go to upload another file, it's a video, I upload it in the same upload field I uploaded my image into. It shows up in the next row of my table with it's media type and duration. I repeat with several other images and video files.. I save my node...
Looking at my node page I see a selection of thumbnails since I've configured the filefield to display images and video as thumbnails 120 x 120 thumbnails...

I hope that suffices for vision...

What you get when you switch to having an imagefield and a filefield.... Twice as much screen real estate consumed, frustration when I try to upload the wrong file type to the wrong field... required to support multiple field type modules and user interfaces for files... Lets not even go into trying to display multiple file types in a consolidated list...

Now the technological pieces that needed to come together for this to happen....
1) File hooks (Done)
2) Unify FileField and ImageField's underlying field storage mechanism and abstract the user interface parts into the widgets. (WAS done in D6)
3) Make field formatters more like widgets with settings and instances (Thought this was in the CCK or Field pipeline, but never appeared)
4) Unified file uploads in FormAPI (Done)
5) Make a supersmart widget for handling all the field types choosing the appropriate sub widget for filefield (Media Sprinters are working on this)
6) Filefields needed a place to store random instance item specific data.. (I added a serialized 'data' field in D6 as a part of the field merge in D6)

Then comes fancy stuff like remote uri references and file reuse which is also on the roadmap for media module, but baby steps first...

So the outcome in splitting the fields...

Fortunately you haven't blocked the work the Media team is still planning... We basically now have to fork filefield (the descent back into how do I handle files with so many options hell) to get back to where I was at in D6, or do some sort of crazy scope creepy mess of schema_alter and form_alter element_info_alter to make filefield behave as it used to...

quicksketch’s picture

I don't think that having separate field types hinders our ability to make a generic file manager. Using field-API, we can easily check which fields store "fid" as their primary data. While this isn't an absolute guarantee that the field references the files table, right now it seems like it would be good enough to figure out which fields are "file-based". It's not perfect, but I don't think it has worse repercussions than arbitrary, undocumented data being shoved into a "data" column that isn't searchable, filterable, or sortable.

I think the whole problem of file-ambiguity could easily be avoided just by making some kind of central place for asking, "which fields are file-based"? Media module very well could be this place, making a hook for asking this question. Since it's pretty clear that Media module won't be part of Drupal 7 core, we can figure out these problems in contrib, Media filling in the blanks for the time being until any required APIs can be moved into core in D8.

pwolanin’s picture

@quicksketch - having multiple file field types absolutely hinders us doing this in contrib - right now the 2 (file and images) have different schemas and neither of them can store the any additional data about the uploaded file.

@webchick - is there a real use case for displaying the alt text in a View? Views could unserialize a field if really needed.

yched’s picture

The smart widget described in #89 sounds nice, but I'm skeptical about display. This means a single formatter being smart and powerful and configurable enough to be able to display an image, or a PDF, or an mp3 file, or a video, with all the corresponding options and flavors ? Sounds like a mammoth...

Re "3) Make field formatters more like widgets with settings and instances (Thought this was in the CCK or Field pipeline, but never appeared)" :
Not sure what you mean by "with instances", but Field API does have formatters settings. It's "just" that no good UI pattern has been agreed so far.

yched’s picture

re pwolanin #91: I think the use case is more about adding filters and sorts on the alt or title.
+ I'm possibly naïve, but the idea that meta-data are better off in a serialized array sounds a bit backwards...

dopry’s picture

There are two concrete issues one technical, one UX/social.

Technical: The serialized data column on filefield provided a place to store field item specific data such as alt, title, caption, etc. This is a fundamental requirement for building a 'smart' widget on top of filefield that can handle multiple types of files.

UX/Social: Splitting imagefield and filefield introduces two ways to handle file uploads via fields. The field + widget vs 2 fields impact 2 classes of users, content type creators and content creators/editors. I feel the biggest impact is on the experience for the content type creators who need to setup either a complex field or multiple fields, there are arguments both ways for which is better. For the content creator/editor just about any UX issues can be addressed at the widget layer.

aaron’s picture

The current implementation sets a poor example, and keeps things in their current tangled mess; we'll continue to end up with 20 different file type fields (dealing with audio, video, youtube, pdf, etc), all implementing their own schema. I'm not sold on serializing the metadata either, but I definitely would rather just have one table handling the basic stuff. Then each module building on top of that can just deal with display formatters, which in the end is all that really needs to happen. For the metadata (obviously not in core for now), I'd personally rather see that ultimately pulled out of the field entirely, and be attached to the files (as File Metadata is setting out to do).

webchick’s picture

I'm not sold on serializing the metadata either, but I definitely would rather just have one table handling the basic stuff.

Pardon my ignorance, but isn't that one table the 'files' table?

We promoted files to first-class objects in D7, and they now have hooks so modules such as Media can react whenever they're saved, loaded, copied, etc. Why are these not good enough?

dopry’s picture

@ #92, Maybe applied to instances is what I meant top say, or unique per field instance... I'll look into the formatter settings, but most likely I'll make up my own approach similar to imagecache, until some 'genius' can at least come up with something that works. What happened to a release early, release often philosophy? Put something out that works and refine it to something good.. You won't even have a point of reference for whats good until you've had something in place to discover whats wrong with it or how people want to use it...

@ #93, maybe you want to search and sort on alt and title, I personally haven't ever recieved that as a request from an end user... I consider it an edge case. I had people ask me to make them visible via views... That can be done by having you handler unserialize the data field and return the appropriate array member.

dopry’s picture

@webchick #96, He's talking about 'alt', 'title', 'description', in the field table... Also keep in mind that we're talking about 'Field Item' specfic metadata and not 'File Metadata'. File meta data such as format, encoding, dimensions, etc should not be stored with the field item at all...

joachim’s picture

> 1. Users choose between File or Image as the choice of field, rather than having to infer that images, videos, etc. etc. are all of a type of "File". That makes sense from a developer perspective, but not from a user perspective.

To take just this on point of webchick's -- that could have been fixed by reversing the UI: choose your widget first, THEN the storage.
This is something quicksketch remarked on in Paris.

pwolanin’s picture

Fundamentally images are files - and file field can hold image fields as well as any other file type. What's in core now is really problematic in my opinion since we now have 2 slightly different field types to store what's fundamentally the same data (a file reference, which is at least the fid and uri).

this is like saying that forum module should not use taxonomy as the storage because it wants to present taxonomy a little differently.

SeanBannister’s picture

Yeah I didn't see this problem originally but it's really annoying, If a user wants to upload Image,Video,Audio,File it'd be great to have one field.

webchick’s picture

I'd like to see quicksketch and ksenzee weigh in here, since they did the bulk of the work on this patch.

quicksketch’s picture

Well I personally see the separation as a good thing, and I largely opposed it because I didn't want to do additional work to re-separate the two. Honestly I can say it's problematic in a lot of ways to have these as the same field. Most notably, it's difficult to know which data is available on which types. For example, in D6 we never did anything with the "description" or "list" fields within imagefield. You could turn them on, use them, but then... nothing. Neither of these fields did ANYTHING when used with ImageField. So we were causing a lot of user confusion ("where does the 'description' field show up", "why doesn't 'list' do anything", etc.) These fields could have been made to do something of course, but they didn't make a lot of sense applying to images at all.

Dopry and pwolanin raise valid advantages of having a "single field" of course also, even though it goes against the "field" concept of CCK (field == database storage), where if a field needs to store different data, it's a different field.

In the end, all I can say is, I don't care how we store this data. Like I said far above in the issue, the important thing was to get this functionality in core. I just wanted a way to fricking upload a picture, insert some details and display it, I'll leave it to the purists to fight it out over the "important" details. I think either way we go, contrib will be just fine. There's no reason that Media module couldn't create it's own generic file type that stores the data serialized, you just wouldn't be able to convert from file.module to media.module as easily.

joachim’s picture

> Neither of these fields did ANYTHING when used with ImageField. So we were causing a lot of user confusion ("where does the 'description' field show up", "why doesn't 'list' do anything", etc.) These fields could have been made to do something of course, but they didn't make a lot of sense applying to images at all.

To me, this says:
- base file field, abstract
-- 'uploaded file' field -- for stuff like PDFs
-- image
-- video
-- etc

yched’s picture

Pushing what I said in #92, one mammoth formatter type able to display images, audios, videos, swfs... with all nifty variants and options kind of calls for a notion of sub-formatter which is still to invent.

In fact, this reminds me of the D5 Asset module, which was a great idea but also a true config hell, and which in several occasions I ended up configuring with separate fields per media type...

Now, I'm not part of the media group and aside from a couple awesome demos (which IIRC focused more on the 'file upload / selection' part than on the 'display' part), I have not been following its work closely. I have no doubt that we've learned since Asset and that what you guys have in the box will be much better, but at this point in time, that does sound like contrib ?

joachim’s picture

I meant from the point of view of data storage and field API stuff.
Can we somehow have a common filefield which takes care of the essentials, and then other field types sit on top of that and tell fieldAPI they need to store extra things (mimetype, audio encoding details, etc)?

A clever formatter that knows how to display a file is a long way off, agreed. I've yet to see a site that requires it, as most designs lump audio in one place, images in another, and so on.

pwolanin’s picture

@joachim - the proposal here was essentially that the file field needs to be that base type, and that we add a serialized data column to handle small amounts of added data (e.g. the alt tag text) that a formatter might want to use.

For images, it seems to me that the file field description data could readily be used as the image title? The image module could expose a widget that makes this happen without needing to rename the column in the DB.

cridenour’s picture

@108:

I think that's what the original module was, to an extent. I mean, field names aside, the point here is usability.

I, as a user, should not have to select File, then Image. As a user that doesn't click for me.

I'm not pushing either way on the coding side, but usability is the reasoning behind this so we either need to select Widgets first (which I'm not sure I agree with) or stick with this current implementation.

aaron’s picture

is it too late to add weights to the widgets? or sorting alphabetically even?

aaron’s picture

doh! too early in the morning... weights, obviously. not abc... :P

pwolanin’s picture

@cridenour - have you tried using the current? you select a field type and then a widget. So only in the degenerate case where a field has only one widget does the current save you making the second selection.

dopry’s picture

the serialized data field needs to be restored to filefield.. That is all... It didn't need to be removed to split the module. The only impact removing the serialized data field had, was cutting of the vector of development I've been working on for years. As for the arguments about the admin ui workflow... It's UI, not data layer... goto form alter if the form doesn't look good or has confusing oprtions to you, don't rearchitect your data layer. Make the presentation layer smarter. You guys fail architecture 101.

quicksketch’s picture

the serialized data field needs to be restored to filefield.. That is all...

I don't think file.module is limiting the abilities Media, or needs to be forked for any reason. Media module can easily provide the "universal" file field with the serialized data column and build off of file.module, just like image.module is doing now. It's essentially the same thing, only it will have a different schema (just like image.module) that includes the serialized "data" column. Then we don't have to compromise in the short-term with presenting UI and Formatter options that don't make sense, and Media module can continue with the intelligent formatters that output according to the type of data uploaded and have a place to store arbitrary data for any kind of file.

pwolanin’s picture

@quicksketch - I think there is some mis-communication - the data column was removed from field field. The suggestion is to at least add that column and the handling for it back, and ideally to merge image field back too.

quicksketch’s picture

The suggestion is to at least add that column and the handling for it back, and ideally to merge image field back too.

Yes I understand that's the suggestion, I was offering an alternative that would only require changes to Media module (media module provides it's own field with it's own serialized data column). Then the user just won't use either the normal Image field or the File Field and you can store all the extra stuff you want.

The current solution works better for core than the single "file" field, since we don't have to present the various Image styles as formatters for file upload fields where users are going to be uploading Word files. What I'd prefer is that core stay non-confusing out of the box, and Media module can make it's own field (extending the 'managed_file' widget just like image.module is doing) and use that for it's universal file handling.

pwolanin’s picture

Well, I find it frustrating that we are designing the API and schema to accommodate a user's first 15 minutes of using Drupal.

I really think the gain in simplicity is minimal if the user has in every case to configure the allowed file extentions and formatters.

aaron’s picture

Media module can easily provide the "universal" file field

I thought that's what File field is supposed to do?

webchick’s picture

Status: Needs work » Fixed

No, let's be clear here. We are designing the schema based on good schema design. Serialized column information is something that we've desperately been trying to get away from in the past several years (though I see users.data lives to see another release... sigh...). While there are workarounds for querying this data such as writing Views handlers, in general it's an absolute nightmare to work with. Have you tried to create a list of users sorted by their birthdate profile fields recently, for example? :P

Proponents of the old-style serialized data column will claim that no one is ever going to need to sort on the stuff in the data column, but I am not sure how you can possibly know this for sure, particularly given that HTML 5 is coming up and there may be all sorts of new types of media that crop up over the next 3-4 years (the lifecycle of Drupal 7). The thought of hard-coding these assumptions into core when this magical formatter that can handle anything has yet to materialize strikes me as dangerous.

It's true that we initially started down the path of splitting these two fields this because of the user experience benefits, but during implementation, suddenly lots of stuff started falling into place and making a lot more sense. There was a lot of coupling of ImageField stuff in FileField (for example, providing a 'default' file if none is uploaded yet -- what? "CHANGEME.pdf"? :P) which we were able to separate. This separation also cleans up totally non-sensical choices like an Image style formatter on a word document.

Proponents of the serialized data column also are poo-pooing the user experience implications here, but since Nate maintains ImageField module and deals with its support requests, I'm inclined to listen to him when he says that users find things confusing. I know for a long time, each time I go to add an image field, I'd end up going back to admin/build/modules to turn it on, then I slap myself and realize it's under "File." Also, I would argue that *most* times when you're setting up your content types, you honestly do just want a place to stick images or documents, so this is a perfect use case for Drupal core to support.

If/When Media module matures in contrib to something that can take any kind of media and handle it in the most optimal way in all cases, we can of course revisit this and say "Oh, silly us. What were we thinking back in Drupal 7?" But for now, the path outlined by quicksketch at #114 seems the most sensible to me.

Marking this back to fixed.

rjkuyvenhoven’s picture

You guys seem to be talking past each other. Rather than putting your foot down and saying this is fixed you really ought to actually work this out and get it right. Even if you can't get the right solution into Drupal 7 at least you'll be on the right path. And if you are lucky you can get it in Drupal 7 and we'll all be better off. This ain't fixed, don't pretend it is.

quicksketch’s picture

rjkuyvenhoven, the conflict here is that some people think this is already the "right" solution, others think it is not.

The current solution works better for anyone using core's out-of-box features and prevents options that don't make sense (such as a "default file" for file fields) or options that simply won't work at all (such as Image style formatters on Word documents).

The folks developing Media module are saying that they will not be able to use the core file.module's database storage for their purposes, and want this patch rolled back (or modified) because it prevents them from using the default storage for Media. However, rolling back this patch is not an option, since we'll loose the ability to have image fields entirely and then we'd likely debate the whole issue again for the next month and end up not have image fields at all.

If we want to make changes to the code, we should open a new issue. It's clear this patch is not going to get rolled back for the danger of everybody loosing and not nothing getting done.

Status: Fixed » Closed (fixed)
Issue tags: -Favorite-of-Dries, -Fields in Core, -ImageInCore, -Exception code freeze

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