EOL Taxonomy Sprint: Add an Edit tab to term view pages (+ menu cleanup)

Here's a patch, which also fixes a bug in the content/taxonomy/edit/term/ (try it without arguments).
Simpletests tomorrow or maybe later tonight.

So at the moment, the only way to edit a term, is to find it in the admin list (where it could be hundreds of pages in). If you're using pathauto, then it's almost impossible to find out the tid of a term when viewing the taxonomy/term/tid page too. Here's some screenshots.

I just opened #306068: Rename taxonomy_get_term to taxonomy_term_load but the issues are so interdependent that I'm going to merge it into this (at least for now).

After feedback from Bojhan, the tab has also been changed from "Edit" to "Edit term", to make it clear that it's the term, rather than the page itself, which will be edited. Note that the standard taxonomy/term/% pages do funky stuff with 1+2+3,4 URLs, so they can't use the standard _load function. However it works for the two admin pages. There's probably some additional cleanup could be done here, but the general idea definitely needs review.

The edit tab was appearing on taxonomy/term/1+2 etc. pages due to the database layer casting to int (thanks pwolanin for the pointers). So taxonomy_term_load now does a sanity check, so we only get the tab on pages for single terms and nowhere else.

I am Bojhan Somers and I approve this patch. (Subscribing)

Looks good here, I wont add it to the list of things to be tested as I doubt it requires it. If we see this pattern emerge at other places, we should think more carefully about it to avoid adding clutter to the interface, but in this context to me it looks clear.

Thanks for supplying images to this patch.

@Benjamin: Sure! I'm very much for example.com/term/4 instead of example.com/taxonomy/term/4! If no serious objections are filed, I think we should make that move. I believe, this is a separate issue, though.

@Benjamin/Pancho : http://drupal.org/node/307222 .

+1, but I don't agree with Bojhan on the "Edit term" label of the local task. Yes, he's the usability expert here ( ;-) ), but it's not consistent with the rest of Drupal and there's hardly anything else to edit at term pages except for the term itself.

+1, and I agree with Xano. "Edit" should be just enough.

What I like even less is the completely inconsistent behaviour for single and multiple term listing pages. Usually, not so many terms are displayed together - so we might want to add an edit tab for every term, in that case. On taxonomy/term/1+2+5, we would have the tabs "View", "Edit 'one'", "Edit 'two'" and "Edit 'five'". How about that?

We don't display descriptions on those pages, I don't think we should display edit tabs either.

I personally think edit term is fine. In answer to 'nothing else to edit on those pages - views overrides, panels overrides, not to mention all the listed nodes.

Bojhan just convinced me. I'd go for 'Edit term' now too :P

I'm biased (as I wrote the original local menu task patch) but as I thought then: This should easily have been in Drupal long ago.
+1
... I'd +2 it if I had also set up and tested it on D7-dev so I could say RTBC :-B. I'll try that when I get a break.

Not fussed about the label, but the arguments for more clarity are good.
.dan.

This looks like a really nice cleanup. Couple of things I noticed:

Would be nice to use positive weights instead of negative ones (looks like this might have been in the original code).
I can't see a need for 'parent' in hook_menu() (ditto above).
Couple of functions are missing doxygen.
forum module needs updating for the function name change.

Would be good to get this in before some of the other taxonomy patches since the menu cleanup is likely to be a dependency.

