Allow overriding of links returned by modules

It's not a patch. Developers who wish to be notified of non-patch issues (like me) subscribe to issue notifications explicitly.

I think that adding a database call to the l() function would significantly burden it. I see l() more like a theme function than a logic function, and as often as it's called, this would be one of those "tight loops" that you wouldn't want to have a bunch of slow db calls in. This could be implemented in such a way that it didn't really affect performance if it could be disabled, so I wouldn't be completely against it on those grounds alone.

That said, I would like the functionality of overriding what modules display, but couldn't this be performed wholesale at the filter level? I would be more inclined to want a filter mode that was applied to fully rendered nodes, as opposed to just the body content. I'm guessing that a few regexps applied to a rendered node would be faster than putting db code in l(), but that would vary with the implementation/situation.

This sort of feature is worth consideration (however it's implemented), but it would require the coordination of module developers, and would be very hard to do when links are completely dynamic (both url and text). If you were developing a site where functionality had far greater precedence over performance, this would be nice to have. Consider, however, the situation where you want a link to appear in one place, but not another. Perhaps (in your opinion) a link is crudely misplaced on one page, but not another...would each url location have a unique id? Would you associate a page where a translation should be applied? A set of pages?

I think it could be argued that the theme system should take care of these display issues, and a few drupal versions ago, I would agree with that. Now, xtemplate (or phptemplate, smarty, etc) typically abstracts theme details and tends to rely on whatever default format the module provides. To override a module's theme also requires that you know PHP or are a good motivator :)

A train yourney, an old battered laptop, Vim, and a few days old CVS checkout.

Here is a completely untested patch (read: pseudo-codeish) that puts a first step towards fully themable _links links.

Bèr

Forgot to mention that I won't have time to maintain or improve this patch, so either ignore it or take it over/improve it :)

Afte a small thread on the develope mailinlist, I strongly feel i've made a wrong choice.

I went for keyed arrays, but in programming land there is a nicer solution for this: objects. We use that a lot already, yet we use it inconsistent. To move the balance a bit more towards consistency I think we must go for objects in this case.

So not:
$links['book_addchild'] = array('title' => 'foo');
but rather
$links['book_addchild']->title = 'foo';

I hope someone has a nice replace pattern for that? :p

Some pattern replacing, and some handediting and viola, objects.

IMO it looks a lot nicer now. But It just feels better: returning objects, as it should, for structured data.

Wow, this really looks nice.

Shouldn't 'entities' be 'attributes'? Eg.

$links['comment_add']->entities = array('title' => t('Add a new comment to this page.'));

Correct. Also changed attachEments into attachments

The approach of this patch is good, it only breaks other calls to theme('links')..
Chx promissed to look into this and patch this once again, so Dries could commit it...

This would solve a long standing issue (from early drupal 3), which makes link management a lot easier..

I very much prefer this implementation over the current one. It is also consistent with the new forms api.

A newer patch.

Primary and secondary links no longer work! This patch breaks them completely. But.... I first want to see where the primary links go and how they are rendered. Therefore this patch will have to wayt for the primary links issue, unfortunately.

oh. Forgot to ention that with this patch, I introduced a way to (finally!) add offsite/absolute links. http://www.something.com is now possible in theme_links()

Can some PHP performance guru look at this code? I want to know if my solution is good enough.

Chx, are you still planning to have a look on this?
Now the primary/secondary links are landed in core we could finish this hopefully..

I am very, very curious about what your plans are for this..

Now the primary/secundary links are stabilised I once more updated this patch..

It makes all the links returned by hook_link() return structured data, where theme_links() builds these links.
These way we get a solid base to build a link management module or something in that direction how people could manage their links..

Please review, comment and commit... ;-)

To be more precisly, this patch will allow you to show/hide all (node)links on your pages!
Isn't this where we are hoping for since drupal 3??

I did some serious testing, and this works perfectly for me..

Please review this patch and set to 'Ready to be committed' if you think it _is_.

I'm goin g to +1 this

