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.
Comment | File | Size | Author |
---|---|---|---|
#33 | 1066274.patch | 1.81 KB | Damien Tournoud |
#30 | 1066274-fordrupal8.patch | 2.68 KB | Damien Tournoud |
#29 | 1066274-fordrupal8.patch | 1.67 KB | Damien Tournoud |
#21 | 1066274-21_options_list_instance.patch | 1.66 KB | xjm |
#10 | 1066274_options_list_instance.patch | 1.65 KB | pcambra |
Comments
Comment #1
pcambraComment #2
pcambraAdded the change to the api docs.
Comment #4
pcambraNow with --no-diff for git workflow as seen in [#707484]
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedmakes sense.
Comment #6
yched CreditAttribution: yched commentedHmm, 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.
Comment #7
pcambraIn 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.
Comment #8
yched CreditAttribution: yched commentedLooks good, thanks ! A couple nits :
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."
If the arg is optional, the param declaration should be $instance = NULL
Powered by Dreditor.
Comment #9
yched CreditAttribution: yched commentedAlso, there's little other choice than calling this a 'feature request' :-)
Comment #10
pcambraGot it, here is the patch with the suggested changes applied, thanks yched
Comment #11
yched CreditAttribution: yched commentedCool, thanks pcambra :-)
Works for me.
Comment #12
aaron CreditAttribution: aaron commentedthis 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.
Comment #13
pcambraPatch is exactly the same for D7 and D8 atm
Tagging "Needs Drupal 7 backport" following rfay's advice http://twitter.com/#!/randyfay/status/56390621337894912
Comment #14
yched CreditAttribution: yched commentedStill RTBC for D8
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented+1 from here too.
Comment #16
amateescu CreditAttribution: amateescu commentedCorrected D7 backport tag.
Comment #17
webchickThis 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)
Comment #18
xjmIt'll probably need a reroll; testing.
Comment #19
xjm#10: 1066274_options_list_instance.patch queued for re-testing.
Comment #21
xjmRerolled against current D8 codebase; should still be RTBC.
Comment #22
xjmAdded issue summary.
Comment #22.0
xjmUpdated issue summary.
Comment #23
sunFor D8, that new argument should be obligatory IMHO.
For D7, the re-rolled patch in #21 looks RTBC and good to go.
Comment #24
catch#21: 1066274-21_options_list_instance.patch queued for re-testing.
Comment #25
catchI 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?
Comment #26
xjmComment #27
xjmActually, 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 ofNULL
. 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."Comment #28
pcambraThanks xjm for pushing this :)
I was going to port it but it's a great example of novice task!
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedHow long can it possibly take to get some small feature request in core? This is really ridiculous.
Here is the version 8 patch.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedWith the two implementations in core.
Comment #31
xjmDamZ, Drupal novice. :P
Comment #32
catchCommitted and pushed, thanks!
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedRe-roll of the Drupal 7 version.
Comment #34
webchickLooks like a harmless API addition.
Committed and pushed to 7.x. Thanks!
Comment #35.0
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAdded link to latest patch roll.