Posted by jonathan1055 on August 2, 2009 at 7:03pm
3 followers
Jump to:
| Project: | Web Links |
| Version: | 6.x-2.1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Hi,
I have made two changes to the function weblinks_link() which I would like you to consider committing into the latest dev release.
- When showing a full page of a specific users own links via weblinks/user/n, I think the admin would still like to have the edit & delete links to be shown. Currently the function exits early when deciding whether to add the "Username's links" link, so the later code to add the edit and delete links for Admin never gets reached. Instead of exiting, I have changed the logic so that the processing can continue, which therefore allows the rest of the links to be added. I changed:
<?php
// We don't want the link if we are asking for a specific user.
if (arg(1) == 'user' && arg(2) == $object->uid) {
return $links; // <-- this means nothing else gets done in the function
}
if ($user_link && $object->uid != $user->uid) {
... code to add "Username's Links" here ...
}
?>
into<?php
// Add "username's links" if we are not showing a specific user, and the link is not the user's own link.
if ($user_link && $object->uid != $user->uid && !(arg(1) == 'user' && arg(2) == $object->uid)) {
... code to add "Username's Links" here ...
}
?> -
The code generated for "Username's Links" actually doubles up the href tag, because in most themes I have seen theme('username') creates a link to the users profile, which then that gets another href tag added via the return from weblink's hook_link. The generated code in Garland looks like:
<li class="weblinks-user-link first"><a href="/mysite/weblinks/user/4"><a href="/mysite/user/4" title="View user profile.">Jonathan</a>'s links</a></li>
The link still works but the html is broken and it is only users name which is the active part of the link, and the " 's links" bit is shown as plain text. This behaviour may change with different browsers. The solution is not to call theme('username') but just pass the $object->name directly into the text for the link. This results in the desired link to that users weblinks. Changing from !name to @name automatically runs check_plain so this does not need to be called.
Hence the code:<?php
$links['weblinks-user-link'] = array(
'title' => t("!name's links", array('!name' => theme('username', $object, array('plain' => TRUE)))),
'href' => 'weblinks/user/'. $object->uid,
);
?>
becomes<?php
$links['weblinks-user-link'] = array(
'title' => t("@name's links", array('@name' => $object->name)),
'href' => 'weblinks/user/'. $object->uid,
);
?>The attached patch for 6.x-2.1 make these changes.
Jonathan
| Attachment | Size |
|---|---|
| _weblinks.user_links.patch.txt | 1.02 KB |
Comments
#1
It is nearly impossible to track multiple problem resolutions in a single DO issue, so that's why virtually all maintainers ask that you put them in separate issues.
Let me deal with #2 first. There is a move even within core to allow alternate display names for users rather than displaying login names. There is also the Real Name module that does this in D5 and D6. The "proper" way to access user names is with theme('username',...). Now I realize that currently even much of core fails (even the user module doesn't do it right everywhere), but there is also another issue to fix this in core. The
array('plain' => TRUE)would cause properly coded implementations of that theme to produce plain text rather than a link; unfortunately, the current core implementation is one of those that doesn't yet recognize this (RealName does). So if we just pick up the name from the object, all those sites that don't want log in names displayed (including several of mine) will break. The only way I can see to fix this would be to add special code to see if theme('username',...) returns a link and, if so, replace it with plain text of some kind.As for #1, there was a conscious decision to bypass those links on a specific user's page. I wish I could remember why. It may be that we didn't want them on the user/xxx tab, which I think uses the same code.
Robert, how would you like to proceed on these issues?
#2
Firstly, sorry that I posted two issues in one. The actual code changes were nested one inside the other, so making a patch for the whole change seemed the obvious thing. I get your point though.
For (1) the scenario I was considering is this: The site admin has set weblink titles to go to the actual website not the drupal node for the link, and has created a role 'weblinks moderator' which has the weblinks permissions you have defined, but not general drupal admin authority. When the moderator finds an unsuitable link and goes to the page showing all the links posted by that user, if they do not get the edit/delete links at the base of each link then they have to trace back to find that link from the main page before they can edit or delete it. Having the admin links makes it much easier. However, I am happy to hear your reasons for not wanting the links.
For (2) I was not aware that there is a move to allow alternative display of usernames, so if this is controlled by theme('username') then of course we should call it. Have you reported the bug that using plain=>TRUE does not work as expected?
Jonathan
#3
When I have intermixed patches, I just say "the patch is included in the patch for issue #...." Not a big deal.
1) I understand and hope Robert will weigh in on this. I really can't remember what the reasons were and obviously didn't document them.
2) Actually theme('username', $object, array('plain' => TRUE)) is working correctly if you have no module that properly services that theme function. The only module I know that currently provides this functionality is RealName. There is some hope that 7.x core will, but don't hold your breath.
#4
1) Was included before the admin links were added and it was to prevent the links for showing links to stuff people didn't have access to. I think his fix should be fine.
2) Is theme('username', $object, array('plain' => TRUE)) what is actually causing the ugly HTML or not? If it is a case that the user module isn't supporting it right can we override that well still allowing modules like realname to override it?
Thanks
Robert
#5
1) Okay, thanks.
2) It is true that the user module is not handling this correctly, but then it doesn't even use its own theme function. There are several core issues on this, I'm just too lazy to go look up the numbers (I think there is a link on the RealName project page). I guess I need to look at a more complex solution that doesn't cause this bad HTML. (Johnathan, in case you didn't realize, I created RealName, so I am using it.)
#6
Ahhh -- the reason why the function quits at that point is because we didn't want to see a who bunch of "XXX's links" on weblinks/user/xxx. The edit and delete links are just victims of being later in the game so they were placed after that. All I have to do is move the code above that check.... Duh.
#7
Committed to 6.x-2.x-dev.
#8
Nice simple fix for the theme(user) html problem, point (2). Thanks.
Yes had I worked out that you did not want 'usernames links' on the /user/nn page. What I was questioning was why the early exit from the function, instead of adding the user links within a conditional block after testing for the positive reason to add the links. This is better than an early exit because it (a) shows more clearly the intention of the programmer (b) allows new code to be added after which won't be affected and (c) allows this code to be moved by a future maintainer who may not realise the significance of its position at the end of the function. You already fell into this trap by your explanation in #6 above.
So the condition code for the block and the accompanying comment as in point (1) in my original post are still valid in the patch given, and I'd like you to consider putting them in. I can re-roll a new patch baed on your latest dev release if you'd like me to.
Thanks
Jonathan
#9
Okay, it took me a while, but I finally realized you're just simplifying the code. Committed.
#10