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
Comment #1
catchI think taxonomy access should remove itself as the options callback when disabled, should be doable by updating the field definition.
Comment #2
Les Lim@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.
Comment #4
Les LimWith git prefixes?
Comment #5
bfroehle CreditAttribution: bfroehle commentedBut 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.
Comment #6
xjmMmmm. 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:
Comment #7
catchI'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.
Comment #8
catchTagging for backport.
Comment #9
catchNeeds work per #7.
Comment #10
catchI'm also downgrading this from major since contrib modules should take care to clean up altered callbacks.
Comment #11
ExceptionHandler CreditAttribution: ExceptionHandler commentedI 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.
Comment #12
xjm#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.
Comment #13
xjmMarked #1358094: Function taxonomy_options_list() should check if function exists as a duplicate of this issue.
Comment #14
Schnitzel CreditAttribution: Schnitzel commentedI tried to do it with call_user_func() as suggested in #7: offending "submit" link with this code:
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
Comment #15
sunFor 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.
Comment #16
xjmWe 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?
Comment #17
catchcall_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.
Comment #18
sunAlright, here's the cuf() variant.
@xjm is right, we could/should add a simple test for this. Any takers?
Comment #19
sunerr, sorry, $function in #18 was slightly braindead.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedA 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.
Comment #21
jdolan CreditAttribution: jdolan commentedA 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:
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..
Comment #22
xjmPatches welcome!
Comment #23
Schnitzel CreditAttribution: Schnitzel commentedhere is the Patch for Taxonomy Access Control which cleans up the callbacks during disabling:
http://drupal.org/node/1358106#comment-5430946
Comment #24
Putr CreditAttribution: Putr commentedWhy 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.
Comment #25
gnugetAlright.
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.
Comment #26
areke CreditAttribution: areke commentedThis needs to be re-rolled again. The patch no longer applies.
Comment #27
alansaviolobo CreditAttribution: alansaviolobo commentedthe re-rolled patch seems rather weird. there is no code-change. just a test-change.
Comment #29
gnugetComment #30
bradklaver CreditAttribution: bradklaver commentedRerolled 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.
Comment #31
bradklaver CreditAttribution: bradklaver commentedComment #33
star-szrThanks @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.
Comment #34
andypostIs the issue still valid for 8.x?
should be one-liner
should be named differently
no need for extra empty lines
trailing white space
Comment #35
adci_contributor CreditAttribution: adci_contributor commentedCleaned up
Comment #37
jibranThere 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.Comment #38
xjmStill may need a fix in D7.
Comment #40
xjmComment #41
hgoto CreditAttribution: hgoto as a volunteer commentedI 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?)
Comment #42
xjmThanks @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.
Comment #43
hgoto CreditAttribution: hgoto as a volunteer commented@xjm, thanks. I got it. So I'll wait for someone to kindly review the patch in this thread.
Comment #44
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedActually the patch from #41 (where the direct
$function()
call was switched forcall_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 nameBut 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: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 totaxonomy_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.Comment #46
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedRemoving tag, this already has tests.
Comment #47
mcdruidNeeded a re-roll - no other changes.
Comment #48
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedReading this again after a year, I prefer to close this as Won't fix, because of this reason (see #20):
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: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.