Pass all the parameters of hook_options_list() to options_list_callback

This makes complete sense.

My only thought is would it make sense that a $form array could be an optional parameter to allow for options list to also be generated contextually on form parameters? (and this of course is a seperate issue and needs tweaks 'upstream' in code).

Makes sense to me, committed/pushed. Will need a change notification.

So, in #1541792: Enable dynamic allowed list values function with additional context, list_allowed_values() was converted pass all parameters, in both D7 and D8.
Is this getting backported as well? Adding the tag for now.

Added a change record.

Also, just pointing out #1548004: Allowed values functions are undocumented.

We'd also need to convert list_test_allowed_values_callback().

As I understand it, API additions are backportable, so updating issue accordingly, now that we have the change record.

We'd also need to convert list_test_allowed_values_callback().

Sorry. Missed that part. Yes.

Hi, wondering if this is the only change?

Thanks.

Fixing tags per http://drupal.org/node/1517250

List is now Options.

Committed/pushed to 8.x, moving to 7.x for backport again.

Is it safe to say that this has been reviewed and tested by the community after it's been three years and it's been ported to D8?

Seems to work fine. Can someone push this to core of D7?

Maybe we need to retest 3 years later, re-uploaded the patch for testing.

Looks good, I think if we publish a change notice this can go into a bugfix release

Committed and pushed to 7.x, thanks!

This change broke the Feeds module. See https://www.drupal.org/pift-ci-job/586774
Is taxonomy_allowed_values() not supposed to be called directly by contrib? If so, I think that the function docs should mention that.

If taxonomy_allowed_values() isn't expected to be called directly, please notify when the next Drupal core release is being made so I can create a new Feeds release on the same day.

I think this is easily resolved by just setting default values in taxonomy_allowed values.

The added parameters aren't used at all in taxonomy_allowed_values(), so I'm wondering why they needed to be added. PHP allows us to pass extra parameters to a function even if not defined in the function signature. Is it only for consistency? If so, is that worth an API break?

@geru
Note that your patch will fail to apply, as it isn't made against the latest dev. Most of the changes in your patch are already committed.

OK. So, the answer options are:

• either remove the parameters from taxonomy_allowed_values()
• or put them in and set their defaults to NULL.

Which is the best?

Based on the above, I rolled this back for now. taxonomy_allowed_values() is a public API function (and documented as such) and I think it's used other places in contrib and custom code too, so this definitely does look like a problem.

The change record for this issue (https://www.drupal.org/node/1638360) is a little confusing in that it mixes a couple things together... but it's also 4 years old (and partially associated with a different issue that was fixed 4 years ago) so I didn't bother pushing it back to draft state. It will need some edits and/or a new change record when this gets committed again, to clarify which types of callbacks the API addition actually applies to.

Then, can we not just put the default NULL values for the added parameters and call it a day?

Personally I don't see the point in adding new optional parameters to a function when the function doesn't actually use them :) So I would vote for not changing taxonomy_allowed_values() at all.

Some of the documentation that was added here could be used in #1548004: Allowed values functions are undocumented instead. I'll mark that issue for Drupal 7 backport now.

The reason is to make the default taxonomy_allowed_values() function compatible with the new callback having multiple parameters.

The reason that I am here on this list is because I want to implement my own allowed_values_callback that takes into account the extra parameters in order to provide instance-specific allowed values instead of field-specific allowed values.

Furthermore, there is a module called content_taxonomy with more than 21,000 sites that promises to provide instance-level functionality, but can not because the instance information is lost.

If we go ahead and put the added parameters defaulted to NULL in the default taxonomy_allowed_values() function, then we should be able to make this work for everyone.

Here is a patch against core 7.53 with NULL defaults in added parameters of taxonomy_allowed_values()

Added spaces for coding standards.

@MegaChriz
Did patch #42 resolve the issue in #28?

As proposed by David_Rothstein in #39, taxonomy_allowed_values() is better to leave untouched:

Personally I don't see the point in adding new optional parameters to a function when the function doesn't actually use them :) So I would vote for not changing taxonomy_allowed_values() at all.

As documented on https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... the parameters of a callback function should be documented in a *.api.php file. As the taxonomy module is invoking the callback here, the callback should be documented in taxonomy.api.php.

New patch attached.

The patch looks good to me and applies cleanly. Does anyone have a line on what testing it failed in #45?

It looked like a random test failure. Tests are passing now, so setting back to "Needs review".

I have tested this and confirm that it works as expected on my installation. Would it be reasonable to mark it as reviewed and tested by the community?

Yes it would. Code looks good, several people tested it.

So, to migrate this into core, should the status be changed to Patch to be ported, or Fixed?

@geru
No, the status "Fixed" would mean that - in this case - the patch was committed. The status "Patch (to be ported)" would mean that the patch was committed as well but the changes are needed for an other major version of the project as well. See https://www.drupal.org/node/156119 for more information about what each status means.

The next step for getting this change in is that a core maintainer reviews the patch and then eventually commits it if he/she has no concerns about it. What could help though is if more people would review the patch: that indicates that there are more people in favor of this change and if the reviews are thorough it would give the core maintainer more trust that the patch doesn't introduce unwanted side effects.

@MegaChris, Thank you for the link about status settings.

@kristiaanvandeneynde reports that several people tested it.

I just wanted to make sure that it was not forgotten.

@joachim
What if a function is used both as a "general" function and as a callback? The thing is that this function is also used now in a non-callback context. I think giving this function the "callback" status now would indicate that it should no longer be called directly? My concern is that future changes may break contrib code: because if the consensus is that a callback shouldn't be called directly, its signature may be freely changed. What's your opinion about this? Do you think it is a valid concern?

@Joachim, So, is this held up because of the documentation in the header of the taxonomy_allowed_values() function?

If so, I would like to get it properly documented so we can move forward.

However, I am unclear on what it should look like. It already looks reasonably documented to me.

Your comment and quoted diff in comment #53 don't make sense to me.

The name of the callback is 'options_list_callback'. It is not 'callback_options_list' as suggested in your comment and it is not 'taxonomy_options_list()' as currently listed in the doc block.

If you might be able to write the doc block properly or educate me on how to write it properly, then, we should be able to get this patch of a single line of actual code incorporated into core.

@joachim OK, So wouldn't the additions of documentation on line 1648 and 1657 from #44 above satisfy the requirements you state in #57? As for the proposal in #55 not sure what would be broken by the change on line 1497 of #44.

I agree with joachim that functions that are used as callback function should be documented in a standard way. So ideally, a function shouldn't be both API and callback as that can cause limitations in the future: with my patch in #44 the implementation of the default callback for taxonomy_options_list() cannot be easily changed as adding more parameters would break the API.

I was still trying to figure out how to document in a standard way that a function may be used as API function. I could just add a line "Is an API function." in the function docs of taxonomy_allowed_values(), but somehow that looked a bit silly to me. I think a function that doesn't start with the word "Implements", should assumed to be usable as an API function. I did came across #983274: Mark API functions with @public in Doxygen as an example how a function can be explicitly marked as API function, but it doesn't look like that became a coding standard in Drupal, so would probably be awkward to use right now.

Anyway, here is a patch that follows the suggestions that were made in #55:

• A function called taxonomy_options_list_default() is added that includes all parameters passed that are passed to the callback. The function is documented as a callback function following the standards at https://www.drupal.org/docs/develop/coding-standards/api-documentation-a....
• Instead of taxonomy_allowed_values(), taxonomy_options_list_default() is now the default callback for taxonomy_options_list().
• taxonomy_options_list_default() simply calls taxonomy_allowed_values().

This is much needed. Could a maintainer review this?