- Patch #669510 by quicksketch, David_Rothstein, Dave Reid, casey, Gábor Hojtsy, mrfelton, effulgentsia: merge administration theme with hook_admin_paths().

15 files changed, 124 insertions, 81 deletions

diff --git a/includes/menu.inc b/includes/menu.inc
index b19abdbb4..382f02cd4 100644
--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -1651,15 +1651,14 @@ function menu_get_custom_theme($initialize = FALSE) {
     if (!empty($custom_themes)) {
       $custom_theme = array_pop($custom_themes);
     }
-    // Otherwise, execute the theme callback function for the current page, if
-    // there is one, in order to determine the custom theme to set.
-    if (!isset($custom_theme)) {
-      $router_item = menu_get_item();
-      if (!empty($router_item['access']) && !empty($router_item['theme_callback']) && function_exists($router_item['theme_callback'])) {
-        $theme_name = call_user_func_array($router_item['theme_callback'], $router_item['theme_arguments']);
-        if (drupal_theme_access($theme_name)) {
-          $custom_theme = $theme_name;
-        }
+    // If there is a theme callback function for the current page, execute it.
+    // If this returns a valid theme, it will override any theme that was set
+    // by a hook_custom_theme() implementation above.
+    $router_item = menu_get_item();
+    if (!empty($router_item['access']) && !empty($router_item['theme_callback']) && function_exists($router_item['theme_callback'])) {
+      $theme_name = call_user_func_array($router_item['theme_callback'], $router_item['theme_arguments']);
+      if (drupal_theme_access($theme_name)) {
+        $custom_theme = $theme_name;
       }
     }
   }
diff --git a/modules/book/book.module b/modules/book/book.module
index a59de8179..25f0254d2 100644
--- a/modules/book/book.module
+++ b/modules/book/book.module
@@ -174,7 +174,6 @@ function book_menu() {
     'page arguments' => array(1),
     'access callback' => '_book_outline_access',
     'access arguments' => array(1),
-    'theme callback' => '_node_custom_theme',
     'type' => MENU_LOCAL_TASK,
     'weight' => 2,
     'file' => 'book.pages.inc',
@@ -185,7 +184,6 @@ function book_menu() {
     'page arguments' => array('book_remove_form', 1),
     'access callback' => '_book_outline_remove_access',
     'access arguments' => array(1),
-    'theme callback' => '_node_custom_theme',
     'file' => 'book.pages.inc',
   );
 
@@ -210,11 +208,13 @@ function _book_outline_remove_access($node) {
  * Implements hook_admin_paths().
  */
 function book_admin_paths() {
-  $paths = array(
-    'node/*/outline' => TRUE,
-    'node/*/outline/remove' => TRUE,
-  );
-  return $paths;
+  if (variable_get('node_admin_theme')) {
+    $paths = array(
+      'node/*/outline' => TRUE,
+      'node/*/outline/remove' => TRUE,
+    );
+    return $paths;
+  }
 }
 
 /**
diff --git a/modules/help/help.test b/modules/help/help.test
index db739f345..8033c4c0e 100644
--- a/modules/help/help.test
+++ b/modules/help/help.test
@@ -22,7 +22,7 @@ class HelpTestCase extends DrupalWebTestCase {
     $this->getModuleList();
 
     // Create users.
-    $this->big_user = $this->drupalCreateUser(array('access administration pages', 'administer permissions'));
+    $this->big_user = $this->drupalCreateUser(array('access administration pages', 'view the administration theme', 'administer permissions'));
     $this->any_user = $this->drupalCreateUser(array());
   }
 
diff --git a/modules/node/node.module b/modules/node/node.module
index 062078908..9a5651350 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -255,16 +255,18 @@ function node_uri($node) {
  * Implements hook_admin_paths().
  */
 function node_admin_paths() {
-  $paths = array(
-    'node/*/edit' => TRUE,
-    'node/*/delete' => TRUE,
-    'node/*/revisions' => TRUE,
-    'node/*/revisions/*/revert' => TRUE,
-    'node/*/revisions/*/delete' => TRUE,
-    'node/add' => TRUE,
-    'node/add/*' => TRUE,
-  );
-  return $paths;
+  if (variable_get('node_admin_theme')) {
+    $paths = array(
+      'node/*/edit' => TRUE,
+      'node/*/delete' => TRUE,
+      'node/*/revisions' => TRUE,
+      'node/*/revisions/*/revert' => TRUE,
+      'node/*/revisions/*/delete' => TRUE,
+      'node/add' => TRUE,
+      'node/add/*' => TRUE,
+    );
+    return $paths;
+  }
 }
 
 /**
@@ -1932,7 +1934,6 @@ function node_menu() {
     'title' => 'Add content',
     'page callback' => 'node_add_page',
     'access callback' => '_node_add_access',
-    'theme callback' => '_node_custom_theme',
     'file' => 'node.pages.inc',
   );
   $items['rss.xml'] = array(
@@ -1979,7 +1980,6 @@ function node_menu() {
     'page arguments' => array(1),
     'access callback' => 'node_access',
     'access arguments' => array('update', 1),
-    'theme callback' => '_node_custom_theme',
     'weight' => 0,
     'type' => MENU_LOCAL_TASK,
     'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
@@ -1991,7 +1991,6 @@ function node_menu() {
     'page arguments' => array('node_delete_confirm', 1),
     'access callback' => 'node_access',
     'access arguments' => array('delete', 1),
-    'theme callback' => '_node_custom_theme',
     'weight' => 1,
     'type' => MENU_LOCAL_TASK,
     'context' => MENU_CONTEXT_INLINE,
@@ -2003,7 +2002,6 @@ function node_menu() {
     'page arguments' => array(1),
     'access callback' => '_node_revision_access',
     'access arguments' => array(1),
-    'theme callback' => '_node_custom_theme',
     'weight' => 2,
     'type' => MENU_LOCAL_TASK,
     'file' => 'node.pages.inc',
@@ -2023,7 +2021,6 @@ function node_menu() {
     'page arguments' => array('node_revision_revert_confirm', 1),
     'access callback' => '_node_revision_access',
     'access arguments' => array(1, 'update'),
-    'theme callback' => '_node_custom_theme',
     'file' => 'node.pages.inc',
   );
   $items['node/%node/revisions/%/delete'] = array(
@@ -2033,7 +2030,6 @@ function node_menu() {
     'page arguments' => array('node_revision_delete_confirm', 1),
     'access callback' => '_node_revision_access',
     'access arguments' => array(1, 'delete'),
-    'theme callback' => '_node_custom_theme',
     'file' => 'node.pages.inc',
   );
   return $items;
@@ -2069,17 +2065,6 @@ function node_page_title($node) {
   return $node->title;
 }
 
-/**
- * Theme callback for creating and editing nodes.
- */
-function _node_custom_theme() {
-  // Use the administration theme if the site is configured to use it for
-  // nodes.
-  if (variable_get('node_admin_theme')) {
-    return variable_get('admin_theme');
-  }
-}
-
 function node_last_changed($nid) {
   return db_query('SELECT changed FROM {node} WHERE nid = :nid', array(':nid' => $nid))->fetch()->changed;
 }
diff --git a/modules/shortcut/shortcut.test b/modules/shortcut/shortcut.test
index abb531ccc..3c5a4f34e 100644
--- a/modules/shortcut/shortcut.test
+++ b/modules/shortcut/shortcut.test
@@ -34,7 +34,7 @@ class ShortcutTestCase extends DrupalWebTestCase {
   function setUp() {
     parent::setUp('toolbar', 'shortcut');
     // Create users.
-    $this->admin_user = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'create article content', 'create page content', 'access content overview'));
+    $this->admin_user = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'view the administration theme', 'create article content', 'create page content', 'access content overview'));
     $this->shortcut_user = $this->drupalCreateUser(array('customize shortcut links', 'switch shortcut sets'));
 
     // Create a node.
diff --git a/modules/simpletest/tests/batch.test b/modules/simpletest/tests/batch.test
index e4aab37f2..b7bfc9bfb 100644
--- a/modules/simpletest/tests/batch.test
+++ b/modules/simpletest/tests/batch.test
@@ -299,12 +299,16 @@ class BatchPageTestCase extends DrupalWebTestCase {
     // is using a different theme than would normally be used by the batch API.
     variable_set('theme_default', 'bartik');
     variable_set('admin_theme', 'seven');
+    // Log in as an administrator who can see the administrative theme.
+    $admin_user = $this->drupalCreateUser(array('view the administration theme'));
+    $this->drupalLogin($admin_user);
     // Visit an administrative page that runs a test batch, and check that the
     // theme that was used during batch execution (which the batch callback
     // function saved as a variable) matches the theme used on the
     // administrative page.
     $this->drupalGet('admin/batch-test/test-theme');
-    // The stack should contain the name of the the used on the progress page.
+    // The stack should contain the name of the theme used on the progress
+    // page.
     $this->assertEqual(batch_test_stack(), array('seven'), t('A progressive batch correctly uses the theme of the page that started the batch.'));
   }
 }
diff --git a/modules/simpletest/tests/menu.test b/modules/simpletest/tests/menu.test
index 7a4560408..f46b81df4 100644
--- a/modules/simpletest/tests/menu.test
+++ b/modules/simpletest/tests/menu.test
@@ -159,26 +159,35 @@ class MenuRouterTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Test that the result of hook_custom_theme() overrides the theme callback.
+   * Test that hook_custom_theme() can control the theme of a page.
    */
   function testHookCustomTheme() {
     // Trigger hook_custom_theme() to dynamically request the Stark theme for
     // the requested page.
     variable_set('menu_test_hook_custom_theme_name', 'stark');
+    theme_enable(array('stark'));
 
-    // Request a page whose theme callback returns the Seven theme. Since Stark
-    // is not a currently enabled theme, our above request should be ignored,
-    // and Seven should still be used.
-    $this->drupalGet('menu-test/theme-callback/use-admin-theme');
-    $this->assertText('Custom theme: seven. Actual theme: seven.', t('The result of hook_custom_theme() does not override a theme callback when it returns a theme that is not enabled.'));
-    $this->assertRaw('seven/style.css', t("The Seven theme's CSS appears on the page."));
+    // Visit a page that does not implement a theme callback. The above request
+    // should be honored.
+    $this->drupalGet('menu-test/no-theme-callback');
+    $this->assertText('Custom theme: stark. Actual theme: stark.', t('The result of hook_custom_theme() is used as the theme for the current page.'));
+    $this->assertRaw('stark/layout.css', t("The Stark theme's CSS appears on the page."));
+  }
 
-    // Now enable the Stark theme and request the same page as above. This
-    // time, we expect hook_custom_theme() to prevail.
+  /**
+   * Test that the theme callback wins out over hook_custom_theme().
+   */
+  function testThemeCallbackHookCustomTheme() {
+    // Trigger hook_custom_theme() to dynamically request the Stark theme for
+    // the requested page.
+    variable_set('menu_test_hook_custom_theme_name', 'stark');
     theme_enable(array('stark'));
+
+    // The menu "theme callback" should take precedence over a value set in
+    // hook_custom_theme().
     $this->drupalGet('menu-test/theme-callback/use-admin-theme');
-    $this->assertText('Custom theme: stark. Actual theme: stark.', t('The result of hook_custom_theme() overrides what was set in a theme callback.'));
-    $this->assertRaw('stark/layout.css', t("The Stark theme's CSS appears on the page."));
+    $this->assertText('Custom theme: seven. Actual theme: seven.', t('The result of hook_custom_theme() does not override what was set in a theme callback.'));
+    $this->assertRaw('seven/style.css', t("The Seven theme's CSS appears on the page."));
   }
 
   /**
@@ -811,6 +820,19 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
     $perms = array_keys(module_invoke_all('permission'));
     $this->admin_user = $this->drupalCreateUser($perms);
     $this->drupalLogin($this->admin_user);
+
+    // This test puts menu links in the Navigation menu and then tests for
+    // their presence on the page, so we need to ensure that the Navigation
+    // block will be displayed in all active themes.
+    db_update('block')
+      ->fields(array(
+        // Use a region that is valid for all themes.
+        'region' => 'content',
+        'status' => 1,
+      ))
+      ->condition('module', 'system')
+      ->condition('delta', 'navigation')
+      ->execute();
   }
 
   /**
diff --git a/modules/simpletest/tests/menu_test.module b/modules/simpletest/tests/menu_test.module
index b3577dbdc..4d673e8af 100644
--- a/modules/simpletest/tests/menu_test.module
+++ b/modules/simpletest/tests/menu_test.module
@@ -58,6 +58,11 @@ function menu_test_menu() {
     'page arguments' => array(TRUE),
     'access arguments' => array('access content'),
   );
+  $items['menu-test/no-theme-callback'] = array(
+    'title' => 'Page that displays different themes without using a theme callback.',
+    'page callback' => 'menu_test_theme_page_callback',
+    'access arguments' => array('access content'),
+  );
   // Path containing "exotic" characters.
   $path = "menu-test/ -._~!$'\"()*@[]?&+%#,;=:" . // "Special" ASCII characters.
     "%23%25%26%2B%2F%3F" . // Characters that look like a percent-escaped string.
diff --git a/modules/system/system.api.php b/modules/system/system.api.php
index 4079c613f..be46d5d10 100644
--- a/modules/system/system.api.php
+++ b/modules/system/system.api.php
@@ -1049,13 +1049,18 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
  *   - "access arguments": An array of arguments to pass to the access callback
  *     function, with path component substitution as described above.
  *   - "theme callback": (optional) A function returning the machine-readable
- *     name of the default theme that will be used to render the page. If this
- *     function is provided, it is expected to return a currently-active theme
- *     on the site (otherwise, the main site theme will be used instead). If no
- *     function is provided, the main site theme will also be used, unless a
- *     value is inherited from a parent menu item. In all cases, the results of
- *     this function can be dynamically overridden for a particular page
- *     request by modules which implement hook_custom_theme().
+ *     name of the theme that will be used to render the page. If not provided,
+ *     the value will be inherited from a parent menu item. If there is no
+ *     theme callback, or if the function does not return the name of a current
+ *     active theme on the site, the theme for this page will be determined by
+ *     either hook_custom_theme() or the default theme instead. As a general
+ *     rule, the use of theme callback functions should be limited to pages
+ *     whose functionality is very closely tied to a particular theme, since
+ *     they can only be overridden by modules which specifically target those
+ *     pages in hook_menu_alter(). Modules implementing more generic theme
+ *     switching functionality (for example, a module which allows the theme to
+ *     be set dynamically based on the current user's role) should use
+ *     hook_custom_theme() instead.
  *   - "theme arguments": An array of arguments to pass to the theme callback
  *     function, with path component substitution as described above.
  *   - "file": A file that will be included before the page callback is called;
@@ -2050,10 +2055,12 @@ function hook_theme_registry_alter(&$theme_registry) {
  * Return the machine-readable name of the theme to use for the current page.
  *
  * This hook can be used to dynamically set the theme for the current page
- * request. It overrides the default theme as well as any per-page or
- * per-section theme set by the theme callback function in hook_menu(). This
- * should be used by modules which need to override the theme based on dynamic
- * conditions.
+ * request. It should be used by modules which need to override the theme
+ * based on dynamic conditions (for example, a module which allows the theme to
+ * be set based on the current user's role). The return value of this hook will
+ * be used on all pages except those which have a valid per-page or per-section
+ * theme set via a theme callback function in hook_menu(); the themes on those
+ * pages can only be overridden using hook_menu_alter().
  *
  * Since only one theme can be used at a time, the last (i.e., highest
  * weighted) module which returns a valid theme name from this hook will
diff --git a/modules/system/system.install b/modules/system/system.install
index 5e0d5b8d2..ba16f8bdf 100644
--- a/modules/system/system.install
+++ b/modules/system/system.install
@@ -2927,6 +2927,17 @@ function system_update_7066() {
 }
 
 /**
+ * Allow all users to view the administration theme.
+ */
+function system_update_7067() {
+  // Preserve the site's current behavior of automatically allowing all users
+  // to view the administration theme whenever they have access to an
+  // administrative page.
+  user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('view the administration theme'));
+  user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('view the administration theme'));
+}
+
+/**
  * @} End of "defgroup updates-6.x-to-7.x"
  * The next series of updates should start at 8000.
  */
