Path table has src and dst limited to 128 chars, while HTTP specification has no limit over the length of a URL.
This patch does not make column size to be infinite, but increases it to 256, to accommodate longer URLs, while keeping the DB sane.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

z.stolar’s picture

FileSize
862 bytes

I increased the column size to 256 instead of 255...
Rerolled the patch.

Anonymous’s picture

Status: Needs review » Needs work

I agree that an arbitrary limit is wrong but 256 is still an arbitrary limit. Is there a reason to not make these a text column? Also, you need to supply an update for the existing tables. And what about the forms?

z.stolar’s picture

I agree about the other fixes - I'll add them.

regarding the length - a question to those who know MySQL better: how to allow long input (>256chars) and still avoid new lines etc.? Should the validation be on Drupal side?

And what about HTML - can we have a text field longer than 256 chars?

Damien Tournoud’s picture

@z.stolar: you can have several lines in a VARCHAR, too, there is no technical limitation.

About the limit, I would say that any website having paths longer than 128 characters would be seriously broken, and even more with 256 characters! I'm not sure this is even remotely needed.

z.stolar’s picture

So regarding new lines, and other characters which are not useful in a URL, we'll do validation during form validation.

@Damien: Why do you think the website may be broken? Even Explorer is capable of very long urls...

Damien Tournoud’s picture

@z.stolar: of course, browsers are capable of handling long URLs, but that's really bad design.

z.stolar’s picture

I think Drupal should assist people design their websites properly, but not take decisions for them.
I suggest we put some kind of recommendation to use not-so-long url aliases.
Can you please supply a short explanation of why it is considered bad design, so I'll add it to the path form?

Anonymous’s picture

With pathauto, the URL's can be longer than the title of the node plus the length of the content type plus any length of text the user gives before that. I think 128 is just a shy short of good. I think any limit is bad. But we have to compromise in the middle I suppose; however, I don't think 256 is enough compromise and the columns should be set to text.

Damien Tournoud’s picture

@earnie: well, there is no such thing as "no limit". Resources are *always* scarce. Pathauto should learn (if it doesn't already, I didn't verified) to guarantee that paths do not go over the limit.

I don't really care which limit it is (128 vs. 255), even if 128 looks *really* long already.

For reference, here is how such URL would look like:

This looks like a won't fix to me.

z.stolar’s picture

I agree it *looks* bad.
But apart from that - why is it wrong?

Aesthetics is a matter of taste.

I think a good advice in the field's description is better than limiting the user. If someone decides to go with too long URLs, he should be noticed about it, not restricted from doing it.

z.stolar’s picture

Title: Increasing path length to 256 chars » Increasing path length to 255 chars
Status: Needs work » Needs review
FileSize
3.57 KB

Attached patch modifies the forms (admin and node), and adds an update in system.install.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

tootsietorres’s picture

My 2 cents:

It is bad to limit the path if you are trying to send variables to a search. I have been building websites for Realtors and most use IDX to access the MLS on their websites. If they want a custom search link, 128 chars is really limiting.

jeffschuler’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

Rerolled to apply to current D7 HEAD.

smk-ka’s picture

Status: Needs review » Needs work

I was also running into this issue in the past when using pathauto, +1 for increasing the length to 255 characters.

@jeffschuler
ALTER TABLE is not the correct way to alter tables, use db_change_field() instead.

jeffschuler’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

Thanks smk-ka.

Re-rolled using db_change_field() in the db update function.

deekayen’s picture

Status: Needs review » Reviewed & tested by the community

FWIW, I don't see any more issues. Upgrade worked for me. Don't know if there should be a test for checking adding of 254, 255, and 256 char strings to make sure 256 fails, etc.

Anyone else in favor of splitting off to a separate issue to discuss why url_alias is managed in system instead of path?

Dries’s picture

Waw, do we really need that long URLs?

deekayen’s picture

I think the answer to that question is another question. Why 128 then? Why not 100 or 200? Those are at least round numbers.

255 at least maxes out a field limit at the db level and gives users more flexibility to use the site as they wish.

sun’s picture

Path table has src and dst limited to 128 chars, while HTTP specification has no limit over the length of a URL.

We are limiting something that should not have a limit.

Just because Drupal core does not exceed the limit does not mean that there is not a use-case that may be limited by the fake limit we apply.

sun’s picture

Status: Reviewed & tested by the community » Needs work

That said, please remove that second, blank PHPDoc line:

 /**
+ * Increase permitted length of url aliases to 255 characters.
+ *
+ */
Dries’s picture

Status: Needs work » Fixed

Fair enough, even though it feels a bit silly. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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

will_in_wi’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

Can this be backported to 6.x? I have a use case where I have an external bound link which is 226 chars long. I have a hack in place at the moment which makes it work, but it would be nice to fix this for the current version.

Dave Reid’s picture

Component: system.module » path.module
febbraro’s picture

Here is a patch for D6 that we have in place and working. Thanks to @emackn for making it a while ago.

jeffschuler’s picture

Status: Patch (to be ported) » Needs review

Awesome, thanks...
Setting to needs review.

HnLn’s picture

subscribe and I don't think it's silly :-)

use case: pathauto aliasses based on menu-path (e.g. a product catalogue, I want the path to reflect the category tree the product belongs to)

Status: Needs review » Needs work

The last submitted patch, drupal-6-url-alias-to-255.patch, failed testing.

