Add path alias to taxonomy term edit

I think this is a cool idea and I'd like to help out on it.

I'd also like to link to #475968: Path helper interface for menu module and quote from merlinofchaos:

[there should be a] toolbox that lets you arbitrarily create a menu item [or path alias?] to the path you happen to be at. This is why the auto menu item for a node works well. But that's too weak. It should be universal. Just click in a toolbox, select 'edit menu entry for this page' or something and presto chango.

@catch, a few questions...

1) should this feature be available on both when adding/editing a node and term (node/1/edit and taxonomy/term/1/edit)?

2) Regarding UI when on the node form, where should the term path option show up? Under the "tag" field, or near the bottom, below the "URL path settings" field?

Here's a patch that should provide a good start to getting this in. Basically I added the URL Path Settings fieldset in the same way that it appears on the node edit form but for the term edit form.

Things I'm unhappy about in this patch:
- please test language support - this is untested and I don't know if $term->language is ever set.
- do we really want $term->path loaded with every taxonomy term (performance hit?). Could maybe handle this just on the term validation and submission instead. From API page for hook_taxonomy_term_load():

For performance reasons, information to be added to term objects should be loaded in a single query for all terms where possible.

- the hook_form_alter() check for the taxonomy form could be better. I'm not happy with looking at empty($form['confirm']) to see if we are on the delete confirmation page.

Also found that the hook triggers for _update and _insert in taxonomy.module were backwards. Should this be broken into its own issue? To work correctly, this needs to be fixed.

This is my first biggish patch for core! :)

Sorry, I didn't know someone else was working on this... Here is my patch... I used as much path.module functions as possible...

resubmitting

Trying to keep the momentum up for this issue. Here are screenshots from the term edit form for the patches in #3 and #7.

We probally want it expanded by default? Or atleast don't do a summary in the fieldset, its not something we want to pursue in drupal.

I'm a big fan of this patch.

For me, it doesn't have to be in a fieldset so I'd be OK with getting rid of the fieldset and always showing the option (when path module is enabled).

The patch has various coding style issues though. It needs more work.

Wrong image.

trying to fix coding style issues

fixed another indentation issue.

Quick 2 minute review:

- There are two spaces between: "term url".

- In user output, we need to write URL instead of url.

- Code comments need to start with a capital letter and end in a point.

- ...

Fixed
- Term url
- User output
- Some of the code comments

Looking at the comments it seems this patch needs finishing off.

This is a reroll of my patch from #3 with the fieldset removed. This patch moves all of the code into path.module where it should be easier to maintain. Reposting in reply to #16 by catch.

Additionally it also fixes the hooks for hook_taxonomy_term_update() and hook_taxonomy_term_insert() in taxonomy.module which seem to be backwards.

Small code style fixes from #17. Couple places where spaces were used instead of tabs.

Here's an update to address those issues. The ternary wasn't nested but for clarity I've broken those out into variables in both path_node_update() and path_taxonomy_term_update().

I've switched to path_form_taxonomy_form_term_alter() for the form alter.

Once I wanted to have a tab on every page to do path aliasing.

Last patch failed because of test bot. Checked out a fresh copy of head and got the same errors, seems something weird was committed.

We have to decide which direction we need to go. Patch #20 code is mostly in path.module. Patch #7 is all done in taxonomy module files. Both patches attempt the same thing. Core devs, do you have a preference? Give us direction please. This thread has some rerolls for both patches...

