This patch adds drupal_alter() to all places where hook_widget_settings() and hook_field_settings() are called. It also adds a small amount of consistency improvement in the calling of these hooks, always casting the result to an array. Currently these results are sometimes caste and sometimes not, but now it's required because drupal_alter() will want an array and it doesn't work well passing a NULL value by reference.

This patch is necessary to try adequately allow widgets and fields to be extended, rather than replaced. Right now I'm starting to run into problems with all the various additions that are being added to FileField and ImageField. Each time a module wants to extend these fields, they make another widget that builds on one of the others. The problem with this is that the user must choose what features they want by which widget they use. For example, you might have ImageField Crop and IMCE Image installed (even though it doesn't currently support ImageField, just an example). You have the option of using the "Image with cropping" widget, or the "Image with IMCE" widget. But you can't have both, because each is a separate widget and there's no way to extend an existing widget.

Some of the many examples of the ways that people want to extend the File and Image Widgets:

  • The current built-in support: upload an image or file
  • Auto-complete textfield to reference an existing file
  • IMCE or a file browser to reference an existing file
  • Enter a URL to download a remote file
  • Crop an image after upload
  • Rotate an image after upload
  • View/edit EXIF tags after uploading an image
  • Improve the FileField preview to allow sampling of video or audio files
  • Flash-based uploading of files
  • Zip and gzip support for bulk uploading of files

Rather sadly, you can do almost any of these things already, but only only get to choose one of them based on the widget you select. Oddly enough, extending the widget when displayed is already possible by using hook_elements(), you can add extra #process functions to any form element. However the only trouble (that this patch solves) is that you can't extend the values saved into the $field or $field['widget'] arrays.

CommentFileSizeAuthor
#38 cck_wordcount.tgz2.29 KBbatje
#29 content_field_settings_alter.patch10.65 KBquicksketch
content_field_settings_alter.patch10.73 KBquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Needs review

Heh, needs review. AFAIK, this patch is fine.

scroogie’s picture

scroogie CreditAttribution: scroogie commented

Unfortunately I don't know enough about the CCK internals to review this, but I'm highly supportive of the functionality behind it :)

momper’s picture

momper CreditAttribution: momper commented

+1 support for this

thePanz’s picture

thePanz CreditAttribution: thePanz commented

@quicksketch: this feature can be the response to #434446: Move Default Image code to ImageField module :)

+1 for this feature!

yched’s picture

yched CreditAttribution: yched commented

quicksketch : looks like filefield_sources found a workaround for at least a subset of the above. Is this still needed ?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

FileField Sources still doesn't have any configuration options, but this will be good to add them in the future. Right now I'm considering making a separate settings page at admin/settings/filefield_sources, but of course it'd be optimal to actually configure the sources you want to use on a per-field basis, rather than having a whole separate configuration page.

So yes, this is definitely still necessary, not only for FileField but other modules that want to extend the functionality of fields without creating an entirely new widget.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Per the last line of the original issue,

However the only trouble (that this patch solves) is that you can't extend the values saved into the $field or $field['widget'] arrays.

Basically, this means in order to configure FileField Sources, we'd need to make a separate admin/settings/filefield_sources settings page instead of configuring on a per-field basis (and saving the settings with the widget instance). A little more explanation here #436174: Configuration Options.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I think this is an interesting idea that could be used in lots of ways. I'm trying to think of what it could potentially break. I don't know that any of these are actual problems, these are just some of the things to look at:

- The affect on fields used in views, especially views that are imported/exported
- The effect on content copy, especially if the same modules aren't available for both the import and the export
- The effect on modules saved with drupal_execute or node_save.

I don't have time to do anything with this immediately, just getting my thoughts written down...

markus_petrux’s picture

markus_petrux CreditAttribution: markus_petrux commented

Here's another use case... I need to extend the features of noderef fields for a custom project, and it would be extremely cool if this patch was in.

These hooks open the door to a lot possibilities, so I'm in the land of +1 :)

kenorb’s picture

kenorb CreditAttribution: kenorb commented

The same problem. #470142: Adding custom settings to the field (I done that in form_alter)

Anyone reviewed this patch already?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I did finally manage to add some settings to a FileField via hook_form_alter() and variable_get/set() for FileField Insert, but it is ugly ugly ugly. This would still be a fantastic improvement, patch still applies. Kenorb, patch is ready to go and fully functional as far as I know. A review would be very much appreciated!

KarenS’s picture

KarenS CreditAttribution: KarenS commented

It would be good both to test it in some 'normal' settings and then to see if any of the issues in #8 seem to be problems.

Trunkhorn’s picture

Trunkhorn CreditAttribution: Trunkhorn commented

subscribe!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

The Video Upload module needs this functionality in order to be compatible with FileField 3.x, as it must add a few additional database columns.

My immediate assessment of the issues raised in #8 is that it is the responsibility of the caller module not to break those, rather than CCK which would only call drupal_alter().