jeffschuler’s picture

Status: Needs work » Needs review
FileSize
3.66 KB

re-rolled for current Drupal 6.x-dev

Status: Needs review » Needs work

The last submitted patch, 288946-d6_longer_paths_2.diff, failed testing.

spamwelle’s picture

how could i change it to unlimited length and is there a fix for drupal 6.19?

Eric_A’s picture

This D6 patch will go from "needs review" to "needs work" automatically if the patch file is not properly named. That is because all test cases run by the bot are for Drupal 7.

From the File attachments fieldset:
For patches that apply specifically to Drupal 5 or Drupal 6, you can prefix the extension with -D5 or -D6 to prevent them from being queued for testing.

jeffschuler’s picture

Status: Needs work » Needs review
FileSize
3.66 KB

Thanks @Eric_A.
Patch from #30 still applies. I've renamed it.

amfis’s picture

Why this is not in the Drupal core yet?

Eric_A’s picture

It is in core, just not in D6. It will only be considered after the community has reviewed the code, tested the patch and changed the status accordingly.

Personally at this stage of the D6 life cycle I feel uncomfortable with code changes that require some module maintainers to think and think again when writing new code because they may need to implement hook_requirements() to enforce Drupal 6.20 minimum or provide code for both scenario's. The fact of the matter is that new code just cannot rely on the form field and db field to be 255 chars long.

rsanuy’s picture

#14: longer_paths_2.patch queued for re-testing.

jfarry’s picture

+1 subscribed.

This becomes a huge issue when linking to files from the menu system, on multisite installs.

I don't know if there is a way around this, but this is the problem that I have:
http://multisitedubdomain.intranet.multisiteurl.com/sites/multisitesubdo...

I can't add this in as a relative path either :( If anyone has another solution, I'm all ears :)

Anonymous’s picture

Version: 6.x-dev » 8.x-dev

Move to D8 since this is a feature request we need to apply there first.

jfarry’s picture

I understand your reasoning and i mean no disrespect, but this has been effecting d6 users for the last three years, whereas nobody is using drupal 8 yet and a patch has already been implemented in D7 core.

I am still learning about the patching/git/how the setup works and I'll have a go at it when i understand all this information, but in the interim, I (and probably about 15 other users from this thread) would not complain at all if this was fixed before I can get to it.

Cheers :)

Eric_A’s picture

Version: 8.x-dev » 6.x-dev
Status: Needs review » Needs work

Yes, this is in D8 and D7, so back to D6. The bot needs a -p1 patch. The -p0 format has been phased out.

jeffschuler’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

Re-rolled against drupal-6.x-dev (with git diff).

jfarry’s picture

I've manually applied this to my D6 6.22 install, then run update.php, run cron, done a full cache flush and when creating a new menu item or editing an old one, the max field size (for the edit-menu-link-path field) is still 128.

I've triple checked my implementation, but it's exactly as described in the diff. Is there something else that I could be doing wrong?

colan’s picture

pyrhoe: There's a separate issue for menu items. Take a look at #1237290: Menu path length is limited by the UI to 128 characters.

dawehner’s picture

I personally think this two patches should be merged as it's a small change and saves some time of the process (review testing etc.)

hillaryneaf’s picture

Tested D6 patch, works for me. Thanks!

andrewsuth’s picture

This has not yet been pushed into Drupal 6 core - any idea of when it will be done?

I feel this is really important as it is causing duplicate alias' to be created.
In path.module, this logic is performed:
$existing = db_fetch_array(db_query("SELECT pid, src FROM {url_alias} WHERE dst = '%s' AND language = '%s' ORDER BY pid DESC", $alias, $language));

So when the alias is > 128 characters, it does not find the entry in the database (as they are truncated to 128 characters at the DB field level) so it continues to reinserts the alias entry, causing duplicates.

colan’s picture

This has not yet been pushed into Drupal 6 core - any idea of when it will be done?

This will not be committed until the status is RTBC. You can set that yourself if you've tested the latest patch and it looks good + works.

j0rd’s picture

Status: Needs review » Reviewed & tested by the community

Just ran into this problem on a D6 site. I simply added #maxlength to link_path to get me passed the UI defaulting to 128 maxlength.

I've applied patch #45 and it resolved my problem.

From a review of the code it seems like sane changes (looks like he got most of the places to change), and the database update worked as expected.

Would like to see this committed.

andrewsuth’s picture

FileSize
3.94 KB

Patch #45 is not OK as it sets the default column value to NULL rather than empty string.

In the system_scheme we see that the default is set to empty string:

@@ -1127,13 +1127,13 @@ function system_schema() {
       'src' => array(
         'description' => 'The Drupal path this alias is for; e.g. node/12.',
         'type' => 'varchar',
-        'length' => 128,
+        'length' => 255,
         'not null' => TRUE,
         'default' => ''),

But then in the database update, the default value is not set, which causes it to reset to NULL:

+  db_change_field($ret, 'url_alias', 'src', 'src', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE));

The correct db_change_field() to execute should be:

db_change_field($ret, 'url_alias', 'src', 'src', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => ''));
db_change_field($ret, 'url_alias', 'dst', 'dst', array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => ''));
colan’s picture

Status: Reviewed & tested by the community » Needs review
j0rd’s picture

Good catch @andrewsuth.

Viliasas’s picture

Are there any news about this change? Will it be pushed to Drupal core at all?

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.