Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Reviewing the ctools plugins code and discussing with merlinofchaos, it seems definition hooks are a deprecated means of defining plugins. This patch simplifies some of the logic surrounding loading from hooks and documents them as deprecated.
Just chose an options name at random, if it might conflict or someone has a better one...
Comment | File | Size | Author |
---|---|---|---|
#25 | 711664_optional_global_definiton_hook.patch | 7.4 KB | neclimdul |
#20 | 711664-20_optional_global_definiton_hook.patch | 7.36 KB | alex_b |
#20 | 711664-20_deprecate_global_definiton_hook.patch | 6.4 KB | alex_b |
#19 | 711664_deprecate_global_definiton_hook.patch | 6.39 KB | neclimdul |
#19 | 711664_feeds_test.patch | 789 bytes | neclimdul |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedCommitted to 6.x! Thanks!
Comment #2
yhahn CreditAttribution: yhahn commentedI'm not totally clear on the reason for this change - I can see that if you are using function-based plugins using
hook_ctools_plugin_directory()
is sufficient for loading plugins. But if you are using a class hierarchy for your plugins having a definition hook to define parent/inheritance relationships seems key.What am I missing here?
Comment #3
AntiNSA CreditAttribution: AntiNSA commentedso when you say comitted to that doesnt mean its in the newest version to download or it means it will be in the newest version to download?
Comment #4
neclimdulactually, the parent/inheritance relationship can easily be sorted out be in includes as well. There is not worries that classes will be included out of order since only definition files are included on the first pass, then after the plugin system is initialized it begins properly sorting out hierarchy as defined by the 'parent' key on the requested plugin method(?). There seems to be a bug that's been reported in irc for this patch though that i'm investigating.
Comment #5
alex_b CreditAttribution: alex_b commentedThis patch appears to be breaking Feeds' plugins architecture #724986-7: CTools 6.x HEAD (6.x 1.4) breaks Feeds as with CTools 6.x HEAD plugins aren't loaded as expected anymore.
Where can I find documentation explaining the change in the plugin API?
No doubts about that, but:
Feeds is making use of plugins hooks for defining title, descriptions, short help text per plugin. What are the recommendations for defining this type of meta information when plugins hooks are not supported anymore?
Comment #6
yhahn CreditAttribution: yhahn commentedCan you explain this a bit more? Do you mean that when using class-based plugins each should now have two includes - e.g.
Where the first contains the
$plugin
definition array and the second is the actual class to be included?Comment #7
neclimdulre #5 I'm not sure I completely understand but the title of this issue is a little misleading and closely tied to the ctools internals that build plugins. Feeds is using an old style global hook for defining all its plugins. This is what's being deprecated. Instead its favoring includes. There are still 2 methods for defining plugins in includes, one of which is still a hook. However if you need plugin definitions to be uncached so you can change titles dynamically in the definition hook, it seems like it might be a little better to use a function hook to allow a callback for that functionality. I'm not familiar with the specific issue.
re #6, yes this has actually always been the designed method for usage. I might blog about it and make a demo module at some point(since my module that actually uses these plugins kinda has it hacked in). The difference being it would actually be, using the general ctools plugins folder structure convention:
Where you where calling
and $foo ended up as "myPluginClass"
Hope that helps everyone understand what's happening a little better. Patch with docs to follow.
Comment #8
neclimdulOk, I added some documentation in a couple places that I hope makes the change a little more clear and how to deal with it without drastically changing your module. ie. the 'use hooks' plugin option. Also fixed a bug where the option wasn't working.
Comment #9
neclimdulWith the appropriate the(ir|y're).
Comment #10
yhahn CreditAttribution: yhahn commentedThanks that does clear things up. Would you recommend moving to the include-style structure entirely, i.e. is this actually an indication of the eventual removal of support for the global plugins hook or just a change in default API behavior?
Comment #11
merlinofchaos CreditAttribution: merlinofchaos commentedI will probably not remove the hook completely but I do prefer people do not use it unless they really want to, which is why having it as a flag that defaults to off is a good choice.
So yes, having a .inc and a separate class file is the way to go. This is the direction I intend to go with Views, too, when it finally converts to CTools plugin architecture. This separation has shown itself to be the easiest to maintain, because everything you need for the plugin is in one place. It is also easiest to teach other people how to do, particularly from example.
Comment #12
merlinofchaos CreditAttribution: merlinofchaos commentedThat said, I am contemplating reverting this patch for now, finding out who is using this, getting everyone to update appropriately, and the putting this patch back in. Breaking other people's modules is NOT what I really want to be doing.
Comment #13
alex_b CreditAttribution: alex_b commented#11:
I'm trying to understand: This would mean for each class based plugin in a .class.php file there would be an accompanying .inc file that describes the plugin, correct? For Views wouldn't that mean a very large number of files in already very crowded directories?
Comment #14
merlinofchaos CreditAttribution: merlinofchaos commentedFor Views I would break up the directories by type, much like Panels does. So you'd see:
etc. The hook that describes all the plugins, right now, is kind of unpleasant to maintain.
Comment #15
neclimdulok, after a lot of banging and discussing with merlin i think we're going to roll this back and try again. it obviously didn't go as smoothly as planned. I think we'd like to continue to deprecate the global hook but I need to make sure doing so doesn't break some of the subtleties of the system the way my patch did.
Comment #16
alex_b CreditAttribution: alex_b commentedI can confirm that #15 fixes the compatibility issues between Feeds and CTools 6 HEAD reported in #724986-7: CTools 6.x HEAD (6.x 1.4) breaks Feeds.
I'd be very interested to keep the global hook definition option alive as we maintain at least five modules (off the top of my head: Purl, Context, Spaces, Casetracker, Feeds) that make heavy use of it. It should be possible to neatly wrap the discovery method of plugins into its own function and keep the global hook approach alive.
At least for the time being. But I gotta say - with all due respect for the awesome work on CTools - I have a hard time warming up to one file per plugin definition plus global variables for declaring them.
merlinofchaos/neclimdul: if you need a hand on CTools/plugin work, please hit me up on IRC. Thank you!
Comment #17
merlinofchaos CreditAttribution: merlinofchaos commentedThe variable isn't global, it's actually local, because of the way the file is included.
I won't remove the hook entirely, but I do think it should probably not be the default. I understand people needing to use it.
I've committed the reversion, so we should unbreak stuff. =)
Comment #18
alex_b CreditAttribution: alex_b commented#17: Thanks!
Comment #19
neclimdulOk, here's another shot. I've tested this one a heck of a lot and I don't think it actually changes any behaviors with the 'use hooks' flag on. Also attached patch for Feeds so it can be tested.
Comment #20
alex_b CreditAttribution: alex_b commented#19: I tested these patches with Feeds and I could not find any regressions. Only issue I found was the 'use hooks' flag was populated in _ctools_process_data() where it should be populated in ctools_plugin_get_info(). Enclosed patches fix this minor issue.
I'd love to see global hooks optional instead of deprecated. I attached an alternative patch "optional_global_definiton_hook.patch" that uses "optional" language instead of "deprecated". Functionally, both patches are 100% the same, they include both the minor 'use hooks' flag fix I mentioned above.
Comment #21
alex_b CreditAttribution: alex_b commentedCommitted 711664_feeds_test.patch to feeds as it's compatible w/ CTools 1.3.
Comment #22
yhahn CreditAttribution: yhahn commentedI can confirm that the patch works as intended. I have committed the necessary adjustments to Context, PURL and Spaces.
Comment #23
AntiNSA CreditAttribution: AntiNSA commentedso which should we use to patch, or are you going to release a feeds dev update?
Comment #24
neclimdulThis is not a feeds issue, its a C-Tools issue so lets not steal it for support.
Comment #25
neclimdulRe-sync and update to take care of a minor notice warning that could occur with non-cached plugins.
Comment #26
merlinofchaos CreditAttribution: merlinofchaos commentedCommitted to 6.x; marking for port to 7.x
Comment #27
sdboyer CreditAttribution: sdboyer commentedThis was taken care of in 7 during the sprint. We went back to defaulting to not enabling the hook method, though, so you'll need to explicitly declare it in your plugin type info hook.