NikLP’s picture

NikLP CreditAttribution: NikLP commented

Subscribing. For sciences.

kenorb’s picture

kenorb CreditAttribution: kenorb commented

I'm not sure, but it's not something that exist already?
http://drupal.org/node/144132#form-id-alter

Example modules:
nodeaccess_userreference
nodeaccess_nodereference

Or it's something different?
On #470142: Adding custom settings to the field I just wanted to add custom settings for specified field.
I think I've found that implementation of hook_form-FORM-ID_alter() solve that.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

kenorb, form_altering is only a very small part of this patch. While you can work-around the form by using form_alter(), there's currently no way to modify what columns are available within a field's database tables. It's also very difficult (but maybe not impossible) to change the data that's saved into content_node_field or content_node_field_instance tables.

As I said in #11, I ended up using form_alter() and variable_get/set() for FileField Insert, but I'm forced to store my own settings separate from the field instance. Probably a good third of the module is all just code to work around the lack of being able to save into the content_node_field_instances table.

skilip’s picture

skilip CreditAttribution: skilip commented

Subscribe

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Bumping this to the top of my list. I really love filefield insert, it's a module that will be very very useful as method to let users add images to their text and still have the images in places you can easily catalog in Views. That's not possible with other methods of inserting images, so I'd like to do what I can to get it functional. Plus I still think this would be a useful addition in general.

I am hoping someone can test out the concerns I posted in #8. No promises because my schedule is crazy, but if I can I will try to investigate those issues. Since I can't be sure if I can get to them, if anyone else can do some testing to move this along it would be great.

kenorb’s picture

kenorb CreditAttribution: kenorb commented

I can test it on ajax_trigger module after some modification as well.

yched’s picture

yched CreditAttribution: yched commented

+1 for getting this in - although I also have very little time, so it would be great if someone helped with the test Karen suggested.

Note that we are starting to see some similar use cases emerge in D7 land:
#394720-16: Migrate profile module data to field API
#501404-2: Field Permissions in Core
Well, we knew we'd have to tackle this in D7 too...

yched’s picture

yched CreditAttribution: yched commented

D7 patch in #502522: Allow drupal_alter() on the various Field API declarative hooks

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

I've been working with this patch in order to get the video upload module stable with FileField 3.0. The use of the drupal_alter hook is fairly light, but works as expected for the 'database columns' and 'views data' operations.

I haven't yet tested exporting and then importing w/o the module which seems to be one of the primary concerns raised in #8. The module does save nodes during cron with a node_save operation, and that is works just fine.

yched’s picture

yched CreditAttribution: yched commented

Quicksketch: we have a little problem on the D7 side with the 'alter field columns' part. See #502522: Allow drupal_alter() on the various Field API declarative hooks, comments #3 and #5.

Even in D6, I'm actually not sure what "alter field columns" results in: the changes do not take effect for existing fields until content_alter_schema() is re-run, right ? Strange things could happen meanwhile, with actual db columns not matching the publicized column definition...

Did you have an actual use case for this, or was it just for consistency with other hook_field_settings() $ops ?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Hey yched: Yeah an actual use-case would be the FileField-ImageField relationship. Right now FileField provides a super-hacky "data" column that stores a serialized list of extra data, including things like "alt" and "title" fields. In the situation where an "Image" widget were added to a FileField, it would be fantastic to be able to add alt and title as real columns in the database, rather than having to serialize the data and cram it into a single column.

In such a situation the column would be very dependent upon the widget (which makes sense in this scenario) since if an "Audio" widget were added on top of FileField, it wouldn't need an alt and title field at all. Fortunately this situation should not encounter any sort of problem with caches and content_alter_schema(), since this change to the field listing would only occur when a new module is enabled (or possibly updated via update.php). In both cases caches are completely cleared.

However, the ability to alter database columns is merely a plus. The functionality I personally was looking for was saving additional settings from the field settings form for use in FileField Sources and FileField Insert. But like jhedstrom, says above, he's specifically looking for the ability to extend database columns.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I was trying to think of a potential problem caused by modifying database columns, but every one I thought of was thwarted by CCK's handling of fields that have missing widgets. Since if a widget's providing module is missing, no modifications on that field are allowed so it's hard to run into a data problem where CCK might want to move that data around. If the widget is missing, then the data just sits there patiently and untouched until the widget module is enabled again.

The only situation where you could really get into trouble would be a place where a module modified database columns but was NOT the providing widget module. Then that module could be disabled, and you could change the field from single to multiple values and some database columns would be left behind. I'd be happy to reroll minus the database columns altering, but seems like that would eliminate the usefulness for jhedstrom in #23.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

The ability to alter database columns is key, not just for the use case I'm currently working on, but also for flexibility in the future (imagefield's data as quicksketch mentions above for example).

