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.
Comment | File | Size | Author |
---|---|---|---|
#3 | theme-callback-route-2011998-3.patch | 2.92 KB | David_Rothstein |
#1 | theme-callback-route-2011998-1.patch | 2.53 KB | David_Rothstein |
#1 | theme-callback-route-EXAMPLE-do-not-test.patch | 602 bytes | David_Rothstein |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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.
Comment #2
Crell CreditAttribution: Crell commentedStandardizing 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.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedTrue, 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.
Comment #4
Crell CreditAttribution: Crell commentedUnless Alex says it would make his life harder, let's do.
Comment #5
alexpottCommitted c65c644 and pushed to 8.x. Thanks!