Fatal error when taxonomy_options_list() tries to call an undefined callback function

I think taxonomy access should remove itself as the options callback when disabled, should be doable by updating the field definition.

But there is a separate issue for D8 which advocates removing all of the function_exists calls. I don't have the issue number at the moment.

Mmmm. Tracking.

The patch looks reasonable to me I guess, but it seems odd that Field API continues to cache the definition from a disabled module's hook. Shouldn't a cache clear resolve this sort of thing?

Edit: #1059884: Drop function_exists for 'callback' functions is the issue referred to above.

Edit: Here's the TAC hook, for reference:

function taxonomy_access_field_info_alter(&$info) {

  // Return if there's no term reference field type.                            
  if (empty($info['taxonomy_term_reference'])) {
    return;
  }

  // Use our custom callback in order to disable list while generating options.
  $info['taxonomy_term_reference']['settings']['options_list_callback'] = '_taxonomy_access_term_options';
}

I'm pretty sure the default is stored in field settings for each field as serialized data (though not at computer to check this). Probably field API could store the fact the default is being used rather than what it is there. If this is the case should probably be its own issue.

From #1059884: Drop function_exists for 'callback' functions we were discussing call_user_func() for this so it's a warning rather than a fatal.

Tagging for backport.

Needs work per #7.

I'm also downgrading this from major since contrib modules should take care to clean up altered callbacks.

I experienced the same problem. I'm creating nodes and terms programatically and probably I messed somewhere with terms and vocabularies IDs. To resolve this problem I needed to delete field tags (term reference) from problematic content type. Now I can edit nodes of this type without getting fatal error with mentioned message.

Edit: I'm using Drupal 7.9. Sorry if I did mess because I see now it's thread about dev version of D8.

#11: No worries. Bugs in Drupal 7 are filed against Drupal 8 to prevent regressions. Usually a fix is applied to both branches around the same time.

Marked #1358094: Function taxonomy_options_list() should check if function exists as a duplicate of this issue.

I tried to do it with call_user_func() as suggested in #7: offending "submit" link with this code:

$return = call_user_func($field['settings']['options_list_callback'], $field);