Although I personally think using associative arrays is cleaner :
Instead of :

     $links['taxonomy_term_'. $tid]->title = $term->name;
     $links['taxonomy_term_'. $tid]->link = "taxonomy/term/$term->tid"; 

Do this :

    $links['taxonomy_term_id' . $tid] = array('title' => $term->name, 'link' => 'taxonomy/term/$term->tid'); 

or if you want to be anal about indentation :

    $links['taxonomy_term_id' . $tid] = array(
       'title' => $term->name, 
       'link' => 'taxonomy/term/$term->tid'
    );

Plus. it brings it into line with the forms api and the menu api.

+1 for the patch
and
+1 for adrians idea
Because:

Plus. it brings it into line with the forms api and the menu api.

A big +1 for the patch.

A -1 on Adrians Idea. But I think we should *not* let this patch die on the debate wether or not objects should be in ->objects or in associated arrays. Untill we have that in the coding guidelines (As of yet Dries has not given his idea on what the standard should be) we should not bother.

This patch might be out of line with the menus and forms, it is in line with users, nodes (APIs), comments, feeditems, terms et al. So id even say that menus and forms are out of line with the mayority of Drupal.

But agian: let us not get this debate to hol up this path. I am merely putting these ideas in here, to provide this patch from being held up because of the object vs array thing.

The problem with adrians approach is that the new primary/secundary links will break once again and needs to be rewritten once again.
IMO it is too much for just this feature, though I'm not against it todo this while the development cycle after 4.7 is released..

So, for now use the object method, after that we should change the method to arrays...

Steef

I did test this patch (on 3 different sites) intensively and did not found any error at all, so setting status to "Ready to be committed"..

Dries, Steven, what's wrong with this patch??

What do the others think about http://drupal.org/node/18260#comment-53582?

Who will document this change when this patch gets committed?

I love this patch (and happen to know that Gerhard does, too).

@Dries, what do you mean with documenting the change with regards to #18? Or did you comment on them separately?

Anyhow, I will write down some docs in the upgrade page.
And I already commented on what i think about #18: that should *not* be a reason to hold this patch any longer, IMHO. Because $objects->var is still used more often in drupal core then the arrays, and because there are no guidelines/standards on this yet. We should decide on what to start using as default *outside* of this issue and this patch.

Ber

I agree with Ber, and Karoly is right about me liking this patch.

nice patch. a couple nits:

- $link->fraction is not correct terminology. you mean $link->fragment
- I think $link->href is more descriptive than $link->link but no big deal.

I think Moshe s variable names are much better. I pattern replaced the patch. tested and it works.

Is anything holding this patch? This simple patch is one the most wanted features in Drupal, yet it is on hold somehow?

If it needs something, Dries, Steven, please, please say so.

it does not work anymore with head...

work is the false word
I cant patch anymore because of changes in tasonomy.module and comment.module

yes it will break on https. I took this literally out of the code we had for primary links. This does not introduce a new bug, but only redo an already existing one :) Feel free to fix

I have to leave this issue for what it is; Again I already spent 3 times the allocated time on it. It is a VERY badly needed patch, but if people are not spending time on it, then so be it. Then let it die here.

Indeed that this issue is badly needed, and for a very long time..
Unfortunatly we are not getting any usefull feedback from the core committers how we should improve the patch so it good enough to commit it..

My final try: Dries or Steven, what is wrong with this patch?

+100.000.000
i tested it, like it and i want it to get commited!

Disclaimer: I do not have time to do this and haven't actually looked at what the patch actually does. :P

But, in terms of https support:

if ($link->href && substr($link->link, 0, 7) == 'http://')

Could you not just change this line to:

if ($link->href && (substr($link->link, 0, 7) == 'http://') || substr($link->link, 0, 8) == 'https://'))

I don't know what the exact problem is, (probably the problem is so big, this patch isn't being committed), but maybe if I think I know what the problem is, this will be even better:

if ($link->href && (substr($link->link, 0, 7) == 'http://') || (substr($link->link, 0, 8) == 'https://') || (substr($link->link, 0, 6) == 'ftp://') || (substr($link->link, 0, 9) == 'mailto://'))

