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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

Status: Needs review » Patch (to be ported)

Committed to 6.x! Thanks!

yhahn’s picture

I'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?

AntiNSA’s picture

so 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?

neclimdul’s picture

Status: Patch (to be ported) » Needs work

actually, 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.

alex_b’s picture

This 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?

actually, the parent/inheritance relationship can easily be sorted out be in includes as well

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?

yhahn’s picture

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(?)

Can you explain this a bit more? Do you mean that when using class-based plugins each should now have two includes - e.g.

plugin/myplugin.inc
plugin/classes/myplugin_class.inc

Where the first contains the $plugin definition array and the second is the actual class to be included?

neclimdul’s picture

re #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:

plugin/mytype/myplugin.inc
plugin/mytype/myPluginClass.class.php

Where you where calling

$foo = ctools_plugin_load_class('mymod', 'mytype', 'myplugin', 'bar');

and $foo ended up as "myPluginClass"

Hope that helps everyone understand what's happening a little better. Patch with docs to follow.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

Ok, 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.

neclimdul’s picture

With the appropriate the(ir|y're).

yhahn’s picture

Thanks 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?

merlinofchaos’s picture

I 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.

merlinofchaos’s picture

That 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.

alex_b’s picture

#11:

So yes, having a .inc and a separate class file is the way to go.

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?

merlinofchaos’s picture

For Views I would break up the directories by type, much like Panels does. So you'd see:

  plugins/
    display/
       page.inc
       views_plugin_display_page.class.php
    style/
       unformatted.inc
       views_plugin_style_unformatted.class.php
    ...

etc. The hook that describes all the plugins, right now, is kind of unpleasant to maintain.

neclimdul’s picture

ok, 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.

alex_b’s picture

I 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!

merlinofchaos’s picture

The 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. =)

alex_b’s picture

#17: Thanks!

neclimdul’s picture

Ok, 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.

alex_b’s picture

#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.

alex_b’s picture

Committed 711664_feeds_test.patch to feeds as it's compatible w/ CTools 1.3.

yhahn’s picture

I can confirm that the patch works as intended. I have committed the necessary adjustments to Context, PURL and Spaces.

AntiNSA’s picture

so which should we use to patch, or are you going to release a feeds dev update?

neclimdul’s picture

This is not a feeds issue, its a C-Tools issue so lets not steal it for support.

neclimdul’s picture

Title: Deprecate plugins hook definitions » Make global plugins definition hooks optional
Status: Needs review » Reviewed & tested by the community
FileSize
7.4 KB

Re-sync and update to take care of a minor notice warning that could occur with non-cached plugins.

merlinofchaos’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 6.x; marking for port to 7.x

sdboyer’s picture

Status: Patch (to be ported) » Fixed

This 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.