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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Adding shortcut - aliases don't work » URL alias "add alias" not working?
Priority: Normal » Critical

Actually, 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...

jhodgdon’s picture

Component: shortcut.module » path.module

Forgot to change component.

jhodgdon’s picture

Title: URL alias "add alias" not working? » URL aliases marked "all languages" do not work.

One 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".

jhodgdon’s picture

I 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():

   '#options' => array('' => t('All languages')) + locale_language_list('name'),
 

to

   '#options' => array(LANGUAGE_NONE => t('All languages')) + locale_language_list('name'),
 
jhodgdon’s picture

Also, 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.

jhodgdon’s picture

This 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.

jhodgdon’s picture

Issue tags: +Needs tests

Dang, forgot the tag after all that. :)

jhodgdon’s picture

Title: URL aliases marked "all languages" do not work. » URL aliases marked "all languages" from UI do not work.
Assigned: Unassigned » jhodgdon

I'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.

jhodgdon’s picture

Status: Active » Needs review
FileSize
3.93 KB

OK, here it is.... With just the added test, I get this failure:

Language-neutral URL alias works	Other	path.test	355	PathLanguageUITestCase->testLanguageNeutralURLs()

With the locale.module fix, the test passes.

I love that.

jhodgdon’s picture

FileSize
3.9 KB

Dang. Here's a patch with the newline fixed.

plach’s picture

@jhodgdon: does not PathLanguageUITestCase overlaps with PathLanguageTestCase? Shouldn't we fix the latter rather than introducing a new test case?

jhodgdon’s picture

I 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.

plach’s picture

I 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.

+++ modules/path/path.test	24 Feb 2010 19:47:04 -0000
@@ -306,3 +306,83 @@
+  /**
+   * Tests that an English URL alias works.
+   */
+  function testEnglishURLs() {

I'd suggest to rename this to testDefaultLanguageURLs and change the comment accordingly.

+++ modules/path/path.test	24 Feb 2010 19:47:04 -0000
@@ -306,3 +306,83 @@
+  /**
+   * Tests that a foreign-language URL alias works.
+   */
+  function testForeignURLs() {

I'd suggest to rename this to testNonDefaultLanguageURLs and change the comment accordingly.

Powered by Dreditor.

jhodgdon’s picture

Really? 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.

plach’s picture

I 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Quick fix
FileSize
3.92 KB

#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.

aspilicious’s picture

Nice!

jhodgdon’s picture

OK.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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