Problem/Motivation

hook_options_list() defines the list of options used in a field widget. However, the hook does not provide a way for the module defining the field to adjust the list of options depending on the specific instance. It would be useful to supply the instance as context for the hook.

Proposed resolution

Add an optional parameter $instance to the hook.

Remaining tasks

Patch in #1066274-21: hook_options_list should allow to pass the instance of a field adds and documents the hook. Core committer has okayed the addition.

Commit patch once the core codebase is suitably unfrozen.

User interface changes

None.

API changes

Minor, backwards-compatible addition only. No existing code will be affected.

Function signature will change from:
hook_options_list($field)
To:
hook_options_list($field, $instance = NULL)

Original report by @pcambra

hook_options_list allows to define the options of a widget for of a field, but it only passes $field as parameter to generate the options, so I think it would make sense to include $instance as well, so the instance context is available to the modules that need to define the options listed in a widget form depending on the instance.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

pcambra’s picture

Status: Active » Needs review
FileSize
1.4 KB

Added the change to the api docs.

Status: Needs review » Needs work

The last submitted patch, 1066274_options_list_instance.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Now with --no-diff for git workflow as seen in [#707484]

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

makes sense.

yched’s picture

Status: Reviewed & tested by the community » Needs work

Hmm, dunno about that. 'Allowed values' are typically a field-level property.

The main reason is Views : when using a filter on a field, you must get the "allowed values" without having an $instance (since, by nature, Views listings are cross-bundles).

Views filters currently use a raw FAPI 'select' (or radio / checkboxes), but having them use actual 'Field widgets' is a long-standing request (especially makes sense for exposed filters). If/when this happens, options.module widgets would hit the problem above if this patch is committed as is.

Suggestions:
- make the $instance param optional, and explicitly state that it might be missing in incoming calls. hook_options_list implementations should still be able to provide a list of values.
- deriving from the above, per $instance "variants" of the list of options should probably only *remove* values from a list defined at the $field level - not *add* new values. This recommendation should probably be added to the phpDoc.

pcambra’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

In Drupal Commerce we would need this in order to be able to restrict the products that are available within a product display, and this is done by a field instance setting that filters which product types are avaiable and which aren't, so you're right that this is a feature for removing options mostly.

I haven't found any problems with the widget when only $field but not $instance is informed, but I've improved the docs with yched's suggestions.

yched’s picture

Status: Needs review » Needs work

Looks good, thanks ! A couple nits :

+++ modules/field/modules/options/options.api.php
@@ -16,6 +16,9 @@
+ * @param $instance
+ *   The instance definition. This is an optional parameter and it is mostly
+ *   used to filter out values previously defined by the field.

I'd suggest :
"(optional) The instance definition. The hook might be called without an $instance parameter in contexts where no specific instance can be targeted. It is recommended to only use instance level properties to filter out values from a list defined by field level properties."

+++ modules/field/modules/options/options.api.php
@@ -26,7 +29,7 @@
+function hook_options_list($field, $instance) {

If the arg is optional, the param declaration should be $instance = NULL

Powered by Dreditor.

yched’s picture

Category: task » feature

Also, there's little other choice than calling this a 'feature request' :-)

pcambra’s picture

Status: Needs work » Needs review
FileSize
1.65 KB

Got it, here is the patch with the suggested changes applied, thanks yched

yched’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thanks pcambra :-)

Works for me.

aaron’s picture

Issue tags: +options

this is nice. i'd also like to allow for #1085704: Add hook_options_list_alter so that other modules can act on this as well, especially once we have an instance available.

pcambra’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

Patch is exactly the same for D7 and D8 atm

Tagging "Needs Drupal 7 backport" following rfay's advice http://twitter.com/#!/randyfay/status/56390621337894912

yched’s picture

Status: Needs review » Reviewed & tested by the community

Still RTBC for D8

Damien Tournoud’s picture

+1 from here too.

amateescu’s picture

Issue tags: +Needs backport to D7

Corrected D7 backport tag.

webchick’s picture

This looks like a pretty harmless API addition, and according to Damien it's required for Drupal Commerce to function fully.

However, we're currently in feature freeze because there are too many critical bugs. I'll have to come back to this once that number gets down to an acceptable level. (<=15)

xjm’s picture

It'll probably need a reroll; testing.

xjm’s picture

Issue tags: -options, -Needs backport to D7

Status: Reviewed & tested by the community » Needs work
Issue tags: +options, +Needs backport to D7

The last submitted patch, 1066274_options_list_instance.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.66 KB

Rerolled against current D8 codebase; should still be RTBC.

xjm’s picture

Added issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

For D8, that new argument should be obligatory IMHO.

For D7, the re-rolled patch in #21 looks RTBC and good to go.

catch’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

I agree with making this obligatory for 8.x, was thinking about making it a followup but this is a tiny patch as is, could we get a re-roll where the param is required and adjusted docs?

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Assigned: xjm » Unassigned
Issue tags: +Novice

Actually, I'll use this reroll as a novice issue today.

http://drupal.org/files/issues/1066274-21_options_list_instance.patch needs to be rerolled against 8.x with the $instance parameter no longer providing a default of NULL. The field definition should remove the word "optional" and the sentence "The hook might be called without an $instance parameter in contexts where no specific instance can be targeted."

pcambra’s picture

Thanks xjm for pushing this :)

I was going to port it but it's a great example of novice task!

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.67 KB

How long can it possibly take to get some small feature request in core? This is really ridiculous.

Here is the version 8 patch.

Damien Tournoud’s picture

FileSize
2.68 KB

With the two implementations in core.

xjm’s picture

DamZ, Drupal novice. :P

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Committed and pushed, thanks!

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.81 KB

Re-roll of the Drupal 7 version.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like a harmless API addition.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

killes@www.drop.org’s picture

Issue summary: View changes

Added link to latest patch roll.