Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
First i discovered http://drupal.org/node/288946, and there is beeing said that that patch is in d7, i mean that the menu path could be 255 in length.
so i tested that and input:
http://www.ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd.de
which is 231 letters long, saved the menu and edited again and copied out the link:
http://www.ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd
that is 128 length, so the string is trimmed and the field is only 128 in length.
Comment | File | Size | Author |
---|---|---|---|
#9 | 1237290-test-only.patch | 1.15 KB | xjm |
#8 | 1237290-menu-link_path-maxsize.patch | 1.86 KB | dawehner |
#5 | 1237290-menu-link_path-maxsize.patch | 1.86 KB | dawehner |
#3 | menu-maxlength-1237290-4818612.patch | 722 bytes | marcoka |
#2 | menu.patch | 642 bytes | marcoka |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedI tested this and it works for me. I copied your original string of 'd's and pasted it into the URL path settings form. It is saved to the database—which does say that the value type is VARCHAR(255)—and the node is also is accessible from the path.
Please re-open if this continues to be an issue.
Comment #2
marcoka CreditAttribution: marcoka commentedi asked another person to test this in d 7.7
as a matter of fact the maxlength of the path field in the menu is 128, so the field itself will not accept more than 128 chars
Screenshot: http://screensnapr.com/v/PGGRUm.png
So i checked the menu.admin.inc form generation:
There is no maxlength defined, so somehow the std. of 128 is used. So if you add maxlength 255 it works
After applying patch: http://screensnapr.com/v/TGREeO.png
Comment #3
marcoka CreditAttribution: marcoka commentedsorry wrong patch format
Comment #4
marcoka CreditAttribution: marcoka commentedComment #5
dawehnerThis patch looks perfect!
Let it first be done in d8 and then backported.
@e-anima: Does this needs backport to d6 as well?
Here is a new patch which has a test for this, didn't runned the test yet, let's see.
Comment #7
xjmShould be: "Attempts to add a menu path with more than 255 characters (the maximum length)."
Typo in '%lenght' so I think the test will fail for the wrong reason. Also, I'd maybe change the assertion text to "A menu path over 255 characters is rejected." for clarity.
Comment #8
dawehnerNext time i will c&p again :)
Oh this so much easier, simpler and better to understand. thanks!
Let's see what the testbot says.
Comment #9
xjmLet's try dereine's test by itself to demonstrate it fails without the fix. (Fails correctly when I test locally.) If testbot comes back with a fail here then I think this is RTBC.
Comment #10
xjmComment #12
xjmFails as expected. Marking RTBC for #8.
Comment #13
Dries CreditAttribution: Dries commentedI don't think we need a test for this in the menu system. We're just leveraging the form API so this should be covered by form API tests.
Comment #14
Dries CreditAttribution: Dries commentedSo I committed the patch in #8 to 7.x and 8.x. I committed a modified patch without the test per my own suggestion in #13.
I'm going to mark this 'fixed' but if you think we actually need the test, I'd be happy to discuss that more. Just re-open the issue. I'm just trying to keep unnecessary tests out of core.
Thanks for the patches.
Comment #15
colanNeeds backport to D6 as per http://drupal.org/node/829190 and #288946: Increasing path length to 255 chars.
Comment #16
j0rd CreditAttribution: j0rd commentedJust 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.