Make contextual links a module

Subscribinationed.

What are the reasons for a module? Contextual links are a standard UI element, and we don't normally make modules for each of these (for example, Vertical Tabs are not a module...)

I think they might actually make more sense as a theme setting. This allows a theme to decide it doesn't want to support displaying them at all, and also allows the site administrator to toggle them on or off for themes that do support them.

@sun - please settle down. your reply has the effect of chilling conversation. David's question is completely on topic.

it is extremely unusual to create a shell module dir in CVS before that module arises. I don't even see a draft of such a module. Perhaps use git/bzr or upload individual files.

We don't need to make contextual links a module in order to enable a 'better_contextual_links' contrib module - we just need to make them alterable. This should be simple if we make them a standard element with a pre-render function, because then hook_element_info_alter() can be used to change any aspect of how they are built. This seems like a good idea to me either way, so I posted a patch for it in a separate issue at #627288: Contextual links are not alterable.

I know a theme setting is a separate issue, but the reason I mentioned it here is that it would also fulfill the goal of allowing them to be disabled. We could have both that and a module, but at some point there are too many different ways to disable the same thing...... Also (unfortunately), looking at http://api.drupal.org/api/function/system_theme_settings/7 and http://api.drupal.org/api/function/_system_rebuild_theme_data/7 it seems like the process for adding a theme setting/feature for an optional module is a bit dicey?

One advantage of making it a separate module is the permission - that does seem to make more sense on the permission screen when it's in its own area, as opposed to stuffed in with everything else in system.module.

system.module really ought to be split up anyway in D8, it's just a big dump of random stuff at the moment, so making this into a module seems like a decent step along the way. It makes as much sense as user, node and field modules being separate modules from system (i.e. a lot, despite those ones being required).

Fixed a couple things.

menu_contextual_links would need documentation updates.

I was surprised by that too, but it seemed that render was still smart enough to still display the contextual links even after their removal from the block.tpl.php template file. Not sure why, I just assumed it was some render magic as it was working, and working quite nicely.

.... I think it's because drupal_render goes through every child element, and only prints it if the #printed variable isn't hit. Since it's not #printed in block.tpl.php, it prints it through drupal_render().

They still display in Garland because Garland's block.tpl.php prints them explicitly. Seven's doesn't, though, which means they won't display at all in Seven with the latest patch.

There's also a problem here in that the $contextual_links variable is not defined when the module is disabled, leading to PHP notices in the template file. Presumably that's why the testbot is reporting 5117 exceptions :)

By the way, I just created #629160: Allow contextual links to be disabled for particular themes and posted a patch there. They are independent issues (and either can theoretically be done without the other), but it is worth crosslinking since they are related.

This is just moving code around, the important place where it's not just a simple move of code is the change from system_preprocess() to contextual_links_preprocess(). When system.module is implementing hooks on behalf of includes, that makes sense, when it's implementing hooks on behalf of itself, then that usually deserves to be its own module.

I fixed a few small issues (which can be seen in the interdiff file), including one that prevented the contextual links from appearing correctly at all... since the library was not being registered for the correct module.

I think this one should probably be good to go.

This still feels to me like as a single UI element, it may be too small of a feature to really belong on the modules page, but it definitely improves the code organization and definitely improves the permission page, so it seems to makes sense. I also realized that the technical conflict between having theme settings for an optional module that I mentioned above is not as bad as I thought - or rather, it probably is as bad as I thought, but it is already affecting the comment module (see #629826: Theme setting toggles for comments appear when the comment module is disabled) so we wouldn't be making things any worse by doing that for this one too :)

Actually let's not delete that function reference from the PHP docs of hook_menu_contextual_links_alter(), but rather replace it with the new function name.

Hm, you are right - it does appear to be the official coding standard now, even though 95% of core is currently violating it :)

I only contributed about 3 lines of code to this patch, and reviewed it while doing so, so I think I can safely mark this RTBC :)

We have a few other issues that have been marked postponed on this one, so I think we want to either get this committed soon (or otherwise un-postpone those...)

Mmm, I want to give this some more thought because I'm not 100% convinced this should live in a stand-alone module. It makes sense from a code perspective, but less so from a feature perspective -- although it is not unreasonable. I'll thinker about this a bit.