Adding the code to path module (per patch #20) is the proper direction.

@Dries, thanks

Nice little patch.

@@ -199,8 +199,11 @@ function path_node_insert($node) {
  */
 function path_node_update($node) {
   if (user_access('create url aliases') || user_access('administer url aliases')) {
+    $path = 'node/' . $node->nid;
+    $alias = isset($node->path) ? $node->path : NULL;
+    $pid = isset($node->pid) ? $node->pid : NULL;
     $language = isset($node->language) ? $node->language : '';
-    path_set_alias('node/' . $node->nid, isset($node->path) ? $node->path : NULL, isset($node->pid) ? $node->pid : NULL, $language);
+    path_set_alias($path, $alias, $pid, $language);
   }
 }

^ is this code clean up really necessary here? Please avoid scope creep as much as possible, it makes patches more difficult to review.

 /**
+ * Implement hook_taxonomy_term_load().
+ */
+function path_taxonomy_term_load($terms) {
+  foreach ($terms as $term) {
+    $language = isset($term->language) ? $term->language : '';
+    $path = 'taxonomy/term/' . $term->tid;
+    $alias = drupal_get_path_alias($path, $language);
+    if ($path != $alias) {
+      $term->path = $alias;
+    }
+  }
+}

^ This will be loaded each time a term is required. I don't believe it's necessary to load the alias each time.

 /**
+ * Implement hook_taxonomy_term_load().
+ */
+function path_taxonomy_term_load($terms) {
+  foreach ($terms as $term) {
+    $language = isset($term->language) ? $term->language : '';
+    $path = 'taxonomy/term/' . $term->tid;
+    $alias = drupal_get_path_alias($path, $language);
+    if ($path != $alias) {
+      $term->path = $alias;
+    }
+  }
+}

^ As for as I know, Drupal Core doesn't support multilingual terms. You cannot do that ;)

+function path_taxonomy_term_insert($term) {
+  if (user_access('create url aliases') || user_access('administer url aliases')) {

+function path_taxonomy_term_update($term) {
+  if (user_access('create url aliases') || user_access('administer url aliases')) {

^ You shouldn't call user_access() in low-level API functions. Access control needs to be done above (in the form itself).

+ /**
+ * Implement hook_taxonomy_term_delete().
+ */

^ There is a small code style error in the first line of the docblock.

+    $form['identification']['path'] = array(
+      '#type' => 'textfield',
+      '#title' => t('URL alias'),
+      '#default_value' => $path,
+      '#maxlength' => 255,
+      '#collapsible' => TRUE,
+      '#collapsed' => TRUE,
+      '#weight' => 0,
+      '#description' => t('Optionally specify an alternative URL by which this term can be accessed. Use a relative path and don\'t add a trailing slash or the URL alias won\'t work.'),
+    );

^ Text fields cannot get collapsed ;)

   if (!empty($term->tid) && $term->name) {
     $status = drupal_write_record('taxonomy_term_data', $term, 'tid');
     field_attach_update('taxonomy_term', $term);
-    module_invoke_all('taxonomy_term_insert', $term);
+    module_invoke_all('taxonomy_term_update', $term);
   }
   else {
     $status = drupal_write_record('taxonomy_term_data', $term);
     field_attach_insert('taxonomy_term', $term);
-    module_invoke_all('taxonomy_term_update', $term);
+    module_invoke_all('taxonomy_term_insert', $term);
   }
 

^ This literally screams: "We need more tests". Of course, that's outside of the scope of this patch.

Thanks for the review Damien, I think I've addressed all of the issues you've raised in this patch. For it to work correctly you will need to switch the taxonomy hooks as I've now left them out.

Opened a follow up issue to fix the taxonomy hooks: #537044: Taxonomy hooks backwards

- I committed #537044: Taxonomy hooks backwards.

- Missing space in + if(!empty($form['#term']['path'])) {.

- + $path = $alias != 'taxonomy/term/' . $form['#term']['tid'] ? $alias : NULL; is weird and probably best written in multiple lines. The code is correct, it's just that I expected drupal_get_path_alias() to return NULL if no path alias was found. It would make this code cleaner. Best left for another issue.

- Please add a code comment explaining why you populate $form['identification']['path']['pid'].

- It is not entirely clear why you implement hook_taxonomy_term_update() and hook_taxonomy_term_insert() instead of implementing a custom #submit callback. Any particular reason? I think you'd also want a #validate callback to see if the specified path already exists. The form has no error handling right now.

This is starting to get into shape!

I do agree with Dries that path_taxonomy_term_insert() and path_taxonomy_term_update() should be replaced by a new #submit handler. That's because if a module updates a term directly (by calling taxonomy_term_save), the path alias risk to be removed.

Followed up on feedback from #30, fixed code style issues and moved to a submit handler with validation.

_{This review is powered by Dreditor.}

+++ modules/path/path.module	4 Aug 2009 09:52:24 -0000
@@ -250,6 +257,79 @@ function path_form_alter(&$form, $form_s
+          ':dst' => $form_state['values']['path'],
+        ))
+        ->fetchField();

Very (very) minor bagatelle: one space missing in each of these lines.
Apart from that the code looks good to me.

Another question: now that we have some code to inject the path alias field into taxonomy forms and into user forms (#476294: Add path alias to user edit) do we want to "outsource" these functions to files like path.taxonomy.inc and path.user.inc?

Wasn't too clear but I believe I have fixed the spacing issue, let me know if it needs any other changes.

This looks good to me. Marking RTBC hoping to get a final review from someone else.

No objections anymore and a big fat RTBC from me.

I'm very sorry, but I don't get this chunk (but I am *very* tired):

+    // When a new term is added, the path is put back in but the tid is missing
+    if (!empty($form['#term']['path'])) {
+      $path = $form['#term']['path'];
+    }
+    else {
+      $url = 'taxonomy/term/' . $form['#term']['tid'];
+      $alias = drupal_get_path_alias($url);
+      if ($alias != $url) {
+        $path = $alias;
+      }
+      else {
+        $path = NULL;
+      }
+    }

Here $path is designed to be a default value. So this chunk should probably be (pseudo code):

  if (!empty($form['#term']['tid'])) {
    // Fetch an existing alias if it exists (ie. the code above)
  }
  else {
    // Default to no alias when creating a term.
    $path = '';
  }

Hi Damien, when the taxonomy form is saved and a new term is added, the form is repopulated with the values from the last submission -- I also have found this confusing and maybe it should be opened as a separate issue? This snippet is to repopulate the path value in the form after the submission just like the rest of the values.

Wow! I like this patch very much!

Some minorish nit-picking and then this is good to go.

+++ modules/path/path.module	11 Aug 2009 17:45:48 -0000
@@ -250,6 +257,79 @@ function path_form_alter(&$form, $form_s
+    // When a new term is added, the path is put back in but the tid is missing
+    if (!empty($form['#term']['path'])) {
+      $path = $form['#term']['path'];
+    }
+    else {
+      $url = 'taxonomy/term/' . $form['#term']['tid'];
+      $alias = drupal_get_path_alias($url);
+      if ($alias != $url) {
+        $path = $alias;
+      }
+      else {
+        $path = NULL;
+      }
+    }

I agree with Damien that this could use some more/better comments. While what's there might be technically correct, it doesn't give me the broad-strokes overview of why it takes 10-12 lines of code to figure out the existing path alias.

Also, all comments should "Start with a capital and end with a period."

+++ modules/path/path.module	11 Aug 2009 17:45:48 -0000
@@ -250,6 +257,79 @@ function path_form_alter(&$form, $form_s
+      '#description' => t('Optionally specify an alternative URL by which this term can be accessed. Use a relative path and don\'t add a trailing slash or the URL alias won\'t work.'),

Where possible (like here) use double quotes around the string so you don't have to escape the quotes.

+++ modules/path/path.module	11 Aug 2009 17:45:48 -0000
@@ -250,6 +257,79 @@ function path_form_alter(&$form, $form_s
+        '#value' => db_query("SELECT pid FROM {url_alias} WHERE dst = :dst", array(
+            ':dst' => $path,
+          ))

Double-check this, but I think in other simple queries in core, we don't break the query parameter to its own line.

This review is powered by Dreditor.

Nit-pickiness is good! I'll see about opening a follow up issue to correct some of the mistakes that were inherited from the node path handling...

I've tried to clarify some of the comments, let me know if they still need some refinement or are still unclear. I couldn't find any docs on db_query() style for D7 yet but did see (and much prefer) the short version elsewhere.

Working on the last comment.

there we go

Back to needs review so bot can take another whack at it.

Committed to CVS HEAD. Thanks!

This needs further work.

The path field comes after the buttons.

I'm guessing, though I'm not sure, that the original form has changed and this alteration of it has not followed pace.

We already have this RTBC'd in #678590: Path alias link is below submit button on taxonomy term add.

This is exactly what I need...but for 6.x. Is there a patch for 6.x already? Thanks!