While Drupal Planet can be a fantastic resource, it can also be a megaphone used when someone has an axe to grind. It's particularly unfortunate when one of those people don't open up comments. Theoretically useful code that comes with an axe to grind should always be suspect to begin with. When the first thing that the author says about the module the code is for is derogatory, it's a pretty good hint to distrust everything that follows. After all, "I don't like this module, and here's some code for this module I don't like," probably suggests that the author would rather you didn't use that module.

That's fine, I don't have a problem with that part. I'm pretty used to people with bad things to say about Panels with varying levels of justification for their words.

However, today Josh Waihi posted an article that contains some code that if you use it, you run the risk of totally breaking your site and having no idea why. I'm completely against people posting code that will break your site, if you use it. I'm especially against people posting code that will break your site with comments turned off, so that no one can actually comment to tell the readers that the posted code is broken.

In particular, this code should never be used:

<?php
ctools_include
('node_view', 'page_manager', 'plugins/tasks');
ctools_include('panel_context', 'panels', 'plugins/task_handlers');
?>

You see, both of those files are plugins. They're specially included files that contain more than just code. They contain an array of variables. Unfortunately, those files can only ever be included once. If they are included once, like this, and then later loaded as a plugin, that array of variables never gets read. And your plugin will just randomly disappear and leave you with an incredibly broken site.

Judging by the code after it, the first line isn't even needed (page_manager_get_task() will load the plugin) and I'm pretty sure the second line isn't needed either, since the get_handlers call should load the second plugin. That said, if you absolutely positively must load a plugin file for some reason (and I often have needed the file loaded to get at functions in it), you have to use ctools_get_plugin().

Also, on a somewhat lighter node, the last line is totally against Drupal code style. You shouldn't use it just because it's a silly way to save 6 keystrokes at the cost of legibility.

Comments

Thanks for the 'heads up' Earl. :)

This is why there's a rule for Planet on http://drupal.org/planet/guidelines that specifically states that comments must be enabled. So I suppose the proper fix for cases like these is to ask the author to enable comments, and if he/she is unresponsive then ask Planet moderators to remove that feed.

Hey Earl. Thanks for the heads up. Hopefully that guy learns from his mistake and enables comments and starts to be a little more positive and open to feedback. I couldn't even get past his first sentence... I think CTools rules! I very glad that you have shared all your hard work with the world; I've used your code on every Drupal project I've ever worked on. Cheers, Jeff

P.S. I contacted the author using their contact form and let them know about this page

What are your thoughts on comment approval? I have recently started to contribute to the issue queue, and our company has recently been added to the Drupal Planet. We have comments turned on with approval needed, but although I regularly check these I would assume that most sites do not and only have it to fulfil the guidelines.

Should approval be allowed? Is open non-approval commenting good for this, so long as it has a CAPTCHA to prevent SPAM?

I've found that none of the automated spam catching techniques are good enough, so I require comment approval so that I don't have to deal with cleaning up spam that slipped through. Instead I deal with approving comments but I generally only bother for a few days around posting something.

Apologies for not having comments enabled - simple access control check was needed.

The reason, I wrote the post was simply because I couldn't find how to do this. I had to find out for myself and decided to post back to the community so that others would have to go through the same trouble.

I'm happy to update my post to make sure its done right.

Just on point on my decision to use ctools_include(): In Page Manager module I had found this: page_manager/page_manager.module:165: ctools_include('page', 'page_manager', 'plugins/tasks');
Which indicated to me that that was how you loaded plugins. (In ctools 7.x-1-1, I can't find ctools_get_plugin(), only ctools_get_plugins() is that the function you're refering to in your post?)

Ah yes, ctools_get_plugins() is the one.

The 'page' plugin might be moderately special and it's possible there's some strange reason it's not fetching that plugin correctly. But I actually believe that's probably a bug from way back in the day before tasks were actually proper plugins. The task plugins, however, use the function notation so they are immune to the problems; it's the panel_context plugin that you can destroy that way.

Also, sorry about posting to planet like this. I totally understand that what you're trying to do is in one of the worst-documented piece of code I have. The rest of what you came up with is pretty okay, (though technically you should probably save the handler not the display, and let the handler save the display -- this only matters, though, if for some reason the handler wasn't in the database.)

I will add though, I *believe* that your call to load the task loads the node_view task plugin anyway, and when you fetch the handlers, that should load all relevant handler plugins. So you shouldn't even need those two lines of code.

Add new comment