diff --git a/modules/system/system.module b/modules/system/system.module
index 43e55c433..4459683d4 100644
--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -230,6 +230,10 @@ function system_permission() {
     'access site in maintenance mode' => array(
       'title' => t('Use the site in maintenance mode'),
     ),
+    'view the administration theme' => array(
+      'title' => t('View the administration theme'),
+      'description' => variable_get('admin_theme') ? '' : t('This is only used when the site is configured to use a separate administration theme on the <a href="@appearance-url">Appearance</a> page.', array('@appearance-url' => url('admin/appearance'))),
+    ),
     'access site reports' => array(
       'title' => t('View site reports'),
     ),
@@ -544,8 +548,6 @@ function system_menu() {
     'page callback' => 'system_admin_menu_block_page',
     'weight' => 9,
     'menu_name' => 'management',
-    'theme callback' => 'variable_get',
-    'theme arguments' => array('admin_theme'),
     'file' => 'system.admin.inc',
   );
   $items['admin/compact'] = array(
@@ -1841,7 +1843,7 @@ function system_init() {
   // Add the CSS for this module. These aren't in system.info, because they
   // need to be in the CSS_SYSTEM group rather than the CSS_DEFAULT group.
   drupal_add_css($path . '/system.base.css', array('group' => CSS_SYSTEM, 'every_page' => TRUE));
-  if (arg(0) == 'admin' || (variable_get('node_admin_theme', '0') && arg(0) == 'node' && (arg(1) == 'add' || arg(2) == 'edit' || arg(2) == 'delete'))) {
+  if (path_is_admin(current_path())) {
     drupal_add_css($path . '/system.admin.css', array('group' => CSS_SYSTEM));
   }
   drupal_add_css($path . '/system.menus.css', array('group' => CSS_SYSTEM, 'every_page' => TRUE));
@@ -1895,6 +1897,15 @@ function system_add_module_assets() {
 }
 
 /**
+ * Implements hook_custom_theme().
+ */
+function system_custom_theme() {
+  if (user_access('view the administration theme') && path_is_admin(current_path())) {
+    return variable_get('admin_theme');
+  }
+}
+
+/**
  * Implements hook_form_FORM_ID_alter().
  */
 function system_form_user_profile_form_alter(&$form, &$form_state) {
diff --git a/modules/system/system.test b/modules/system/system.test
index d32b3ae3e..65c3c587a 100644
--- a/modules/system/system.test
+++ b/modules/system/system.test
@@ -1355,7 +1355,7 @@ class SystemThemeFunctionalTest extends DrupalWebTestCase {
   function setUp() {
     parent::setUp();
 
-    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer themes', 'bypass node access'));
+    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'view the administration theme', 'administer themes', 'bypass node access'));
     $this->drupalLogin($this->admin_user);
     $this->node = $this->drupalCreateNode();
   }
diff --git a/modules/taxonomy/taxonomy.module b/modules/taxonomy/taxonomy.module
index e2e99a983..fd473e226 100644
--- a/modules/taxonomy/taxonomy.module
+++ b/modules/taxonomy/taxonomy.module
@@ -299,8 +299,6 @@ function taxonomy_menu() {
     'page arguments' => array('taxonomy_form_term', 2),
     'access callback' => 'taxonomy_term_edit_access',
     'access arguments' => array(2),
-    'theme callback' => 'variable_get',
-    'theme arguments' => array('admin_theme'),
     'type' => MENU_LOCAL_TASK,
     'weight' => 10,
     'file' => 'taxonomy.admin.inc',
diff --git a/modules/taxonomy/taxonomy.test b/modules/taxonomy/taxonomy.test
index 7d5533b77..b31f7898b 100644
--- a/modules/taxonomy/taxonomy.test
+++ b/modules/taxonomy/taxonomy.test
@@ -1153,8 +1153,8 @@ class TaxonomyThemeTestCase extends TaxonomyWebTestCase {
     variable_set('admin_theme', 'seven');
 
     // Create and log in as a user who has permission to add and edit taxonomy
-    // terms.
-    $admin_user = $this->drupalCreateUser(array('administer taxonomy'));
+    // terms and view the administrative theme.
+    $admin_user = $this->drupalCreateUser(array('administer taxonomy', 'view the administration theme'));
     $this->drupalLogin($admin_user);
   }
 
diff --git a/modules/translation/translation.module b/modules/translation/translation.module
index af295bd20..2fb710e50 100644
--- a/modules/translation/translation.module
+++ b/modules/translation/translation.module
@@ -65,7 +65,6 @@ function translation_menu() {
     'access arguments' => array(1),
     'type' => MENU_LOCAL_TASK,
     'weight' => 2,
-    'theme callback' => '_node_custom_theme',
     'file' => 'translation.pages.inc',
   );
   return $items;
@@ -89,10 +88,12 @@ function _translation_tab_access($node) {
  * Implements hook_admin_paths().
  */
 function translation_admin_paths() {
-  $paths = array(
-    'node/*/translate' => TRUE,
-  );
-  return $paths;
+  if (variable_get('node_admin_theme')) {
+    $paths = array(
+      'node/*/translate' => TRUE,
+    );
+    return $paths;
+  }
 }
 
 /**