I this the solution we are searching?

Loose

+      if ($link->href && substr($link->link, 0, 7) == 'http://') {
+        $output[] = '<a href="'. check_url($link->href) .'"'. drupal_attributes($link->attributes) .'>'. check_plain($link->text) .'</a>';
+      }

I searched on http in the patch and this feature is not used.

Rerolled (did not apply any more) and simplified theme_links significantly.

-1 on the simplified version.

We lost the ability to post external links in primary links, we now also loose the ability to add outgoing links in the hook links.
Weblinks module, node aggregator module, links.module etcetc all loose the ability to use that hook links when this simplified version hits core.

BUT: i think this patch should be committed anyway. We can sort out those rare cases of external links afterwards.

Now you -1 or +1 my patch???

I would absolutely +1 a patch that kept the ability to post external links
I +1 a patch that fixes this long standing hooklinks issue, even if that one does not allow us to use external links.

If we have to choose between a patch that allows us to use external links and a simpler one that does not i will choose the first. My previous -1 referred to this patch vs the older one that allowed external links.

conclusion, a +1, be it with hesitation.

I'm with Bèr on this one. Most of the links on sites I maintain are external, often through modules like weblinks and aggregator. Let's go with the non-simplified patch now.

• I still do not think this is the place to handle ext. links
• There is no non-simplified patch now as the last such do not apply, so those need that should roll one.

In essence I agree with you chx, but since we have no such place *yet* we should not break:
* weblinks.module
* relatedlinks.module
* links.module
* naggregator.module

and probably more, but we should at least allow them to pass plain HTML and a full url.

I'm willing to update this issue a couple of times more, but then I *must* know what I need to change..
Dries or Steven my last cry for some comments on this please..

Otherwise I'll mark this as "Postponed" or "Won't Fix"..

dries and steven,
I think I speak for all of us:
we are willing to write a link management module as soon as this feature is in head (if wanted)

dries posted an answer for this topic
but in the false issue
http://drupal.org/node/38981#comment-57399

Yes, I posted to the wrong issue. Here is what I said:

I prefer the simplified version made by chx but I'm not 100% happy with the keys used in the links array.

1. Some are really cryptic like blog_usernames_blog, comment_comments or comment_comments_new.
2. Some keys are dynamic while other keys are not:
   

   +        $links['taxonomy_term_'. $tid]->title = $term->name;<br />
   
   +        $links['taxonomy_term_'. $tid]->href = &quot;taxonomy/term/$term->tid

@Dries
The keys are built up as follows:
modulename_unique_identifier
That is a naming convention, to avoid conflicting IDs and to allow nice preg matching and so.

The flipside of that namingconvention is that we can have oddly named IDS. They are theere for usabilty and themability, not for readability of code.

Keeping up with HEAD.

i did a code review and this latest patch looks good to me.

Won't apply.

Changed nothing, only updated to latest head, rerolled & tested. So I am setting this 'ready to be committed'.

Please, please, please, raise your voice for this patch. Its not only one of the longest standing issues in Drupal, it also fixes one of the most FA-edQ wrt themeing. *anyone* whoe has *anything* to do with tweaking Drupal to its needs needs this patch badly.

I really don't like using ->href... it suggests it's an URL similar to <a href="">, which is flat out wrong. l() and url() take Drupal menu paths and turn those into a valid URL (relative or absolute).

well, i /was/ a real HREF untill after #41. Which I -1ed!

We now loose the ability to add absolute links, thus breaking
* node aggregator
* related links
* weblinks
* links
* service links

and prolly more i forgot. however, it has been Decided So By The Higher Drupal Gods That we no longer need absolute urls. we should live with it! The naming can be changed by a search replace, if anyone fees like it.

An updated patch.

since drupal now allows url to have absolute urls (w000t) I kept the name href. Path is no longer a correct word to use, since we can now provide anything that goes in the href.
->url would be too limited too, for nothing prohibits a module developer from adding some javascript in there.

