Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When adding a shortcut to a shortcut set (or editing) via the add/edit form (as opposed to adding a shortcut by clicking on the handy-dandy link by the page title in Seven theme), you get an error saying "The link must correspond to a valid path on the site." if you enter a valid path alias.
That seems wrong to me. It should accept a path alias.
Comment | File | Size | Author |
---|---|---|---|
#16 | 722354fixnewline.patch | 3.92 KB | catch |
#10 | 722354fixnewline.patch | 3.9 KB | jhodgdon |
#9 | 722354.patch | 3.93 KB | jhodgdon |
Comments
Comment #1
jhodgdonActually, the problem I am having is that adding URL aliases is not working at all.
Reproduce:
a) Visit the URL alias page: admin/config/search/path
b) Click "Add alias". This takes you to: admin/config/search/path/add
c) Type in "node" for the system path, or "search" if you have the search module turned on, or "admin" or some other path. Type in "foo" for the alias. Click Submit, and see a nice alias added message, and the alias is in the alias list, although if you look at the URL shown for the alias in the table, you'll see it's the base system path, not the alias.
d) Attempt to visit page "foo". You will get a "Page not found error".
e) Clear all caches. Still (d) applies.
This is sounding like a critical error now...
Comment #2
jhodgdonForgot to change component.
Comment #3
jhodgdonOne more note. I just added a page to the site and gave it a URL alias, and that alias worked.
So I went back to the URL Aliases page to see what was different. It immediately jumped out: The working alias had a language defined of English, and the non-working had the default from the add alias page: "All languages".
When I edited my foo alias to say it was for only English, it worked fine.
So it appears this is only a problem for aliases marked "all languages".
Comment #4
jhodgdonI think the problem is that http://api.drupal.org/api/function/drupal_lookup_path/7 is assuming that the language field in table {url_alias} will have the value LANGUAGE_NONE (which is 'und'), but http://api.drupal.org/api/function/locale_form_path_admin_form_alter/7 puts in '' as the value for "All languages" when building the list of options for the URL alias form.
The fix would be to change locale_form_path_admin_form_alter():
to
Comment #5
jhodgdonAlso, as a note, when you are creating a path alias when editing a node, it does save LANGUAGE_NONE in the path alias table. It is only when you use the path.module UI to create a path alias that this is a problem.
And only when the locale module is enabled. Otherwise, the language list is not even in the form, and the default in path.module is to set the language to LANGUAGE_NONE.
Comment #6
jhodgdonThis also needs a test. Needs to enable locale and path modules, and test that if you use the UI to create a path alias, then visit the aliased path, it works.
Comment #7
jhodgdonDang, forgot the tag after all that. :)
Comment #8
jhodgdonI'll make a patch with a test. I'll make the test first, verify it fails, then try the fix in #4 and hopefully have a good fix.
Comment #9
jhodgdonOK, here it is.... With just the added test, I get this failure:
With the locale.module fix, the test passes.
I love that.
Comment #10
jhodgdonDang. Here's a patch with the newline fixed.
Comment #11
plach@jhodgdon: does not
PathLanguageUITestCase
overlaps withPathLanguageTestCase
? Shouldn't we fix the latter rather than introducing a new test case?Comment #12
jhodgdonI thought it was actually fairly different, because one is testing the lower-level functions and the other is testing the user interface.
They could both have issues that are semi-independent. In this case, the issue was specifically in the UI. I think having both tests in there is useful, since if a future change breaks just the UI, then we know we don't have to go back to the lower-level functions, just fix the UI.
Comment #13
plachI gave this a closer look and I agree that the two test cases don't overlap. The patch takes care of the issue and comes with additional test coverage. I'd suggest a couple of minor changes to make the two function names below more semantic.
Aside from those RTBC.
I'd suggest to rename this to
testDefaultLanguageURLs
and change the comment accordingly.I'd suggest to rename this to
testNonDefaultLanguageURLs
and change the comment accordingly.Powered by Dreditor.
Comment #14
jhodgdonReally? The test explicitly sets the language of the URL to English, and implicitly assumes the default site language is English. I think the headers are reflecting what the tests do.
Comment #15
plachI was just suggesting to make the default/non default language URL thing explicit, as it's the main difference in the two test functions, not certainly the fact that they use two different languages, as this would make no difference for what the URLs language logic is concerned.
Comment #16
catch#15 makes sense to me. Since it's just a string change, I'm marking this RTBC, the actual fix here is a one-liner.
Comment #17
aspilicious CreditAttribution: aspilicious commentedNice!
Comment #18
jhodgdonOK.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!