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

<?php
/**
* 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.

Files: 
CommentFileSizeAuthor
#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

Comments

Status:Active» Needs review
StatusFileSize
new602 bytes
new2.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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.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.

Status:Needs review» Reviewed & tested by the community

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

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.