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.

Files: 
CommentFileSizeAuthor
#33 1066274.patch1.81 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 36,760 pass(es).
[ View ]
#30 1066274-fordrupal8.patch2.68 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 33,446 pass(es).
[ View ]
#29 1066274-fordrupal8.patch1.67 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 33,443 pass(es).
[ View ]
#21 1066274-21_options_list_instance.patch1.66 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,057 pass(es).
[ View ]
#10 1066274_options_list_instance.patch1.65 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1066274_options_list_instance_3.patch.
[ View ]
#7 1066274_options_list_instance.patch1.49 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 31,561 pass(es).
[ View ]
#4 1066274_options_list_instance.patch1.38 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 31,582 pass(es).
[ View ]
#2 1066274_options_list_instance.patch1.4 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1066274_options_list_instance_0.patch.
[ View ]
#1 1066274_options_list_instance.patch673 bytespcambra
PASSED: [[SimpleTest]]: [MySQL] 31,566 pass(es).
[ View ]

Comments

StatusFileSize
new673 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,566 pass(es).
[ View ]

Status:Active» Needs review
StatusFileSize
new1.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1066274_options_list_instance_0.patch.
[ View ]

Added the change to the api docs.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 31,582 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

makes sense.

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.

Status:Needs work» Needs review
StatusFileSize
new1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 31,561 pass(es).
[ View ]

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.

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.

Category:task» feature

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

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1066274_options_list_instance_3.patch.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Cool, thanks pcambra :-)

Works for me.

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.

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

Status:Needs review» Reviewed & tested by the community

Still RTBC for D8

+1 from here too.

Issue tags:+needs backport to D7

Corrected D7 backport tag.

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)

It'll probably need a reroll; testing.

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.66 KB
PASSED: [[SimpleTest]]: [MySQL] 33,057 pass(es).
[ View ]

Rerolled against current D8 codebase; should still be RTBC.

Added issue summary.

Issue summary:View changes

Updated issue summary.

For D8, that new argument should be obligatory IMHO.

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

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?

Assigned:Unassigned» xjm

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

Thanks xjm for pushing this :)

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 33,443 pass(es).
[ View ]

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

Here is the version 8 patch.

StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 33,446 pass(es).
[ View ]

With the two implementations in core.

DamZ, Drupal novice. :P

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

Committed and pushed, thanks!

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.81 KB
PASSED: [[SimpleTest]]: [MySQL] 36,760 pass(es).
[ View ]

Re-roll of the Drupal 7 version.

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.

Issue summary:View changes

Added link to latest patch roll.