Ber

href is more descriptive of how the value is actually used, so I think it's safe to leave that way. Patch applies cleanly with only one offset. +1.

This is very nice indeed. I'm tinkering with a module that populates a sidebar block with specific links (if they're present) instead of keeping them in the node links line, and this makes the process loads easier. Three cheers.

+1

Haven't heard from Stefan in a while, assigning this to myself.

A concern:
Should we still consider this patch for 4.7? I mean, its an issue that is gong to break all themes and load of modules *again*.

Ber, why don't we simply pull the trigger and close this issue or mark it "won't fix"? We updated this *a lot* of times, and still there is noone around who wants to commit this..

Dries or Steven do not even care about checking it once again.. My time is too expensive to keep this up with HEAD forever..

This is going to take way to long for me, so I'm out...

@stefan. You are probably right. But I decided to play it nasty :) Just bump it daily, or maybe a few times a day. Then one day, people will get very annoyed by receiving this issue in their mailbox over and over, and either mark it won't fix, or commit it :)

sidenote: Maybe we should have another patch status called "I give up" :) .

I believe url() supports absolute links now.

Patch looks good, but I'm afraid it will have to wait for 4.8. It is too late in the game now. I don't feel like breaking contributed modules again.

and another patch.

I wasn't able to find this issue and, a few days ago, I opened feature request to add the ability to sort links.

Maybe the best solution would be to implement something in such a way that modules publish links and then, the site admin can choose which to show, the order, etc. Like menus or blocks?

Sorry to hijack the issue, I just thought it is somehow related. Maybe it worths to explore the possibility...

I don't think it's exactly hijacking the issue.

Adding $links['mymod']->weight = 5 is an obvious extension of this patch.

Too bad this patch won't make 4.7. Updating a module to support this change seems trivial. It shouldn't be that difficult to make a patch for most of the contrib modules.

Personally I am getting more and more dissapointed by the actual "frozenness" of HEAD. So if nothing changes in the current behaviour, why should this issue wait?

I will review the status in a week time, if nothing has changed in 4.7, if more features made it in, then I will revive this and I hope that tangent et al will help me make noise around this.

Bèr

The patch did not apply cleanly anymore. Can someone look into that?

Disclaimer: I haven't tried to install the patch, but there seems to be an important usability feature which is overlooked:

Aren't you tired of seeing the same crowded list of links in the node teasers list as in the node details page?

From an usability point of view, we must allow to decide which links will be displayed only when the full node is displayed and which links will also be displayed in the node teasers list. This is essential since the list of links is already too crowded in the nodes list, but the full list of links is badly needed in the bottom of full nodes.

Amnon
-
Personal: Bring Dolphin's Simple Joy to your Work - Job - Career
Professional: Small business web hosting strategies, Drupal Hebrew Hosting

New revised patch.
For BZR: see also here: https://launchpad.net/people/ber/+branch/drupal/structuredhooklink

New patch for HEAD.

I switched this to be consistent with forms API, makes a lot more sense and is cleaner. We've all seen the power for form_alter(), this should hopefully open up the floodgates for the same sort of power when dealing with links.

Oops forgot settings.php, here's the real patch.

Here's an updated patch which introduces the new hook_link_alter() ... works in similar fashion to hook_form_alter(), now it's super easy at the module level to interact with other links. The $node object and array of $links is passed in.

Note, all calls to theme('link') are broken with this page (and as it was before I resurrected this patch). Cleanup to those calls to follow once this patch is ready to go.