:(

Why do we have all of this stuff in one monstrous patch? I was so looking forward to committing something that did #2, and now I need to find a spare hour to review the whole thing. :(

Is there any way we could make a "Make Taxonomy module suck less" patch separate from this nice usability improvement, or are they really that tied closely together?

The content/taxonomy/edit/term/ page generates a warning if you view it without the tid argument, this is due to not using the menu system properly, so it's a little hard to separate the two. We could do the cleanup first with no tab, or add the tab with the current broken menu implementation I guess. Adding the tab without the menu cleanup means copying around more duplicate code and adding more drupal_set_titles() where they don't belong though :(.

Note that this doesn't introduce the hooks from #306224: EOL Taxonomy sprint: add proper taxonomy term hooks just renames taxonomy_get_term (that patch will need re-rolling once this one gets in).

A quick summary of what the patch actually does:

1. changes taxonomy_menu() to use the named autoloaders instead of %
2. Juggles the organisation of the various vocabulary admin screens so they're actually tabs instead of seperate menu callbacks inaccessible from each other.
3. Add the taxonomy/term/1/edit

90% of the patch is find and replace for taxonomy_get_term() to taxonomy_term_load().

If you don't mind a slightly ugly patch for the edit tab, we could roll the cleanup elsewhere.

Ok, that's fair enough. I'll try and look later today.

Is it still code needs work?

forum.test needed updating for the new URLs. There's some casts to array etc. in this patch which aren't pretty, but I'm marking this as needs review in the hope we can get it in and do more cleanup in followup patches. Ran all tests and manually checked the term and vocabulary edit pages.

Here's a screenshot for the new vocabulary edit tab organisation.

Hint taken ;)

Here it is with no associated menu fixes, just the tab.

After discussion with Damien in irc, here's a few fixes to the 20k patch to clean it up that little bit more.

And those fixes broke the BlogAPI test, so here it is with that hunk too.

concat operator.

Ok. To summarize, this patch:

- cleans-up taxonomy_menu to implement proper loaders for %taxonomy_term (a single term) and %taxonomy_terms (multiple terms)
- moves the tag editing form from 'admin/content/taxonomy/edit/term/%term' to 'taxonomy/term/%term/edit' (new tab!!)
- moves the vocabulary edition from 'admin/content/taxonomy/edit/vocabulary/%vocabulary' to 'admin/content/taxonomy/%vocabulary/edit'
- removes taxonomy_get_term for the new loader taxonomy_term_load

It does all that without killing too many kittens. Moreover, the order of changes in the patch was carefully thinked to allow easy review.

Careful code inspection reveled several misuse of the string concatenation operator that were promptly fixed by catch. More over all tests pass, so I'm green-lighting this.

Excellent job, catch and Ben.

Damien noticed in irc that we were linking to vid/edit instead of vid - the /edit is redundant because that's the default local task. This patch removes it.

I've spent 30 minutes with this patch, and I think it looks good. I like being able to edit my tags faster.

* The only little bit of ugliness is:

+  if (!is_array($edit)) {
+    $edit = (array)$edit;
+  }

* When I'm changing a term, I nearly always want to update its URL alias too. This would be a great extra, and could be really trivial to add. It would save me a ton of time, and would be a awesome usability improvement. :)

* I have to do a slightly deeper review to check the check_plain()'s and friend.

One more thing: add an entry to CHANGELOG.txt :)

With the casts to array, that's somewhat unavoidable without changing API a bit, taxonomy.module is littered with them... However, we've got a fair few patches planned to clean up the API, like #306224: EOL Taxonomy sprint: add proper taxonomy term hooks - so if possible I'd like to go back in and remove all those later on in one sweep (or a couple of sweeps).

For URL alias editing, I've posted a follow-up issue here: #310613: Add path alias editing to taxonomy term forms- it's already in the edit_term module which inspired this, but there's some other issues which can't progress without this cleanup. so would be nice to do separately. Will be a tiny patch in itself.

I've added the CHANGELOG.txt entry.

As catch says, url alias editing on the term form was part of the edit_term enhancement module, as was a menu item editor, making term edits very similar to node edits. Both were HUGE usability improvements, and trivial to impliment.

These two extras were left out of this patch, mostly to just keep feature adds modular and managable.
We'll be following these up as new issues, I think.

I did a deep dive on this patch and it looks good. Committed to CVS. I'm hopeful that we tackle the follow-up tasks shortly. Thanks for opening issues for those. Rock on!