Admin theme is used is for adding taxonomy term, but not for editing taxonomy term

@jhodgdon - apparently selecting or unselecting that box has no effect on the outcome. I get the same results as you.

@JoshuaRogers - followed these steps to reproduce the above error

Installed d7

Either it is already enabled or enable Seven via http://localhost/d7/#overlay=admin%2Fappearance as Administration theme - "Use the administration theme when editing or creating content " box can be checked or unchecked. Thus Seven is Administration theme, and Garland is site default theme.

Now come to http://localhost/d7/#overlay=admin%2Fstructure%2Ftaxonomy%2Fadd and "add vocabulary"

Now "add terms" - add any term

Now hit "edit" for that added term

Instead of Seven Garland shows up. ( It was expected that it will be still Seven)

What you want is - that in /admin this link goes to Seven but when you are in your Garland theme this link goes to Garland.

I confirm this...

Well that is what you want, isn't it. What you are now coming across is the behavior by core that makes sense, its not the most favorable.

Editing a taxonomy term is something you only do in admin or am I wrong??? So why does it jumps to Garland then? Im prety confused with this topic...

@aspilicious No, ofcourse not - it could very well be that editors terms is something you want your editors or registerd users to do :)

It's like editing a page or article, this also brings you to the seven theme...
I don't see the difference?

With the exception of below code review, I'm +1 for this patch with respect to its implementation, if there's agreement about the UI decision to treat adding/editing taxonomy terms as "content" with respect to the "Use the administration theme when editing or creating content" setting.

As to that UI decision, it fits with the way I think about taxonomy, but for me, there remains a WTF, which is why is "taxonomy" part of "structure" (i.e., on the "admin/structure" page), and if adding a taxonomy term is to be thought of as content rather than "admin", then why is the path for it "admin/structure/taxonomy/VOCABULARY/add"? So my concern is that a UI decision has already been made that taxonomy is structure rather than content, and unless we're revisiting that decision, then this might not be the right solution.

+++ modules/system/system.module	28 Jan 2010 21:30:00 -0000
@@ -3588,3 +3588,29 @@
+
+  return variable_get('theme_default');

Please remove this. Theme callbacks should only return overrides of defaults and let the theme system manage the default.

+++ modules/system/system.test	28 Jan 2010 21:30:00 -0000
@@ -1224,7 +1224,7 @@
-    parent::setUp();
+    parent::setUp('taxonomy');

I'm guessing the intention was to add a test, which I recommend doing, but I don't see the added test in this patch.

I'm on crack. Are you, too?

Yes, I agree. If taxonomy is content, we should refine #17. If taxonomy is structure, we need to roll a patch that uses the admin theme when editing a taxonomy term. What's currently in HEAD (admin theme for adding and default theme for editing) makes no sense.

.

sorry for all the spam. makes sense to change "component", however.

Thinking about this a little more, it seems the UI decision has already been made: taxonomy is structure not content (since it appears on the "admin/structure" page), and if there's desire to change that decision, it should be a separate issue. This issue is about the inconsistency between "add" and "edit". This patch fixes that to make "edit" work like "add". It's a simple change that does not need a test, as the functionality having a menu callback use the admin theme in this way is already tested for the "admin" path. Input from usability folks is probably still a good idea though, so leaving those tags.

This one includes a code comment.

I think this page probably needs to be added to hook_admin_paths() as well, at least until something gets done to unify those approaches at #669510: Merge administration theme with hook_admin_paths()...

Yes, system module uses it for all pages under admin/ by default, and other modules (e.g. node module) are using it too when necessary.

BTW, an issue surrounding that which you might be interested in :)
#651586: Improve the API documentation for hook_admin_paths()-related functions

Also there's an issue about breadcrumbs #576290: Breadcrumbs don't work for dynamic paths & local tasks

I think just this is all that is needed.

The way it goes is that any page which uses the administration theme should (by definition) also be labeled an administrative page, but the converse is not necessarily true, since some pages (e.g. node creation) always have an administrative functionality but don't make sense to show in an admin theme to all users on all sites. Yes, it's a bit crazy - which is why we need to fix it in the other issue, to make Drupal's admin theme feature more flexible :)

Added a comment.

