Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We do it for nodes, users and comments. There's no reason why this shouldn't be done for Taxonomy also!
Related to: #1905004: Error when finding dependencies in flag_features_export()
Comments
Comment #1
joachim CreditAttribution: joachim commented> We do it for nodes, users and comments
We don't actually... we implement the hook ourselves and declare all the entity types we support there.
That won't help with #1905004: Error when finding dependencies in flag_features_export() because IIRC we don't say which module provides the entity types.
I'm not sure there's much point to this, as we don't need a dedicated flag handler class for taxonomy terms.
Comment #2
alexweber CreditAttribution: alexweber commented> I'm not sure there's much point to this, as we don't need a dedicated flag handler class for taxonomy terms.
If we implement taxonomy_flag_type_info() and list it's handler as flag_entity, then the taxonomy module will get correctly picked up as a dependency with exported flags on taxonomy terms (and vocabularies).
The same exact situation also applies to the core File entity. It's an optional module and the dependency won't be discovered otherwise...
Comment #3
joachim CreditAttribution: joachim commentedThere's no declaration of the providing module:
Comment #4
alexweber CreditAttribution: alexweber commentedJoachim, I mean literally implementing
The module used as a dependency is, in fact, the one implementing the hook. Correct me if I'm wrong but if we do it on behalf of the taxonomy module, not fr it, then the taxonomy module will correctly get picked up as the dependency!
PS - Despite being "rude" to implement this on behalf of the taxonomy module, using it's prefix and all, there's no chance this will break because core will never support this directly. Also, this exact practice is used in Views...
Comment #5
joachim CreditAttribution: joachim commented> then the taxonomy module will correctly get picked up as the dependency!
I don't think it would -- look at flag_fetch_definition().
Comment #6
alexweber CreditAttribution: alexweber commentedJoachim, I'm sorry to say this but you are wrong! :)
I've just tested this myself:
Add the following code to flag.flag.inc:
Repeat the above steps. It now correctly identifies the taxonomy module as a dependency!
Note: the module_exists() is kinda useless because even if Taxonomy doesn't exist this code should cause no side effects and it might actually be cheaper NOT to test for taxonomy module on every request ;)
Comment #7
joachim CreditAttribution: joachim commentedWell I have **no** idea how that works! The flag definition has no idea which module provided it.
> Note: the module_exists() is kinda useless because even if Taxonomy doesn't exist this code should cause no side effects and it might actually be cheaper NOT to test for taxonomy module on every request ;)
Agreed.
> PS - Despite being "rude" to implement this on behalf of the taxonomy module
Not at all, it's a fairly standard pattern in contrib -- lots of it in Views for instance.
Comment #8
alexweber CreditAttribution: alexweber commented> Well I have **no** idea how that works! The flag definition has no idea which module provided it.
It's pretty simple really: flag_features_export() uses information from flag_features_providing_module().
module_invoke() keys the return array by the name of the module that implemented the hook, and we also key our own return array with the same variable. Since we implement it on behalf of taxonomy, module_invoke() it doesn't know any better and it just works! :)
I think what messed you up is that you're thinking about the flag definition but it's really flag_features_providing_module() that does the work here...
Comment #9
joachim CreditAttribution: joachim commentedOh blimey, that's a bit wacky!
I guess for this issue we should move comment flags to a hook on behalf of comment module for the same reason. node and user flags don't need to move, since those modules are always enabled anyway.
Comment #10
alexweber CreditAttribution: alexweber commented> Oh blimey, that's a bit wacky!
Hahahaa, yeah it is! :)
> I guess for this issue we should move comment flags to a hook on behalf of comment module for the same reason.
The reason I didn't suggest this in the first place is because you actually implement a flagging class for comment but, come to think of it, I don't think this makes a difference and the comment dependency is probably not being picked up either...
Patch on the way!
Comment #11
alexweber CreditAttribution: alexweber commentedComment #12
alexweber CreditAttribution: alexweber commentedIf #1905766: Don't allow flagging taxonomy vocabularies get committed, we should remove the implementation for taxonomy vocabularies... :)
Comment #13
alexweber CreditAttribution: alexweber commentedNew patch that, according to #12, removes declaration for taxonomy vocabularies.
Comment #14
joachim CreditAttribution: joachim commentedCommitted!
git commit -m "Issue #1905750 by alexweber: Added implementations of hook_flag_type_info() on behalf of non-required core modules." --author="alexweber "
Here's the patch I committed -- I tweaked comments a bit -- in case this gets backported.
Comment #15
joachim CreditAttribution: joachim commentedThere's a problem with this patch -- the comment hook is not getting picked up, and I have no idea why.
This is now either a critical bug, or I have to revert the patch :(
Comment #16
joachim CreditAttribution: joachim commentedThis returns nothing at all, even after lots of cache clears. The taxonomy hook is working fine, so what's different about comment module?
Comment #17
alexweber CreditAttribution: alexweber commented@joachim, I've found out why this happens but can't provide a good explanation...
It has to do with module weights: all of comment, flag and taxonomy have weight = 0 so the hooks run in alphabetical order; first comment, then flag, then taxonomy.
Put this in hook_init().
It will print an array with: "node", "user" and "taxonomy_term" as the keys.
Now change flag's module weight to 1, so it runs after taxonomy.
Now flush caches and run the hook_init() code again:
It will print an array with only "node" and "user" as they keys.
Now change flag's module weight to -1, so it runs before comment and taxonomy:
It will print an array with only "node", "user", "taxonomy_term" and "comment" as they keys.
Boom! :)
So, for whatever, reason, since we're implementing the hook on the behalf of the module, our module has to run before that specific module. Not sure why this works like this; I would have expected it to the opposite, requiring us to run after the module we're implementing the hook for, but there it is.
Not sure how to proceed and whether it's an option to reduce our weight and whether its worth it just to satisfy this requirement...
Comment #18
joachim CreditAttribution: joachim commentedWow. Nice work on the debugging!!!
I have figured out why this is. It's to do with the way http://api.drupal.org/api/drupal/includes!module.inc/function/module_imp... loads includes files based on hook_hook_info(). This goes:
- if the hook function exists, the module implements the hook
- otherwise, try to load the include file for the hook
- now check again if the function exists
So what happens for us is:
- comment. No hook. Try to load comment.flag.inc, which doesn't exist. Still no hook.
- flag. No hook. Try to load flag.flag.inc. Now we have a hook.
- taxonomy. Because flag.flag.inc is now loaded, the function exists.
The fixes I can think of are:
- move the comment hook implementation into the module file. Which is not brilliant.
- change our module weight, which is fiddly
- implement http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... to tweak the order this hook runs in. also a bit fiddly.
- revert this patch and add a 'module' key to the flag type data, which would be optional, but which we'd set for the core flag types we define.
Comment #19
alexweber CreditAttribution: alexweber commented> Wow. Nice work on the debugging!!!
Thanks! :) In my experience if something doesn't work that simply *should*, fiddle with module weights and see what happens...
> I have figured out why this is
Wow, nice! Thanks for explaining that, I had no idea :)
As far as the options go, I agree with you 100%, let's add an extra 'module' key and be done with it! It's transparent and nobody else will ever use it so it'll be our little secret :P
Edit: I'm not sure we really need to revert the patch, can't we roll a new one that moves the core module's stuff up into our hook implementation?
Comment #20
joachim CreditAttribution: joachim commented> roll a new one that moves the core module's stuff up into our hook implementation?
That would pretty much be the same thing, given that the last patch moved them out...
This way will be cleaner in the history too.
Comment #21
alexweber CreditAttribution: alexweber commentedGood call, waiting on the rollback to get on the new fix :)
Comment #22
joachim CreditAttribution: joachim commentedDone.
Comment #23
alexweber CreditAttribution: alexweber commentedHere we go:
Comment #24
joachim CreditAttribution: joachim commentedPerfect, thanks!
git commit -m "Issue #1905750 by alexweber: Added module property to hook_flag_type_info(), and explicitly defined taxonomy term flag type, so that non-required core modules correctly declare dependencies." --author="alexweber "
Comment #25
alexweber CreditAttribution: alexweber commentedAwesome!