Posted by Les Lim on September 16, 2011 at 4:39pm
16 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | taxonomy.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | gnuget |
| Status: | needs review |
| Issue tags: | needs backport to D7, Needs tests |
Issue Summary
This refers to changes made in #730418: Allow modules to bypass taxonomy_get_tree() for term reference select lists.
Modules can define a custom callback function for taxonomy_options_list() which gets saved as field data in {field_config}. However, taxonomy_options_list() doesn't make a function_exists() check before trying to call it, causing a PHP fatal error if it the custom module is disabled or uninstalled.
This just came up for me trying to uninstall taxonomy_access.module on a site. After disabling it, taxonmy_options_list still tries to call _taxonomy_access_term_options() because the callback is still stored in the field data.
Comments
#1
I think taxonomy access should remove itself as the options callback when disabled, should be doable by updating the field definition.
#2
@catch: I can agree with that, and I'll open an issue separately in that queue.
Still, as far as I can tell, every other variable function call in core is wrapped in a function_exists() check. I can't think of a reason why this should be different.
#3
The last submitted patch, 1281732-taxonomy_options_list.patch, failed testing.
#4
With git prefixes?
#5
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.
#6
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:
<?php
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';
}
?>
#7
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.
#8
Tagging for backport.
#9
Needs work per #7.
#10
I'm also downgrading this from major since contrib modules should take care to clean up altered callbacks.
#11
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.
#12
#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.
#13
Marked #1358094: Function taxonomy_options_list() should check if function exists as a duplicate of this issue.
#14
I tried to do it with call_user_func() as suggested in #7: offending "submit" link with this code:
<?php$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
#15
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.
#16
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?
#17
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.
#18
Alright, here's the cuf() variant.
@xjm is right, we could/should add a simple test for this. Any takers?
#19
err, sorry, $function in #18 was slightly braindead.
#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.
This is a won't fix to me.
#21
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..
#22
Patches welcome!
#23
here is the Patch for Taxonomy Access Control which cleans up the callbacks during disabling:
http://drupal.org/node/1358106#comment-5430946
#24
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.
#25
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.