There are several places in Drupal 8 that currently have code like the following:

 * Implements hook_custom_theme().
 * @todo Add an event subscriber to the Ajax system to automatically set the
 *   base page theme for all Ajax requests, and then remove this one off.
function edit_custom_theme() {
  if (substr(current_path(), 0, 5) === 'edit/') {
    return ajax_base_page_theme();

That's incorrect, since theme callback functions should be used instead. In addition to being ugly, the code is also error-prone since other hook_custom_theme() implementations might accidentally override it, plus the string-matching might accidentally match paths it doesn't intend to.

There's an issue at #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system that may wind up doing something to replace theme callbacks in Drupal 8 anyway. However, it's an easy patch to fix it here first, and that way the other issue can just do more of a find/replace on "theme callback" rather than dealing with these too, which if nothing else will make an eventual patch there easier to review.

#3 theme-callback-route-2011998-3.patch2.92 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es). View
#1 theme-callback-route-2011998-1.patch2.53 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 56,432 pass(es). View
#1 theme-callback-route-EXAMPLE-do-not-test.patch602 bytesDavid_Rothstein


David_Rothstein’s picture

Status: Active » Needs review
602 bytes
2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,432 pass(es). View

Here's a patch. I'm not sure exactly how to test it, but I did a bit of manual checking and it seems to be setting the correct Ajax theme (just like other Ajax-related paths in Drupal 8 which are already using "theme callback" do).

There was a question in #914382-133: Contextual links incompatible with render cache if this technique would work when the hook_menu() entry already has a corresponding route (as all these examples already do), but I believe it does. I've attached a "do-not-test" patch which can be used to try this out... after applying that patch and clearing caches, visiting admin/reports/updates will show you the page in Bartik. Also, looking at the code in menu_router_build() I don't see why it wouldn't work.

Crell’s picture

Status: Needs review » Needs work

Standardizing like this seems like a good idea, but we should probably mark them @todo be removed again by #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system. Otherwise I'm fine with this step 0.5.

David_Rothstein’s picture

Status: Needs work » Needs review
2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es). View

True, unlike other places where theme callback is used, these hook_menu() entries exist for no other purpose than the theme callback itself.

Attached patch adds the @todo.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Unless Alex says it would make his life harder, let's do.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c65c644 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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