Truncate long paths and path aliases on admin/build/path

Coding style needs work. Not sure this is the proper solution.

features go into HEAD

Needs to be converted to use truncate_utf8().

Adding Novice tag

Being a novice, I figure I'll take a crack at this. It seems fairly straight-forward, at least as far as truncating to 50 characters, as the original patch did. I also took the liberty of adding a 'title' attribute to each link with the full original path.

Okay... How about I try to "Ensure the patch only contains unix-style line endings."

Being a nitpicker (almost by trade), I have to point out that the good coding standards say not to have lines longer than 80 characters, even if this is sometimes ignored as other concerns outweigh it. In this case, however, there is a perfectly good solution for making the lines <=80 characters long: breaking up the array()s! :D

Give http://drupal.org/coding-standards#array a look, and all will be good. It's good seeing you go at 'em. :)

Thanks for the encouragement! I was glad to discover the "Novice" tag, as the core issue queues can be intimidating.

Grrr! Again with unix-style line endings.

Should use http://api.drupal.org/api/function/drupal_substr/7 instead of truncate_utf8. truncate_utf8 does a byte count, not a character count.

Is there a reason to call drupal_substr() directly? All truncate_utf8() appears to do is provide optional parameters for making a sub-string respect word boundaries and add ellipses. It otherwise calls drupal_substr() to do the actual substring selection.

The Drupal 5 version appears to have performed a byte-count, if that helps...

Setting back to "needs review" as the argument for "needs work" in #14 was responded to in #15.

@jhodgdon Yep it's truncate_utf8 that should be used in this case, like all other cases in D7 with shortening & adding ellipses.

Dave Reid: I was going by #200185: truncate_utf8() is used as a substring function.

Also, are we really sure that adding "..." is appropriate in all languages, such as Chinese for instance? I don't know, I'm just asking.

@jhodgdon Oh I agree the truncate_utf8() function has been horribly misused for quite a while, and changing the function name is a big +1. We'd still be using that function here though, so it should stick with truncate_ut8() until it officially gets renamed right? The current patch is not using wordsafe.

Correct - at least the current patch isn't using the truncate-to-a-word functionality...

What about the question of whether adding "..." is appropriate for all languages? I still don't know the answer to that one.

According to the Wikipedia article, ellipses are used in many languages, including Chinese, though in that case, it is usually represented as 6 dots arranged vertically. It seems to me that the purpose of the $dots parameter is to "indicate an intentional omission," and that any logic about how (and whether) to represent it ought to be handled at the translation layer, since it's language specific.

While truncate_utf8() just adds ' ...', perhaps it should add t('…'), t('…'), or something similar instead. I don't know if passing HMTL entities or numeric character references is valid in t(), but you get the point. A translator with the appropriate knowledge could then provide the proper character(s) for his or her language.

Either way, I don't think it's something specific to truncating paths, and perhaps ought to be addressed in how truncate_utf8() handles it.

Adding t('...') sounds like a good solution. I don't think using an HTML entity is a good idea, as much of the time truncate_utf8 is used for plain text.

But yes, it should be addressed on truncate_utf8(). I've just filed an issue:
#768040: truncate_utf8() only works for latin languages (and drupal_substr has a bug)

#13: truncate_paths.patch queued for re-testing.

I believe the issues brought up about the patch in #13 have all been addressed, so I've submitted the patch for re-testing.

Looks good. Looking forward to the truncate_utf8 rename which shouldn't hold this up.

#13: truncate_paths.patch queued for re-testing.

Reroll and amends to patch so as the code reflects the new array structure used for path aliases.

I think you need to wrap the HTML title attribute for l() in check_plain(), correct?

There is no need to add check_plain around the HTML attribute as drupal_attributes() performs the check_plain().

Ah, you are correct. I think this patch is probably fine then, but I haven't personally tested it.

Hopefully the truncate_utf8() function issue will be fixed sometime, so that we can feel comfortable about using it for these purposes... right now it is waiting for a test debug, which I'll do when I have time, but if anyone else does:
#768040: truncate_utf8() only works for latin languages (and drupal_substr has a bug)

See comment http://drupal.org/node/86461#comment-2803440 with regards that if this path needs to be revisited lets do that after the utf path lands.