After talking with both Adrian and Moshe about this, there seems to be a slight issue. Whether or not to use hook_link_alter(). Currently, we would need such a hook to easily alter links at the module level (*not* the theme level). However, we could change the order of function calls and make it so that nodeapi('view") could be used to alter links since they would be in arrays.

Here's the current code with proposed new hook:

  // Allow modules to change $node->body before viewing.
  node_invoke_nodeapi($node, 'view', $teaser, $page);
  if ($links) {
    $node->links = module_invoke_all('link', 'node', $node, !$page);
    
    foreach (module_implements('link_alter') AS $module) {
      $function = $module .'_link_alter';
      $function($node, $node->links); 
    }    

nodeapi('view') is called first, then hook_link, then hook_alter_link. This works great.

Now if we changed the order of this a bit, we could get the module links first and then send these to the nodeapi('view') where one could easily change the links around.

The problem with this, is links aren't only on nodes, but they are on comments too. This patch addresses this by calling hook_link_alter() for comments too. The advantage of this is you can use one hook_link_alter() to modify links on both nodes and comments, insetad of using nodeapi('view') and comment_view(), which could provide some logic updates but doesn't require an extra hook.

I'm in favor of hook_link_alter() hence the patch :-)

Note, hook_link_alter() is sent both the $node object and the $links array. In the case of $node links, we pass in both $node and $node->links. This could be thought of as repetitive, however, for comments, these are completely seperate, $node and $links, so for consistency and less confusion and the "allusion" that comments are nodes, I chose this distinction :-)

We should then eliminate the following function from taxonomy_term_path() and its use in the forum module:

function taxonomy_term_path($term) {
  $vocabulary = taxonomy_get_vocabulary($term->vid);
  if ($vocabulary->module != 'taxonomy' && $path = module_invoke($vocabulary->module, 'term_path', $term)) {
    return $path;
  }
  return 'taxonomy/term/'. $term->tid;
}

Dries great catch. Removing that function gives hook_link_alter() even more power, as now it can override taxonomy term links for each node too, as shown by forum.module.

Updated patch. After this review/RTC, I'll work on a patch that fixes the calls to theme_link() so they can send in structured links. Want to keep this small and managable.

I removed all whitespace at the end of the lines from the patch.

Looks good to me. I'll send an e-mail to the mailing list so we can move this forward ASAP.

Right, as I said this patch changes the functionality and breaks things in a few places. There are still a few calls to theme('link') that need to be changed.

I left those out of the patch to keep this clean and easy to understand.

I'll roll another patch that fixes a handful of those spread throughout core.

Here's an updated patch that fixes the rest of calls to theme('links') in core.

Also, menu_primary_links() and menu_secondary_links() now return structured arrays. This allows themes to add classes to menu items with easy (closes 3 different issues).

In terms of what this patch breaks, not much. All hook_link() calls are broken and themes that use menu_*_links() will need to be updated. This will take all of 30 seconds for modules/themes to come into compliance. Many themes and modules are *not* affected.

i'd likew to see a class= added autgomatically by theme('links') oir wherever these links are used. this would make it very easy to build a stylesheet for all themes that adds icons for every kind of link. we could only add the class if no classes are already provided in the link array but i see no real reason for that special case.

my feature request need not hold up this patch.

Moshe, I agree.

However, adding a class isn't the best option. Often, you may want to add a comment icon to "add comment" and a little page icon to "read more". Using a class won't fix this.

Adding an ID would.

Perhaps, we should be adding an id=link-name to this patch as well? It's quite simple to add and it limit the extra work themers would need if they wanted to add icons.

Of course, they have full control with this patch to take out IDs/classes/links/titles etc.

i dont like IDs at all.
what if you print more than one node on a page?
--> id conflict
I think this is possible with classes too
whats against class="link-item link-item-linkname"?
maybe link-item to give a default style (mybe not necessary)
and link-item-foo for the specific link item...

imho the name has to be clear so that we avoid namespace conflicts (users theme vs. drupal vs. modules)

Err sorry, I misunderstood what Moshe was saying, disregard all comments about IDs. Haven't had my 2nd cup of tea yet today ;-)

Here's a new and hopefully final patch. Now automatically adds class="module-link-name" to all links (appending as necessary).

Also, I fix a tiny issue with menu_primary_links() returning duplicate keys, which get translated into duplicate classes in wrong places, so I appended parent ID to make those unique.

Better patch without a few sneaky tabs in there.

Overall, this patch looks great. But:

1. I have very mixed feelings about the optional $theme argument in the menu_item_link() function. IMO a function should either return structured data, or themed output - not both. However, right now I can't think of a better alternative for this particular situation. We could get rid of menu_item_link() altogether, and put everything in theme_menu_item_link() - but then the special code that needs just the raw processed array would have to duplicate some code itself. Or we could simply make menu_item_link() return only structured data - but then almost every bit of code that calls menu_item_link() would need to manually pass its return value on to theme_menu_item_link() - which would be 2 LOC instead of 1 in heaps of places. If anyone has an answer to this, that involves removing the $themeargument, I'd love to hear it.

2. The new foreach in taxonomy_term_path() should have its 'as' keyword in lowercase.

3. Just wondering - various people have now mentioned that they want to write a "links management module". What exactly will this be? I'm guessing that such a module would be similar to the locale module, the path module, and the menu module - only for links. That is, it would allow users to override any attributes of a link (or do other special things, e.g. disable a link) using a nice user-friendly interface. If my guess is correct, then does the hook_link_alter() in this patch make such a module possible? The new hook looks good, and it can clearly facilitate overriding of links by other modules - but can it also facilitate overriding of links in a user-defined way? Since user-defined links will be managed by a module, what I'm really asking is: can it handle multiple modules trying to override a link? E.g. a hypothetical link.module with a user-defined override for a taxonomy link in a forum topic, which has already been overridden by forum_link_alter(). Or will we need the approach of menu.inc, which includes custom code to cater for links that are user-defined through the menu module?

Jaza, yes I agree about the optional $theme argument, however, I could not come up with a better solution that didn't break things around.

I've always thought AS should be capitalized?

hook_link_alter() should work like hook_form_alter(). Multiple modules can use this hook and override others. It just becomes a race because they are called in alphabetical order, something that can be offset when using the module weight.

AS is only capitalized in SQL, not PHP code.

Cool, good to know!

Updated patch.

Committed to HEAD. Please mark this 'fixed' as soon the documentation has been updated/extended. Thanks! Great job, guys.

Ok I've created a new page for upgrading from 4.7 to 4.8: http://drupal.org/node/64279

Do we need docs anywhere else?

Just updated the developer docs on drupal-contrib with the new hook.

The module upgrade docs have officially passed webchick's sniff test. ;) I published them.

There are two other places I can think of:

Page.tpl.php - has a sample PHPTemplate theme - but no point in updating this now imo when:
1. It will be the reference page of choice for 4.7 themers for the next X months
2. There are bound to be many other theme changes between now and when 4.8 is released.

Therefore, I recommend deferring re-writing this page until closer to 4.8.

I've also added Converting 4.7 themes to 4.8 with a subset of information from Ted's original updating modules post (no sense in scaring the poor themers half to death with the details, imo ;))

Marking this issue fixed.

Chameleon is now horribly broken.

I think its time to port chameleon to phptemplate
http://drupal.org/node/41005

Broken chameleon sounds like a bug, not a feature. :)

Hmmm testing it as of today, it does seem that broken, only a few links here and there. Attached is a patch that fixes this.

Committed to HEAD.

This is broken - if you have the forum module enabled, *all* taxonomy terms stored in phptemplate's $terms (itself generated by taxonomy_link('taxonomy terms', $node)) turn into forum/$tid, regardless of whether the term is a forum term or not.

Here's a patch that only modifies the taxonomy/term/$tid link if the $tid in question belongs to the Forums vocabulary.

It looks as if it would work, but it also looks as if would add a query per term...

I think the API documentation needs to be updated:
http://api.drupal.org/api/HEAD/function/hook_link

For example the image module is currently broken as it uses those l() links in hook_link.

sorry didn't intend to change status.

Updated docs, thanks Thomas!

Morbus, your patch looks ok. You're right though, that is one tricky instance I must have overlooked :-) Not sure if there is a better way than what Morbus suggests.

Morbus' patch is a little bit ugly but I committed it nonetheless. Can't think of a better solution. If we can clean up the system, by all means, but it can be in a new patch. Marking as fixed.