I don't think it should be removed just because poorly written modules might have the power to break something. The very nature of the drupal_alter system has the potential for one module to render another module unusable, but that doesn't mean we shouldn't offer hook_form_alter, hook_menu_alter, etc.

yched’s picture

yched CreditAttribution: yched commented

In the situation where an "Image" widget were added to a FileField, it would be fantastic to be able to add alt and title as real columns in the database"

Well, this doesn't really map to the CCK (and D7 Field API) model :-(. Db columns only depend on the field type, and possibly on field settings. Widgets (and formatters) are only plug-ins for a field type. Several instances of the same field can use different widgets, so db columns just cannot depend on the widget type, right ?
If a widget needs different columns, then it needs a different field type. More precisely, "If a widget needs different columns" doesn't really make sense in CCK.

This might be the limitation of 'audiofield = fieldfield with different widgets and formatters'. If an audiofield needs additional columns, then it's a different field type - duplicating lots of code, yes :-(. I think the real solution for this would be an OO Field API, where class audioField extends fileField and thus inherits code. Unfortunately, that will most likely not happen before D8.

Fortunately this situation should not encounter any sort of problem with caches and content_alter_schema(), this change to the field listing would only occur when a new module is enabled (or possibly updated via update.php). In both cases caches are completely cleared.

On new module enabled, CCK caches are cleared, but we don't run content_alter_schema() on all existing fields to check if db columns need to be synched. In the curren,t model, there's no need for that. Should there be a need, it would bring potential timeouts.
So my remark in #24 still stands.

So right now, the feature to alter db columns is possibly key, but I don't see how it's doable :-(

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
FileSize
content_field_settings_alter.patch10.65 KB

Several instances of the same field can use different widgets, so db columns just cannot depend on the widget type

Ah, yes you're quite right there. Well no point in holding up the whole train for this. Your D7 patch looks just fine and we can potentially move this widget-based fields thing to a separate issue. Though like you say, it's really not compatible with the current paradigm, so it seems unlikely to fit into CCK as we have it today.

Here's a patch that excludes the alter on field database columns.

jhedstrom: Maybe a good approach for modules that absolutely need extra columns is to depend on filefield then call module_invoke('filefield', 'field') and module_invoke('filefield', 'field_settings') as necessary in that module's implementations? Or optionally store all the extra data in a separate table similar to how filefield_meta works. I think yched is right here, widget-based columns really isn't feasible with the way CCK is constructed currently.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

I'm fine working around the immediate issue with video upload in the manner quickscketch mentions above, since that's how it's previously worked. However, I think it's a mistake to explicitly exclude the potential flexibility the db column altering could provide in the future. Allowing a widget (or even non-widget-providing modules, for that matter) to responsibly add db columns (or possibly alter/extend types) doesn't necessarily mean the field won't still work with other widgets.

mattyoung’s picture

mattyoung CreditAttribution: mattyoung commented

+1 for this.

this will solve my problem: #513284: Need someway to add additional field global_settings value

for a feature I'm trying to implement: #508990: Integration with cck fields?

Since this is in "needs review" status, can I assume this will be added?

markus_petrux’s picture

markus_petrux CreditAttribution: markus_petrux commented
Status: Needs review » Needs work

@quicksketch #29: Re: "Here's a patch that excludes the alter on field database columns."

I'm afraid that there are still a couple or three of drupal_alter invocations for 'database columns' in the patch.

Once fixed, the patch looks good to me. It should cause no harm nor compatibility issues with existing code, and it already offers a nice DX feature.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Well holy moly, I seemed to have only removed the first one. I'll reroll. Fantastic to see #502522: Allow drupal_alter() on the various Field API declarative hooks get in. Great work yched!

yched’s picture

yched CreditAttribution: yched commented

I would have preferred Karen's list in #8 was minimally tested, but it looks like neither CCK committer has real time for this, and we don't seem to be getting too much help.

Quicksketch gave a convincing "better to commit and find followup bugs in d6 than after d7 is released" on IRC, so unless Karen or Markus object, I'll commit this (without the leftover 'db columns alters') very soon.

yched’s picture

yched CreditAttribution: yched commented
Version: 6.x-2.x-dev » 6.x-3.x-dev
Status: Needs work » Patch (to be ported)

Committed the attached patch to CCK 2.x (same patch as #29, with all alters on 'db columns' cleaned out)
Thanks for the patience, quicksketch.

Markus, can you take care of the 3.x branch commit ? I don't think it really differs, but am not 100% sure.

markus_petrux’s picture

markus_petrux CreditAttribution: markus_petrux commented
Status: Patch (to be ported) » Fixed

Committed to CCK3 as-is. No diff with CCK2. ;)

Status: Fixed » Closed (fixed)

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

batje’s picture

batje CreditAttribution: batje commented
FileSize
cck_wordcount.tgz2.29 KB

Just wanted to show how cool this is. I wrote a Wordcount module for CCK using these hooks. Thanx for all your work.