the Problem is, that there is no possibility to check if the called function exists or not (well I didn't found one)

!empty($return) does not work, because the return could be empty

So the question is if function_exists() is really soo bad?

I attached a Patch for the newest D8-dev version at #1358094: Function taxonomy_options_list() should check if function exists there is one for the newest D7-dev

For D7, we can only add a function_exists() there (as proposed).

For D8, this is the wrong approach to fix the issue. Squeezing a function_exists() in there fixes the effect, but not the cause. We need to fix the cause - i.e., the field configuration must not contain an invalid callback name in the first place.

In light of that, I'd suggest to commit #14 to D8 and D7 as a stop-gap fix. Afterwards, move this issue back to D8 and fix the cause.

We could probably add a test for this, no? Create a test module that defines a phony callback function, and test what happens when the user tries to use a taxonomy options field?

call_user_func() ought to return nothing if the function doesn't exist, so I'm not sure why that's a problem from #14?

Would really like to avoid the silent failure as well in D7 if we can so it'd be good to know why it's not possible to do that.

Alright, here's the cuf() variant.

@xjm is right, we could/should add a simple test for this. Any takers?

err, sorry, $function in #18 was slightly braindead.

A fatal error is the proper response here. There is no guarantee that the result of this function is going to be correct if the callback function does not exist. As a consequence, the consumers of this function might proceed and you will end up with inconsistent data.

This is a won't fix to me.

A code-free way to fix this (warning: total hack) is to patch the database. MySQL supports a "REPLACE" mechanism for basic pattern matching and replacement:

UPDATE field_config SET data = REPLACE(data, 's:29:"_taxonomy_access_term_options"', 's:0:""');

Use at your own risk.

Also beware that any fields you are managing with Features will have this setting baked into the serialized field definition (see my_feature.features.field.inc).

It's pretty absurd that taxonomy_access doesn't unregister itself cleanly on uninstall..

It's pretty absurd that taxonomy_access doesn't unregister itself cleanly on uninstall..

Patches welcome!

It's pretty absurd that taxonomy_access doesn't unregister itself cleanly on uninstall..

Patches welcome!

here is the Patch for Taxonomy Access Control which cleans up the callbacks during disabling:

http://drupal.org/node/1358106#comment-5430946

Why has this STILL not been pached in D7.

I just applayed it as a hot patch (#14) to my production site and can confirm it works.

Alright.

I did a simple test for #19.

First i attach a re-rolled version of the patch.

And Second i re-attach the patch with a simple test.

This needs to be re-rolled again. The patch no longer applies.

the re-rolled patch seems rather weird. there is no code-change. just a test-change.

Rerolled gnuget's drupal8.taxonomy-options-callback.1281732.25.patch. It rerolls the test, but I believe the function gnuget's patch attempts to fix, has been rewritten correctly to taxonomy\src\Plugin\Field\FieldType\TaxonomyTermReferenceItem.php in the function getSettableOptions.

Thanks @bradklaver it was a pleasure sprinting with you last month. Just removing the needs reroll tag, this still applies just needs some troubleshooting I think.

Is the issue still valid for 8.x?

1. +++ b/core/modules/field/tests/modules/field_test/field_test.module
   @@ -145,6 +145,29 @@ function field_test_query_efq_table_prefixing_test_alter(&$query) {
   +* Callback function for test the options_list_callback's setting
   +* in the taxonomy_term_reference type field.
   

   should be one-liner

2. +++ b/core/modules/field/tests/modules/field_test/field_test.module
   @@ -145,6 +145,29 @@ function field_test_query_efq_table_prefixing_test_alter(&$query) {
   +function taxonomy_test_set_options_list($field) {
   

   should be named differently

3. +++ b/core/modules/field/tests/modules/field_test/field_test.module
   @@ -145,6 +145,29 @@ function field_test_query_efq_table_prefixing_test_alter(&$query) {
   +}
   +
   +
   +
   +
   +
    /**
   
   +++ b/core/modules/taxonomy/src/Tests/TermFieldTest.php
   @@ -178,4 +178,22 @@ function testTaxonomyTermFieldChangeMachineName() {
   +
   +  /**
   +   * Tests options_list_callback
   

   no need for extra empty lines

4. +++ b/core/modules/taxonomy/src/Tests/TermFieldTest.php
   @@ -178,4 +178,22 @@ function testTaxonomyTermFieldChangeMachineName() {
   +  ¶
   

   trailing white space

Cleaned up

There is no more taxonomy_options_list() after #1847596: Remove Taxonomy term reference field in favor of Entity reference so we don't have this bug anymore.

Still may need a fix in D7.

I believe these patches (fix + test) can be backported to D7 with small adjustment. This patch includes both fix and test.

(Is it OK to use this thread or should we make a new issue for D7 backport?)

Thanks @hgoto. We do make new issues now for Drupal 7 backports generally, but in this case no fix was needed in D8, so it's fine to use this issue.

@xjm, thanks. I got it. So I'll wait for someone to kindly review the patch in this thread.

Actually the patch from #41 (where the direct $function() call was switched for call_user_func()) does not make any difference now, because in PHP 8+ you will get:

Fatal error: Uncaught TypeError: <code>call_user_func(): Argument #1 ($callback) must be a valid callback, function "xxx" not found or invalid function name

But even if it would not throw this error, as Damien mentioned in #20, it is not good to rely on some potentially incorrect response especially if the hook_options_list() has this documentation:

@return
 *   The array of options for the field. Array keys are the values to be
 *   stored, and should be of the data type (string, number...) expected by
 *   the first 'column' for the field type. Array values are the labels to
 *   display within the widgets. The labels should NOT be sanitized,
 *   options.module takes care of sanitation according to the needs of each
 *   widget. The HTML tags defined in _field_filter_xss_allowed_tags() are
 *   allowed, other tags will be filtered.

According to this I would expect the taxonomy_options_list() to return array or throw an error. Not something empty / different.

So I think that the one option is to leave it as won't fix and the other option is to use function_exists() as the patch in #4 was trying to achieve. If we really want to avoid silent fail (and fallback to taxonomy_allowed_values()) then some watchdog log can be added to inform user about non-existing/wrong callback function.

I have rerolled patch from #4 with tests from #41 and added one additional test case to check the real failure if the callback in options_list_callback is non-existent.

Removing tag, this already has tests.

Needed a re-roll - no other changes.

Reading this again after a year, I prefer to close this as Won't fix, because of this reason (see #20):

A fatal error is the proper response here. There is no guarantee that the result of this function is going to be correct if the callback function does not exist. As a consequence, the consumers of this function might proceed and you will end up with inconsistent data.

If we commit this patch, we will change the expected output, because instead of custom callback, the default taxonomy_allowed_values function will be used even if the custom callback function is set:

-  $function = !empty($field['settings']['options_list_callback']) ? $field['settings']['options_list_callback'] : 'taxonomy_allowed_values';
+  if (!empty($field['settings']['options_list_callback']) && function_exists($field['settings']['options_list_callback'])) {
+    $function = $field['settings']['options_list_callback'];
+  }
+  else {
+    $function = 'taxonomy_allowed_values';
+  }

This is not ideal and it could lead to unexpected behavior. If anybody needs the patch, feel free to use it to prevent the error.

Thanks all for your effort here.