It would be handy for link_field_settings_form to be replace partially or completely by link_field_instance_settings_form

This would allow per instance configuration of the link field. It also makes dynamically generating compound fields that include the link field easier.

Since none of the attributes set in link_field_settings_form affect how the database storage works, I don't think there is a need to use that hook at all.

Comments

jcfiala’s picture

Assigned: Unassigned » jcfiala

I'll admit I'm a little uncertain on all of the relevant hooks, but I'll take a look at that later today.

jcfiala’s picture

I've got this done locally, although I'm putting in more fixes/changes. It's forced me to get a better understanding of how the field process works, which is good.

I think I might need to do a hook_update_n for this, as instance and field settings are kept in different tables, and for folks who started working with an earlier version of link in D7 will otherwise have their field settings disappear.

pcambra’s picture

I'm not totally sure but I think that some of the "per instance" options of link should be in hook_field_widget_settings_form, like the title widget options and the rest in hook_field_instance_settings_form

jcfiala’s picture

Heh. I don't want to be argumentative, but I'm having a really bad month. Can you be more specific about which settings you think should be field-wide and which ones should be instance-wide, and say why?

I'd be happy to start a discussion on this before I push it to a release.

pcambra’s picture

Sure, glad to help, but I may introduced a confusion here:
Afaik, both hook_field_instance_settings_form & hook_field_widget_settings_form are instance based, but I think that the widget form is to define the widget itself.
hook_field_settings_form is field based.

IMHO I'd rather see all settings instance based than field based, but maybe there are other opinions out there.

I can help with the development as well, did you start working on this change?

jcfiala’s picture

Hmmm... I don't think you're right, but I haven't looked at the code in a few days.

I have mostly made this change, and then started banging on getting some unit tests up and working so I knew that the code was doing what I thought it was doing.

Unfortunately, then my wife's aunt died, and while we were in North Carolina for the funeral, an ice storm rolled in (on freaking North Carolina!) and stranded us there. I've only now returned home.

I'm hoping to spend some time on getting the code up into cvs tonight. That is, if we're still using cvs. Did we switch over to git already? I've been out of touch.

pcambra’s picture

Oh, I'm sorry about your loss.

We haven't moved to git yet, btw.

For the differences between hook_field_instance_settings_form, hook_field_widget_settings_form & hook_field_settings_form you can take a look to image.field.inc of core's image module.

jcfiala’s picture

Alright, I see where I was confused. There are three hooks for field settings we're discussing, and for some reason I missed that. Wow.

I've committed my changes to date to cvs, so you can have a look at what I've done to switch over to hook_field_instance_settings_form. I'm not entirely sure what's meant to be the difference between hook_field_instance_settings_form and hook_field_widget_settings_form, but I'm planning on digging about to try and figure it out.

pcambra’s picture

Ok, got the difference between hook_field_instance_settings_form and hook_field_widget_settings_form

hook_field_instance_settings_form - this is for defining the settings per instance of a field

hook_field_widget_settings_form - let's say that you create a module that exports a field widget that is going to be potentially used by other modules, this hook would allow to share a common settings form for all of them.

So maybe the hook_field_instance_settings_form is ok for this...

pcambra’s picture

I think this change is already commited to the module

jcfiala’s picture

Status: Active » Fixed

Okay, this is done for now - further suggestions on this are welcome, but I want to mark it as fixed.

Status: Fixed » Closed (fixed)

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