Contextual links is not 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.

1) Who said it's a standard UI element? No one.

2) Contextual links are an optional feature. Features need to be able to be completely disabled. In fact, there may well be a better_contextual_links contrib module. Take away the presumptions, please.

3) As mentioned in the OP, further changes to contextual links should happen in follow-ups. A per-theme setting may be worth to explore. But not in here.

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

The last submitted patch failed testing.

Is there are (simple) reason for why my git checkout of HEAD has differrent $Id$ tags than my CVS checkout of HEAD?

The date format is different. And due to that, this patch, which moves an entire file, doesn't apply. Fixed manually for now.

The last submitted patch failed testing.

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.

The last submitted patch failed testing.

menu_contextual_links would need documentation updates.

huh? Why are you removing the template variable rendering?

I'm on crack. Are you, too?

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

Because you did not remove it from the overridden template in the theme?

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: Add a theme setting for contextual links 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.

Re-rolled more easily thanks to http://mikkel.hoegh.org/blog/2009/nov/9/attention-all-drupal-git-mirror-...

This is a straight move of functions and files. Can we go ahead?

The last submitted patch failed testing.

- Fixed template variables.

- Added Contextual links module to default profile.

Tests pass.

Incorporated #13.

Fixed PHPDoc of hook implementations according to new standard.

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.

Implements is the current and ONLY valid coding standard. Please revert.

I'm on crack. Are you, too?

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

yes, please.

Any patches for #601150: Improved UI for contextual links (none exists yet) will break after this one lands.

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.

Did we think about it?

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.

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

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.

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.

And lastly:

For D7, we agreed that the entire concept of contextual links is too young to base any required functionality on it.

Not to mention that we reverted comment administration links from being contextual links due to performance reasons.

This is a "product feature" that may be suitable for 80% of Drupal sites, perhaps only 50%, perhaps only 20%. It would be wrong to do any assumptions for how Drupal is going to be used.

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.

There is nothing to "rely on".

If your module's functionality absolutely needs them, then it ought to do what all modules do that want to rely on something:

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