Right. I wouldn't delay this patch because truncate_utf8() is not working as it should be. This patch should be fine once truncate_utf8() is fixed.

Anyway, I think the above patch is fine.

#27: drupal.truncate.27.patch queued for re-testing.

change to task.

#27: drupal.truncate.27.patch queued for re-testing.

This patch somehow reminds me of #288039: URL alias admin page should include a link to the aliased URL

Please make sure that all discussed details from over there are taken into account in this patch.

#27: drupal.truncate.27.patch queued for re-testing.

#27: drupal.truncate.27.patch queued for re-testing.

That failure is bogus:
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.

#27: drupal.truncate.27.patch queued for re-testing.

Needs to be rerolled for 8.x.

Reroll

It works as expected.

Let's add an automated test for this. Thanks!

Updated issue summary.

Adding automated test.

+++ b/core/modules/path/path.test
@@ -128,6 +128,18 @@ class PathAliasTestCase extends PathTestCase {
+    $this->assertNoText($alias, 'Th untruncated alias was not found.');

This looks RTBC, but there is a minor typo here.

Oops. Fixing typo.

Thanks!

Reviewed patch and test. Looks good!

Makes sense. Committed/pushed to 8.x, moving to 7.x for backport.

Backport.

Diffstats are the same, test fails as expected. Looks good to me!

I reviewed this and it looks good to me, but can someone clarify why we are choosing 50 characters as the number to truncate at? That seems like an awfully small number. My laptop screen isn't that big, but there is still way way more room than 50 characters... making it look awfully funny to truncate that small (see attached screenshot).

Or maybe this issue is part of the mobile initiative? :)

If we're going to change this in D7, I'm thinking we should do it only once... so I'm bumping this issue back to D8 for the moment (for my question above about whether 50 characters is too short for the truncation).

If there needs to be a high-level discussion about this, I will remove the "Novice" tag, because such a discussion would need input from the mobile initiative team rather than Drupal Ladderers like me...!

Updated issue summary.

Added Issue summary template.

Isn't there a CSS rule to handle single-line-truncation-of-text-with-added-ellipsis for us client-side rather than trying to guess some perfect number of characters? http://css-tricks.com/snippets/css/truncate-string-with-ellipsis/

If we're not worried about IE10- support for this particular ellipsis, then we should totally use the CSS solution.

Why is this as needs work?

This issue was committed in May 2012 and set for backport to Drupal 7. It was reopened to discuss why '50' was used as the length of the alias. There's been one comment about that in the intervening 11 years. In the meantime paths and path alias have been converted to entities with a length of 255.

Looking at Drupal 7 path.admin.inc I don't see that this change has been applied to Drupal 7, there is a patch in #53 for Drupal 7.

Therefore moving to the Drupal 7 issue queue.

The patch applies to D7, restoring the RTBC for the patch from #55.

Patch #53 does not apply anymore, it does needs a reroll (but read other thoughts below).

After reading the whole discussion and @David_Rothstein comments, I think that for D7 we have three options now:

1. Accept the rerolled #53 to the D7 - it is the same code as the current D9 code (checked it and aliases in D9 are still truncated to the 50 characters on the admin/config/search/path page , see the attached image).

2. Do not accept this and close this without commiting to the D7.

3. Reopen this for D9 to change the truncated length from 50 characters to another number and then update the D7 patch to reflect this (this was what @David_Rothstein proposed originally).

I understand that for D8/9 this was probably important from the UI/UX and mobile friendly point of view, but D7 is in a bit different state now and I think we should decide between 2. or 3. Just for the illustration, if you create a long alias in D7, it is wrapped correctly (if it doesn't contain only characters without delimiters, which should be uncommon scenario), so nothing too disturbing for me there (see the attached image).

Switching to Needs review as this needs to be decided by @mcdruid / @Fabianx.

If #3351638: Remove truncation of path alias lands, this issue can likely be closed.

Thanks! Let's postpone this until that one is resolved (either committed or not).

This was committed to D8 on 8.5.2012. And the behavior was "reverted" in D10 today, see: #3351638: Remove truncation of path alias

This means that the current status in D7 (without the patch committed) is the same as in the D10 - aliases are not truncated.

I am going to close this issue as Fixed for D8, based on #52, so that credits are assigned properly. For D7 backport it is a Won't fix (we will not make any change).

Thanks all.