I think that only a term edit should be changed, vocabularies are part of system.

+1 for test

jhodgdon: I mean that vocabulary add/edit/del should stay in admin/* as it is now but only term add/edit/del operations should be affected by settings in Appearance

jhodgdon: I just commented your #33

So the idea is that with this patch, all Taxonomy term/vocabulary editing should drop you into whatever you've set as your admin theme, right?

Only taxonomy term edit is affected by this patch

Also I ask about term delete theme.

I'm at the Drupalcon sprint so I'm going to try to build these tests. If you want to work together let me know in #drupal

Testing this is a little too intense for me right yet. If this still exists at the end of the day I'll get some more help and try again

Im at the sprint and I'll give it a go.

I have added a test case that checks to see if the admin theme switches correctly when adding a taxonomy term, to editing a taxonomy term. I needed to inject a meta tag (this could be done another way) but I had to add the current theme to the page somehow. I created a module to go along with this test as something was needed to fill this gap. The patch is for taxonomy.test and the module is standalone. The script will enable it during the test. Is there a better way to inject/extract the current theme from the page?

Jacked up my refactoring. New patch added.

It's gonna fail until the patch for the admin theme on edit is applied. And the added module is enabled, not sure on how else to present this. Let me know if something needs changing.

Fixed the file name.

I can add a wrapper that will fail out if the module isn't present or turned on, but I didn't know if this is the best way to handle it. That is where the notices is coming from as $matches[1] isn't set. Let me know.

This patch is a test on the patch submitted here. Another reason its failing is I had to create a module that injects the theme name into a meta tag so I could scrape the html output to actually verify that the theme did in fact change on these pages to the admin theme.

That make sense? It's kinda funky but I don't know another way to do this, I am open to suggestions.

-Dennis

New patch added with all relevant information.

I will verify this authors patch as I just applied their work to my checkout and made a complete patch. I will get my documentation up to D7 standards.

New patch added. I cleaned up the documentation and verified the authors patch. Removed his commenting as I don't think its needed in core modules? (Commenting in hook menu). Hopefully my commenting is better :).

Anyway to clarify the original authors patch, Yes you do need both spots. In hook_menu() he added the theme callback to display the admin theme when on the edit page for a taxonomy page. The hook_admin_paths function adds the same path to the overlay popup. So both spots are needed. The reason the "add" portion wasn't affect is its pre-pended by the path 'admin/structure...' etc and I think those paths are handled differently.

-d

Code sprint tag

Confirmed that this patch

- keeps both adding and editing a taxonomy term in the admin theme, regardless of the setting of admin theme with respect to editing content

Patch is good to go

When editing a term, the edit term screen in the Seven overlay works as expected as long as Clean URLs enabled. If clean URLs are disabled, when you click edit on a term, the edit screen is displayed in Seven, but not in an overlay.

If the above issue isn't part of this, please set the status back and I can issue a new bug report.

I was testing this patch because I found a similar issue when returning from the Edit of a Term. This works as expected with clean urls enabled, but would return a Page Not Found error in Seven overlay when disabled.

I would log this as a new bug report as this does the same thing for node edit when you edit from the content listing page.

-d

#60: taxonomy_admin_theme-678592-60.patch queued for re-testing.

+++ modules/taxonomy/taxonomy.test	21 Apr 2010 15:53:23 -0000
@@ -990,3 +990,101 @@
+  }
+}
\ No newline at end of file

no newline at the end of file

Powered by Dreditor.

Let's get this one in.

The previous test was a great start, but it also seems like a lot of code to add for what is a pretty small thing to test :) If we're going to search through the rendered HTML for an indication of what theme was used anyway..... then rather than injecting something specific it's probably simpler to just look for a CSS file from the theme (a technique which is already used in other tests we have in core).

Since the previous patch still needed some work for code style, etc, anyway and hadn't been worked on for a number of months, I decided to reroll with this simpler approach for the test.

sub

sub

#69 looks great.

Are we missing the term add and delete pages? As well as vocabulary add/edit/delete?

Yeah brain fart, I forgot those cases are already under admin/*...

Though technically this violates UX freeze, I think the WTF factor here of jumping back and forth is pretty significant.

Committed to HEAD. Thanks!