I'm also not sure about this. Vertical tabs, collapsible fieldsets, local tasks, autocomplete fields, and other such common UI patterns elements are not disableable. In contrast to things like Dashboard, Toolbar, Shortcuts, etc. which are clearly features, contextual links feels like another such pattern. If contributed modules can't count on them being there, I fear we get back into Views, Zen, etc. all implementing their own different and often incompatible versions of the same thing, which is what that patch was intended to address.

I understand they look ugly as all hell now, but that's what #601150: Improved UI for contextual links is for.

If the desire is to retain the ability to "opt out" of contextual links, then a simple variable seems like it'd be sufficient, if it's not already present.

Would be good to have some other people chime in. Any thoughts folks?

I'd prefer this is split properly in D8. Lots of things could be their own modules from system.module. I'd prefer we have this built-in currently so lots of other things can rely on it.

Yeah I don't see what's wrong with declaring dependencies either, it's not like you're requiring a separate download even.

I also don't get the "contrib has to rely on it so it can't be a module" argument. modules routinely emit blocks and thats an optional module. modules routinely implement search hooks and thats an optional module. module splits are good - less code to audit for security and performance when launching a significant site.

Ok yeah, makes sense.

These are all sound arguments for clean-up and better code organization; however, I don't find any of these really compelling reasons for committing the patch now, over a month after code freeze. If anything, it makes things totally confusing because you need to add a dependencies line for this and you don't for any other UI pattern in core.

Can someone make the case for a critical bug this fixes? If not, this really feels like D8 to me, so we can decide how to deal with all of the baggage that system.module has accumulated over the years in a consistent, holistic way.

But Dashboard is not a UI pattern. It's a feature, and belongs very cleanly in a module. Same with Toolbar and Shortcuts. Overlay's somewhere in between, but probably makes sense as a module, too.

On the other hand, vertical tabs, collapsible fieldsets, progress bars, drag-and-drop table rows, and even regular menu local tasks are UI patterns. Those are simply included with Drupal and module developers can just make use of them directly without having to litter ugly module_exists() calls everywhere or be forced to bloat their dependency list. Local tasks are also a means of navigation that not every site uses, and we provide hook_menu_alter() if site developers want to remove them. I honestly don't see why contextual links are being treated diferently.

We currently have a bunch of developers in this issue arguing about code cleanliness and separation. Since this is fundamentally a UX issue, it'd be nice to have the UX team chime in here as to where they believe contextual links fit in; as a UI pattern akin to vertical tabs, or as a purely optional way to navigate the site which makes sense to be turn-offable by users.

I believe this is a module, purely for the reason that it is exposed in someones theme. Contrary to other UI patterns which are almost primary displayed in the admin area.

I know the code isn't that alike, but from a user point of view, I'd say contextual links are similar to shortcuts, aside from customizability, so if shortcuts are a stand-alone module, it would make sense to me that contextual links are aswel

also, I'm all for splitting up system, right now it's just one big pool, impossible to go trough without a (semi) advanced editor/IDE. It's probably not a problem for most people, but I can imagine less experienced people to be scared by a 3500+ lines file, if all you wanted to do was change some help text...

on the other hand, if we were to continue this pattern throughout other stuff, we might end up with 90% of contrib having dependencies for like 10 core modules but I guess that's not really the problem at hand

I get the feeling that splitting it off into it's own module has more advantages than disadvantages, and I guess the real question is, where do we draw the line?

For me it makes sense to create contextual_links module, because in comparision to, for example vertical tabs, the contextual links are displayed on nearly all pages, likes blocks or the toolbar.

splitting contextual links into a separate module makes sense to me. Similarly to contextual links, the RDF module injects HTML markup in node (teasers and full nodes), comments, terms and has been built as a module (even though it does not have a UI). On the other hand, UI patterns like vertical tabs or collapsible fieldsets have a narrower usage (admin area) and less performance impact.

