The URL alias admin page at admin/build/path offers Edit and Delete for aliases, but no equivalent to View, which would mean turning the displayed URL (alias or source) into a link.

The suggested patch adds that feature to the "System" column.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Needs review » Needs work

There is no reason to do the check_plain for the text since the l function[1] does that already.

[1] http://api.drupal.org/api/function/l/7

catch’s picture

Category: feature » task
Status: Needs work » Reviewed & tested by the community

@Earnie: the check_plain is completely outside the l() function and not introduced by the patch.

I can't believe this feature was never there before. This is lovely. Tested works. RTBC.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

As far as I can see, Earnie is right, there is a superfluous check_plain() that should be removed because already in the added l() call.

catch’s picture

Whoops sorry. Clearly I can't read :(

fgm’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

Changed. I had left it because it was there already, but it is indeed superfluous.

Beyond this, should the link be on the canonical path, the alias, or both ? I currently added it only to the canonical path.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Damien Tournoud’s picture

Beyond this, should the link be on the canonical path, the alias, or both ? I currently added it only to the canonical path.

It doesn't matter: it will get canonicalized by url() anyway.

The patch looks good to me, too.

fgm’s picture

I was asking from a usability perspective, of course: what is the best for the user of that page. I think it is the current choice (the second colum), but others might disagree (first column, or both).

Dries’s picture

FileSize
9.49 KB

On my screen, the Operations column is not visually pleasing -- it is too spread out (see screenshot). Adding a 'visit'-link might make it look better and might make it more explicit and thus easier to spot?

fgm’s picture

FileSize
1.06 KB

New patch based on Dries' suggestion, but with "visit" changed to "view", in order to stay in line with current practice in core.

This being said, I tend to find aliases being typically much longer than the ones in this example, meaning links are not usually as much spaced as in the screenshot, and adding one column for this operation seems to add some unneeded clutter. Maybe we should rather achieve a similar spacing reduction without by not forcing the table width to 100%, but then this would be theme-dependent.

Maybe at some point it would be a good idea to include a set of standard icons for standard operations throughout core, like View/Edit/Delete and Move Top/Move Up/Move Down/Move Bottom (à la Views UI) ?

catch’s picture

Status: Reviewed & tested by the community » Needs review

fgm's right - I've had url alias tables break some themes with long aliases (i.e. bleeding into the right sidebar and non-clickable ops). So I'd prefer #5 to conserve space for those situations, and because the width of the table doesn't have much to do with this patch IMO, more a Garland issue.

Anonymous’s picture

What Catch said. The link is enough to represent viewing the page and I too believe that Dries' Operations issue is more a theme issue.

Dries’s picture

A possible solution for the long URLs is to truncate them as we usually do. As it is related to this patch, it would be good if we could tack this onto the current patch. It would probably be handy to see screenshots of both approaches with a real-world alias table.

fgm’s picture

The problem with URL truncation is twofold:
- we don't know when preparing the page how wide the actual display area is, since it depends on the theme layout, the font size, and the browser window size. We might end up uselessly truncating paths to unusable widths and leaving large empty areas for sysadmins with wide screens, while still not truncating enough for some users with small displays like an EeePC. It seems to me this is an issue which can only be handled by client-side JS, where this information is available. And as such, it seems to belong more in the theme than in path.module. Do we really want to add JS to this module ?
- I think we do not (yet) have a standard ellipsize/truncate library function, either in JS or PHP, like the one in Pango, for instance. If we want to start handling screen fitting of strings, we will need one, won't we ?

catch’s picture

Also - if I have a url like example.com/foo/bar/baz/123 and example.com/foo/bar/baz/1234 and they both get truncated to example.com/foo/bar/baz... then that screen becomes a lot less useful for administering paths IMO.

swentel’s picture

Status: Needs review » Needs work

If locale module is enabled, another column is added with language, and the delete operation isn't renderend anymore after this patch.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

New patch which fixes colspans when locale module is enabled.

lilou’s picture

FileSize
2.02 KB

Re-roll.

meba’s picture

Seems OK to me. Still applies and works.

lilou’s picture

FileSize
1.99 KB

Remove windows line breaks.

ultimateboy’s picture

FileSize
38.57 KB

Tested patch. Appears to work without a hitch.

Attached updated screenshot to account for changes.

yoroy’s picture

The 'view' operation is inconsistent with how similar screens in core do this. They link the title itself for viewing.

Tracker:

Content listing:

Comments:

I'd prefer doing same here.

agentrickard’s picture

FileSize
1.1 KB

I agree with yoroy.

One-line patch attached. Turns both the alias and the src into links -- both of which should point to the aliased path.

swentel’s picture

Status: Needs review » Needs work

Initial patch didn't get in, so last patch misses some lines (especially when local is enabled)

agentrickard’s picture

Whatever. I'm out.

sun’s picture

Status: Needs work » Needs review
FileSize
7.63 KB
7.47 KB
1.06 KB

ff1’s picture

We need to also think about new users with this patch. I know that when I created my first url alias, I wanted to test it out the make sure it worked, so I think it would be useful if both paths were linked.

The attached patch adds the link to both the system path and the alias.

alexanderpas’s picture

I would even say, remove the link from the system path... opinions?

sun’s picture

Status: Needs review » Needs work

If we link both, we can add a sanity check here: The alias link should _show_ the alias, but use src (not dst) as target path. So, by hovering over the link you can immediately see whether Drupal thinks that the internal path really translates into the displayed alias.

Oww... couldn't we even add a CSS class then to highlight all aliases that are obsolete?

sun’s picture

Status: Needs work » Needs review
FileSize
10.67 KB
1.92 KB

Owww, nice! :)

sun’s picture

FileSize
1.93 KB

Of course, this should take $data->language into account.

Dries’s picture

Status: Needs review » Needs work

Please add a code comment explaining + 'class' => ($data->dst != drupal_get_path_alias($data->src, $data->language) ? 'warning' : NULL),.

sun’s picture

Status: Needs work » Needs review
FileSize
2.06 KB

Added inline comment.

alexanderpas’s picture

Status: Needs review » Reviewed & tested by the community

looks good! (btw: how about backporting this patch?)

swentel’s picture

Testbot happy and tests also pass on postgres too, so ready to go !

Dave Reid’s picture

Very nice addition.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Glad we were able to drive this one home. Committed to CVS HEAD. Thanks to all people involved.

swentel’s picture

sun’s picture

Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

alexanderpas’s picture

testbot race condition? lol!

Dave Reid’s picture

Status: Needs work » Reviewed & tested by the community

This continues to not be corrected in HEAD after a day. :/

Dries’s picture

It was a mistake to let that other patch slip in, but it is probably OK now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.