@webchick (in #41):

Can someone make the case for a critical bug this fixes?

Not a critical bug per se, but as mentioned above, this absolutely cleans up the permissions page. Having the "access contextual links" permission in the System module section looks very weird and out of place, and this totally fixes that...

@Bojhan (in #44):

I believe this is a module, purely for the reason that it is exposed in someones theme

That actually is an argument for #629160: Allow contextual links to be disabled for particular themes rather than this patch. However, there is no reason we can't do both.

**

I was earlier a bit uneasy but still in favor of making this a separate module, and after reading the above comments, that's basically where I still am... However, one part of the patch that I don't really like is lines like this:

-<?php if ($contextual_links): ?>
+<?php if (!empty($contextual_links)): ?>

Perhaps if we do go ahead and split it off, we could still leave one line of code in system.module that defines $contextual_links as an empty array, just so we can remove the above changes and prevent themes from having to deal with that kind of ugliness?

Subscribe.

I'll thinker about this some more. Stay tuned.

Contextual links are, IMO, still problematic. They're not ready to be a "core part of everything Drupal", especially given that they slipped in at the last minute. Cleanly factoring them out to a separate module is a good thing, and makes it easier to remove them entirely if necessary. (And yes, there are cases when you want to; like when they jack up the theme.) So this gets a conceptual +1 from me as well.

[@Crell] Contextual links are, IMO, still problematic. They're not ready to be a "core part of everything Drupal", especially given that they slipped in at the last minute.

How do you define "slipped in at the last minute"? :) The issue went on for months, had hundreds of comments, and had essentially universal agreement that this feature belongs in Drupal core...

[@Crell] and makes it easier to remove them entirely if necessary. (And yes, there are cases when you want to; like when they jack up the theme.)

At the risk of sounding like a broken record, I'll again point to #629160: Allow contextual links to be disabled for particular themes as the proper way to deal with the theme issue. Making it a module is a blunt tool for solving that particular problem.

[@sun] Heck, the current implementation could even be moved into contrib. The only important change for Drupal core is the paradigm shift that has been applied to local tasks.

Hm, not totally sure about that - there is definitely value in having common UI elements in core that sites are encouraged to use (but can replace if necessary). But anyway, I agree this should be a module :)

***

We should get this patch in ASAP. Earlier I suggested to Dries that if there was still some uneasiness about it, one possibility would be to commit it but with required = TRUE in the module's info file....

If left that way, it would still be a big improvement over the current situation, but without any of the perceived risks. Then we could leave this issue open and continue to discuss whether removing that line from the .info file is the right thing to do.

Agreed with David, required = TRUE would be fine if there's more general discomfort about separating it out - and really, now that we hide required modules from admin/config/modules, we should do that with many, many more parts of system module in D8 as part of a more general refactoring. required = TRUE also lets sites which really, really want to kill something hook_system_info_alter() it out, so better than an include.

On 'slipped in at the last minute' - there's a difference between an API which has been in core for months and had time to bed down, and something which was in the queue for months then committed - discussions like whether to use it for comment links or not, and the various performance issues are still ongoing, and we have a very limited window to make any changes now. To me that isn't an argument for making it a module as opposed to the other efforts going on elsewhere but I can understand Crell's caution. Every time we commit new features to core there's followup work to do no matter how long it was in the queue.

required = TRUE seems a bit strange to me. I'm against the idea of making a module required that really isn't required, and that in fact can be turned off (hopefully by #629160: Allow contextual links to be disabled for particular themes). It's just confusing, IMO.

I'd rather it was a module that's required, than a part of system module that's required, if that's the choice we have - since that has zero effect on end-users or module developers whatsoever. Making it an optional module then becomes a one line patch.

I'd rather it was a module that's required, than a part of system module that's required, if that's the choice we have

Why are those our only two choices now? I thought making it required was just an idea that David tossed up.

Based on the conversation, I've decided to commit this patch. It will need another re-roll though.

How about calling this module 'contextual' instead of 'contextual_links'. If called 'contextual_links', it would be the first module in /modules with an underscore.

If called 'contextual_links', it would be the first module in /modules with an underscore.

Uh... field_ui? :)

'contextual' doesn't describe what the module actually does.

Not to mention 'field_sql_storage' which is a real winner :)

Presumably failed because of this line still in there...

+dependencies[] = contextual_links

But I recommend going back to #63 for the reasons listed above.

Sorry, I was sleeping! Is that allowed? ;-) Either way, I tried this patch and it doesn't seem to work. The contextual links are not properly styled.

Per Dries' comment.

No need to put the image in an images subdirectory: modules/contextual/images/cog-select.png. It is OK to put it in modules/contextual/cog-select.png.

OK, committed to CVS HEAD. Thanks.