Problem/Motivation

In drupal 7 there has been different ways to change the current active theme on the page:

  • You could use the 'theme callback' on the hook_menu() definition of the current page
  • You could use hook_custom_theme() which was triggered really early in the bootstrap

Proposed resolution

The idea is to add a similar kind of mechanism like breadcrumb does, so multiple services can set the current active theme,
if wanted. By default the active theme is determined from the config, but there is also a theme determined by the user setting.

If something like OG comes in , they would just implement the service and override if needed.

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)

User interface changes

(New or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text.)

API changes

This removes hook_custom_theme() and removes 'theme callback' from hook_menu()

@todo Fill out.

Original report

Hook menu allowed items to specify a theme callback which is called to determine the used theme.

  $items['system/ajax'] = array(
    'title' => 'AHAH callback',
    'page callback' => 'ajax_form_callback',
    'access callback' => TRUE,
    'theme callback' => 'ajax_base_page_theme',
    'type' => MENU_CALLBACK,
    'file path' => 'core/includes',
    'file' => 'form.inc',
  );

This has to happen somehow early in the request (before the actual _controller is called. It could be a method defined on the route or simply something manually called in your controller, though this feels wrong.

Files: 
CommentFileSizeAuthor
#245 interdiff.txt1.43 KBdawehner
#245 theme_callback-1954892.patch89.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,798 pass(es).
[ View ]
#243 theme_callback-1954892.patch89.9 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,884 pass(es), 14 fail(s), and 156 exception(s).
[ View ]
#226 theme_callback-1954892.patch89.85 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,374 pass(es).
[ View ]
#226 interdiff.txt2.68 KBdawehner

Comments

dawehner’s picture

Issue tags:+WSCCI

Adding a tag

Crell’s picture

Yeesh. I'll add a better question: Do we need to retain that functionality? Is it at all useful? It sounds like it's only useful for one-off custom code anyway, for which you can just as easily use hook_custom_theme() (or whatever that evolves into). I'd be more than happy to just see this go bye bye as redundant and vestigial.

dawehner’s picture

There are several places in core in which you probably want to have a shared functionality for the logic which theme is used. Here are some examples:

  • Every ajax request should be not rendered in the default theme of your site, but the theme which have been active in the request.
    So if you have an ajax thing going on in seven, it should be also rendered like seven/css added etc.
  • Theme inheritance: If some parent menu item is rendered in a certain theme, per default all child items does it as well.

hook_custom_theme() just feels wrong to be used :) What we could do, which would cover a lot of use-cases already, would be provide a common base controller class for ajax requests handling the logic in ajax_base_page_theme() for you already.

effulgentsia’s picture

This has to happen somehow early in the request (before the actual _controller is called.

I think the interesting question is whether we can change this for D8. Right now LegacyRequestSubscriber::onKernelRequestLegacy() initializes the theme ahead of calling hook_init(), so that hook_init() implementations can interact with the theme registry (e.g., call theme(), l(), or other functions that interact with the theme system). If we replace hook_init() with a pre-controller subscriber that isn't allowed to interact with the theme system and a separate hook called early by our generic controllers, then we can make theme initialization a responsibility of those generic controllers. However...

Theme inheritance: If some parent menu item is rendered in a certain theme, per default all child items does it as well.

If we want to retain this, then we're basically saying that it shouldn't be the controller's responsibility to initialize the theme, and rather should remain a request subscriber that does it based on route / menu link information.

Crell’s picture

My key word here is "simplify". Wide cascading implicit functionality is how we end up with unpredictable spaghetti. That means I'm more than happy to kill the theme key inheritance stuff.

dawehner: If you want to ensure a certain theme gets used by an Ajax request, couldn't you just specify the theme in the request, as a GET variable? It's not going to be human-visibile anyway. (I don't think that's a security hole?)

dawehner’s picture

StatusFileSize
new2.76 KB

Seriously this functionality can't be that important. All over core 'theme callback' is used, but the actual code in menu.inc is using 'theme_callback'. I'm wondering whether this could be a reason why some people have issue with the theme suggestions in views.

Once #1886448: Rewrite the theme registry into a proper service is in I would suggest the state to be on there, but for now we could just set menu_get_custom_theme() directly, damn that's ugly.

That's just an idea what we could do here.

David_Rothstein’s picture

Seriously this functionality can't be that important. All over core 'theme callback' is used, but the actual code in menu.inc is using 'theme_callback'.

Just to clarify, 'theme callback' is the hook_menu() entry, and 'theme_callback' is the database column in the {menu_router} table. So nothing is actually broken, just confusing (menu.inc has code to switch between those for a whole bunch of hook_menu() properties, including 'page callback'/'page_callback', etc).

The actual functionality is used in several places throughout Drupal core and works correctly.

dawehner’s picture

Oh i'm sorry.

effulgentsia’s picture

#6 won't work so long as LegacyRequestSubscriber calls drupal_theme_initialize(). However, maybe it makes sense to move what LegacyRequestSubscriber does into HtmlPageController? That way, AjaxController and other controllers could do something different. But, that then means every _controller (including route-specific ones) that needs to interact with the theme system becomes responsible for initializing it.

effulgentsia’s picture

StatusFileSize
new4.27 KB

However, maybe it makes sense to move what LegacyRequestSubscriber does into HtmlPageController?

The more I think about this, the more I think it makes sense:
- The entire concept of figuring out what theme to use and initializing it is an HTML concept. There's no need to force that code to run on autocomplete and rest.module requests.
- Per #1911728-23: Remove hook_init(), we want to distinguish between a hook that runs at the beginning of HTML page building from a general request event subscriber. The latter we already have a KernelEvents::REQUEST event for. The former needs to be called by controllers that deal with HTML after initializing the theme system. We may want to eventually rename the former from hook_init() to something else, but not doing so quite yet.
- Once #1812720: Implement the new panels-ish controller [it's a good purple] is figured out, we may want 'theme callback' to become a property of Displays. We shouldn't hold this issue up on that, but I think it's another pointer towards theme initialization being the responsibility of the controller.

But, that then means every _controller (including route-specific ones) that needs to interact with the theme system becomes responsible for initializing it.

I think that's ok, because there shouldn't need to be many route-specific controllers that deal with HTML: routes that return HTML should generally use _content, so that the content can be included in a full HTML page, an AJAX dialog, or any other HTML container. Meanwhile, for anyone writing a custom controller for wrapping HTML content (such as if Panels wants to swap out whatever core's default is, or if Views UI needs to customize its dialogs in a special way), it's not that hard to call drupal_theme_initialize(); module_invoke_all('init');. And this way, they can customize what they do prior to calling that.

So, let's start with this and see what breaks. Then we can merge in #6.

effulgentsia’s picture

Status:Active» Needs review
Crell’s picture

I think effulgentsia's point makes a lot of sense. I'm fine with shifting that logic to HtmlPageController or whatever replaces it. The question then is, so what do we do with this logic once we've shifted it there? :-) Putting a theme callback key on route objects doesn't sit well with me.

effulgentsia’s picture

Discussed in the wscci irc meeting. We concluded that meeting with the idea of removing 'theme callback' and just invoking hook_custom_theme() from HtmlPageController. However, I thought about it some more, and am now tempted to remove both 'theme callback' and hook_custom_theme(), and simply use either KernelEvents::REQUEST listeners or route enhancers to $request->attributes->set('_theme') to a theme literal (not a callback). Feedback welcome either now, or after I try it out and post a patch. I also want to chew on this a bit more to see if that would continue to make sense with the new scotch controller being worked on.

Crell’s picture

Actually, attributes[_theme] could work. I like the idea of removing both a global and a hook.

Would we then set _theme somewhere post-routing, and then say "and if you want something different, there's the request event, have fun" and call it a day?

dawehner’s picture

From my understanding the event approach seems to be way better then calling the hook from the HtmlPageController, because most of the usecases for this callback have been ajax requests, which will not take part in the HtmlPageController, but just on the AjaxController.

sdboyer’s picture

The entire concept of figuring out what theme to use and initializing it is an HTML concept. There's no need to force that code to run on autocomplete and rest.module requests.

strongly agree.

However, maybe it makes sense to move what LegacyRequestSubscriber does into HtmlPageController?

probably not. i know y'all changed your mind, and i'll get to that, but i'll splain why quickly first, from a SCOTCH perspective (as that's what replaces HtmlPageController). there's basically two ways we could do this within SCOTCH - record the theme to use as a property of the Display, or leave it up to the DisplayController directly. both are not good.

placing it into the display is bad because, while displays represent *almost* all of the page, they don't actually represent all of it: they're expected to (with the help of a controller) produce an HtmlFragment, not a full, finalized HTML response. they do not directly record the route(s) to which they are attached. more technically, they also contain a reference to a layout, which might be provided by a theme...maybe, the custom theme to which they are attached? that's starting to smell chicken-or-eggy. generally speaking, while displays still control a lot, they still operate within a particular injected context, and we should try to avoid violating that. themes are one of the contexts in which they operate.

the controller COULD do it, if it were a property set on the request attributes and thereby injected into the controller; at that point, it's just making a simple call. the only other way to do it would be to hardcode a desired theme on a controller...and that's a hilariously terrible idea. but even the former isn't worth doing, because...

simply use either KernelEvents::REQUEST listeners or route enhancers to $request->attributes->set('_theme') to a theme literal (not a callback)

this is how it should be done. not as a part of route enhancement, but on a KernelEvents::REQUEST subscriber. that subscriber should inspect the negotiated content type for the request, see that it is html, then bootstrap the appropriate theme. routes *could* allow that theme to be injected or manually switched by having a param or an internal property - the subscriber would try to read from those first, then fall back to configured defaults if nothing is found. we need to do it because i agree that @dawehner's AJAX case from #3 is indeed important. i also think there's *maybe* an interesting use case for allowing selective preview of any route in a named theme - all that would take is a contrib module that adds a route enhancer that looks for a particular GET param on any route, then slots that data into place in such a way that the core subscriber picks it up.

Damien Tournoud’s picture

The entire concept of figuring out what theme to use and initializing it is an HTML concept. There's no need to force that code to run on autocomplete and rest.module requests.

Maybe. Maybe not.

There are *many* things that want to generate HTML fragments. If you want to generate an HTML fragment, you have to have the theme layer initialized; and that cover things even as simple as l(). The Entity Reference autocomplete, to take just that example, supports generating HTML teasers via Views. Even sending out an (HTML) email requires the theme layer to be initialized.

So, no, it's not as easy as the theme layer is only needed for full-page HTML rendering.

On the other hand, tying up the choice of the theme to the path (as we do since Drupal 7) is *also* really wrong, especially because it doesn't cover things like system/js at all.

effulgentsia’s picture

Good points in #18. I think for this issue, the comments since #14 convince me that this issue's scope can just be about replacing hook_custom_theme() and 'theme callback' with request listeners / route enhancers, and not moving drupal_theme_initialize() out of LegacyRequestSubscriber quite yet, but rather opening a new issue for that once the menu_set_custom_theme() bit is gone. This is still high on my list to work on, but not assigning it to myself quite yet, in case someone's inspired to do it before I get to it.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new678 bytes

Just curious if anything fails if we move theme initialization to after when route enhancers run. If so, I'll open a spin-off issue for just that, before using them.

sun’s picture

FWIW, I fully agree with @Damien in #18.

The router-path-binding was a red herring right from the start, when 'theme callback' was originally introduced for D7. Architecturally, it always looked very wrong. Major concerns were raised too late in the release cycle at beta/RC stage though, so D7 maintainers decided to keep it.

That said, concerns were raised, but no one was able to figure out how this functionality/behavior could be re-implemented differently. The Ajax subsystem uses a custom request parameter to pass along the theme of interest already. To my knowledge, the primary consumer of theme callbacks today is Overlay module and/or closely related functionality.

That's completely independent from the request-specific hook_custom_theme() though, which exists in one way or another since many Drupal releases already, and is also what drives dynamic theme switching modules.

Crell’s picture

Related: #1886448: Rewrite the theme registry into a proper service

If the theme is a for-reals service, then couldn't all the theme init stuff turn into a factory object that gets stuff added to it to sort out the appropriate theme, which then gets returned and persisted? We're using that model in a few places; breadcrumbs is the latest one where we're talking about that. (Series of negotiation objects, first one to say it has a breadcrumb wins: #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service). Language resolution, I think, uses a similar model.

That would allow the theme system to truly lazy-init on first use, regardless of the request type; then hook_custom_theme() can go away and just be "add a new theme negotiator", and you don't need to explicitly initialize the theme because it will initialize when first called, which is what we want. If you need path- or router-specific logic, well, you're weird but you can write your own negotiator and shove it in there. (So per-user-theme-switching modules can still do what they need, but if they're not there we don't have any overhead.)

effulgentsia’s picture

Themes can implement (some) alter hooks (like hook_node_view_alter()), but not necessarily ones that run during bootstrap or for non-html (like hook_query_alter()). And whether or not to initialize the theme system for all invocations of l() isn't clear. If we can sort through those issues to come up with clarity around what "first use" is, then lazy initialization and resolution would be great.

Crell’s picture

Any update here? I think #1813762: Introduce unified interfaces, use dependency injection for interface translation will change the approach here somewhat, but we discussed in Portland what to do. Alex, do you need backing in doing it or is this still on your radar?

effulgentsia’s picture

Sorry for the delay on this. It's still on my radar after I get a couple other (unrelated) issues out of the way first. I'll try to resume on this at the end of this week.

David_Rothstein’s picture

The documented purpose of theme callbacks is for "pages whose functionality is very closely tied to a particular theme", and that's why it's part of the router.

This includes the batch and Ajax themes, but the canonical example is probably the block demo page. You would never want the block demo page to appear in another theme besides the one whose blocks are being displayed, since it wouldn't make any sense.

So besides the fact that using the theme callback cleans up the code (compared to having strpos() checks against the URL in a totally different part of the codebase) the other purpose it serves is to make sure that the above is respected. If, for example, a module implements hook_custom_theme() to have a theme dynamically chosen based on the user's role on "every" page on the site, we still don't want them to override the theme on places like the block demo page. The theme callback guarantees that won't happen. It's possible to deal with that in other ways (hook ordering, event priorities, etc) but it gets very messy very fast.

I'm not sure how the various proposals here would deal with all that?

effulgentsia’s picture

I'm still thinking something along the lines of #14: route enhancers that set the _theme "route default" (that's a confusing name, but basically, just means a request attribute determined in some way other than via a URL parameter). The reason it's called a default though is that it can be overridden via a URL parameter. So, the block demo page can be given a URL pattern of /admin/structure/block/demo/{_theme} in order to achieve the direct tying of theme to page mentioned in #27. This is similar to how Symfony's _format attribute is normally determined dynamically via various factors (e.g., Accept headers), but can also be added to a URL pattern like /node/{node}.{_format} for projects/routes that want to handle formats via URL rather than via Accept headers.

David_Rothstein’s picture

I think that makes sense although not sure how it would work for things like Ajax requests and the batch API - those pages should "never" have their theme overridden by other code either, so would they have to have the theme appear in a URL parameter too? It would be a little strange to have it in the URL in those cases, although I suppose it could work.

In any case, there are at least three places in Drupal 8 that are using string-parsing in hook_custom_theme() to set the Ajax theme (rather than theme callbacks) at the moment. I've thrown up a quick patch at #2011998: String-parsing in hook_custom_theme() should be replaced with theme callbacks (for now) to fix that. It's a pretty simple patch, but if it doesn't get committed for some reason this issue will have to fix those too. (One of the goals of that issue is to standardize everything that should be using "theme callback" currently to actually use it, which would make any patch here in this issue easier to review.)

effulgentsia’s picture

Assigned:Unassigned» effulgentsia

I'm starting on this tomorrow.

markcarver’s picture

Assigned:effulgentsia» Unassigned

Any other takers?

effulgentsia’s picture

Sorry folks. I ended up getting swamped on issues that were API freeze bound. I should have unassigned myself sooner. I won't be able to pick this up again for at least another week, so if anyone wants to work on it in the meantime, go for it.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.24 KB

Here is a general idea: provide a service which stores the active theme and theme list
and use that information in drupal_theme_initialize() now.

On the longrun theme "theme callback" could be translated to a listener which then manipulates the object.
Previous hook_custom_theme implementations could be also just another request listener.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new13.27 KB

This is a first start which converts the default (config based negotiation), the user based negotation (which probably fixes most of the test failures).
I am not sure yet about the "theme_callback" use-case, but I think the easiest way would be to just allow people to set it, by injecting the service into the custom controllers.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.78 KB
new13.76 KB

Let's fix the failures.

Crell’s picture

Services should be stateless. Using one as a pseudo-global doesn't improve anything.

I think what we'd discussed is storing the active theme on the request. If there's a service that depends on the active theme, then it depends on the request and should get the request set via a setRequest() method. (That way the new container in 2.3 is able to re-set the request when the scope changes without completely recreating the object.) That's primarily the theme object, which we keep trying to add in #1886448: Rewrite the theme registry into a proper service.

dawehner’s picture

I can't say that this is the optimal solution but setting the theme on the request object, even nothing requests it also feels wrong. For stuff like the themekey module a proper Interface certainly would also help to figure out how to do it properly.

Crell’s picture

Another option: Make active-theme resolution a chain-of-responsibility task, the same way breadcrumbs work and the serializer works. (Multiple resolvers, each one gets a shot in turn until one declares "this is the active theme, I so decree!")

dawehner’s picture

That is certainly a part what this patch was doing.
On the breadcrumb issue though you there is just a single place where you ask for the breadcrumb.

The active theme feels to have the potential to be asked multiple times so you would better put the result in some state.

Crell’s picture

I'm more comfortable with memoizing the result. At least then it's still idempotent within a request, although it makes it then request-aware... blech.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new5.56 KB
new11.82 KB

Removed the theme.active service and instead uses the attribute on the request

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new481 bytes
new11.82 KB

Urgs.

dawehner’s picture

Status:Needs work» Needs review

#46: drupal-1954892-46.patch queued for re-testing.

Crell’s picture

Priority:Normal» Major
Issue tags:+Needs issue summary update

Tagging

dawehner’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new1.93 KB
new13.48 KB

Damn Views!

pwolanin’s picture

Yes please, I still am sad that this got glommed onto hook_menu in 7

Crell’s picture

+++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
@@ -0,0 +1,93 @@
+    $this->determineActiveTheme($request);

There's no reason to call this in the constructor. There won't be any negotiators yet so there is nothing to do.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -1341,11 +1340,13 @@ public function render($display_id = NULL) {
       // Let the themes play too, because pre render is a very themey thing.
-      foreach ($GLOBALS['base_theme_info'] as $base) {
-        $module_handler->invoke($base, 'views_pre_render', array($this));
-      }
+      if (isset($GLOBALS['base_theme_info']) && isset($GLOBALS['theme'])) {
+        foreach ($GLOBALS['base_theme_info'] as $base) {
+          $module_handler->invoke($base, 'views_pre_render', array($this));
+        }

-      $module_handler->invoke($GLOBALS['theme'], 'views_pre_render', array($this));
+        $module_handler->invoke($GLOBALS['theme'], 'views_pre_render', array($this));
+      }

Why are these still using the global?

markcarver’s picture

dawehner’s picture

Here comes the implementation I would like to do on the longrun:

  • make the request sychronized and set it via setRequest
  • Look up in determineActiveTheme() whether '_theme_active' is already set and do set it if needed.

The question which arries is how people would access the information. If they should be able to use the attribute on the request, the active theme should be rather determined when setting the request on the theme negotiator ... but wait, someone would have to ask for it, so you actually want to always call the method, but then there is no point in storing it on the request? Maybe you could enlighten me, where the logical problem is in my point.

alexpott’s picture

Discussed with xjm and effulgentsia.

One use case of 'theme callback' is to replace the theme for ajax callbacks. It makes no sense to have hook_menu implementations for these routes. Therefore we have to address this issue.

dawehner’s picture

It really seems to be that we want to just have a service and don't rely on the magic _theme key on the request.

The account issue really has shown what kind of problems this can cause.

tim.plunkett’s picture

StatusFileSize
new13.5 KB

#1987636: Convert block_admin_demo() to a new style controller is the last page callback in block.module, but still uses theme callback, so helping reroll this.
And removing the determineActiveTheme() call from ThemeNegotiator::__construct()

catch’s picture

Priority:Major» Critical
dawehner’s picture

StatusFileSize
new11.5 KB
new24.88 KB

It would be cool to get #1979038: Convert views_ajax_autocomplete_taxonomy() to a Controller in, as it is one of the two instance which still rely on the old router and use theme callback => 'ajax_base_page_theme'.
The other one is #2066445: Convert a bunch of AjaxResponse callbacks in system.module's test's ajax_test.module to a new style controller

The question is how much should be done in this patch. It would be for sure possible to convert nearly all instances of 'theme callback' but this just blows up the patch size.

pwolanin’s picture

If those 2 blocking issues were in, we should convert them all. Since they are not in, I think this plus follow-up issues would be sufficient

twistor’s picture

+++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.phpundefined
@@ -0,0 +1,115 @@
+  public function determineActiveTheme(Request $request) {
+    foreach ($this->getSortedNegotiators() as $negotiator) {
+      if (($active_theme = $negotiator->determineActiveTheme($request)) && $active_theme !== NULL) {
+        $request->attributes->set('_theme_active', $active_theme);
+      }
+    }
+    return $request->attributes->get('_theme_active');

$active_theme !== NULL is unnecessary here. Missing a break?

+++ b/core/modules/contextual/contextual.routing.ymlundefined
@@ -1,5 +1,7 @@
+  options:
+    _ajax_base_page_theme: 'TRUE'

Can't we use content negotiation here?

dawehner’s picture

StatusFileSize
new3.24 KB
new25.13 KB

$active_theme !== NULL is unnecessary here. Missing a break?

My idea was that multiple theme negotiator services might override each other. For example the default one, will always set a value, but the one acting as user override, not all the time.

Can't we use content negotiation here?

Not really. At least at the moment this was explicit set on routes for certain ajax callbacks, but yeah drupal_ajax should maybe just be used?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new789 bytes
new25.39 KB

There we go.

David_Rothstein’s picture

Looking at the patch, it seems to have the problems discussed in #27 (especially if the goal is to replace hook_custom_theme() with this also, as it says in a @todo in the code). Pages which require a particular theme in order to function correctly currently have a way to "guarantee" that the theme will be used, but with this patch they'd be reduced to picking a big number for the priority and hoping for the best.

Because of that, the earlier ideas discussed here made more sense to me. There is only ever (at most) one theme callback per page currently, so the infrastructure to allow more than one seems unnecessary (in addition to causing the problem above).

Perhaps, though, there's a way to keep what this patch is doing but maintain the distinction by defining constants for the priorities? (Similar to how the theme layer has different CSS_* constants for CSS file ordering.) That could be a way to maintain the difference between the two groups that currently exist (theme callbacks and hook_custom_theme), while still combining them into one system.

dawehner’s picture

Because of that, the earlier ideas discussed here made more sense to me. There is only ever (at most) one theme callback per page currently, so the infrastructure to allow more than one seems unnecessary (in addition to causing the problem above).

I can understand that, but this is just true for the route/page callback specific themes. There is some logic for the default theme, and user override theme etc ...

Maybe we could apply a similar logic as we do have with access checkers. They can say on route build time, whether they apply on this route. This information is stored on there.

On the actual request then we just look at the route and see which theme negotiators exists on there. Yes, we still have to find a way how to handle the ordering.

Crell’s picture

We discussed this a bit in IRC. We *must* keep the COR implementation, since we may want to determine the theme before we get a route. That means just having a "callback and be done with it" approach won't work.

Constants for ordering sounds like a terrible idea to me, especially when we already have a priority on the services themselves that is more fine-grained to begin with.

I could see a case for a _theme_callback similar to _title_callback for one-off cases, which would then be used *only* by a specific negotiator that checks for that and delegates to it. That would resolve one-off complex cases while leaving the more common cases to the standard COR. (The _theme_callback would be a callable key, same syntax as _title_callback, _content, and _controller.) That could be a follow-up or done here if we decide to go that route; I'm comfortable either way.

donquixote’s picture

We discussed this a bit in IRC. We *must* keep the COR implementation, since we may want to determine the theme before we get a route.

I am all for COR, but I don't get this one.
How can we determine a theme before the route, if the determineActiveTheme() method of theme negotiator services needs the request as an argument?

Can you point out a use case for theme before route?

donquixote’s picture

I could see a case for a _theme_callback similar to _title_callback for one-off cases, which would then be used *only* by a specific negotiator that checks for that and delegates to it.

I could imagine a lot of things to add to route.yml..
I am just puzzled why whenever I suggest such a thing it is seen as "coupling" or illegitimately mixing different subsystems.
Imo it is all just about providing meta information to whatever subsystem that wants it, without these subsystems depending in any way on each other.

dawehner’s picture

We discussed this a bit in IRC. We *must* keep the COR implementation, since we may want to determine the theme before we get a route. That means just having a "callback and be done with it" approach won't work.

Does that mean that for example a service which does something based upon the route should be disabled? I can't see a usecase in determining the theme without having the route already available,
as what should be rendered?

Constants for ordering sounds like a terrible idea to me, especially when we already have a priority on the services themselves that is more fine-grained to begin with.

I do agree, this is just magic which removes flexibility and causes more problems.

That would resolve one-off complex cases while leaving the more common cases to the standard COR. (

Seems like a good idea for things like the batch based theme. For ajax_base_page_theme I would rather think to do that automatically on drupal_ajax requests.

David_Rothstein’s picture

I could see a case for a _theme_callback similar to _title_callback for one-off cases
...
(The _theme_callback would be a callable key, same syntax as _title_callback, _content, and _controller.) That could be a follow-up or done here if we decide to go that route; I'm comfortable either way.

That sounds reasonable to me, and also sounds like it's addressing the title/original goal of this issue, not a followup :)

Crell’s picture

Use cases:

1) I'm not sure, but we didn't think about menu link generation when designing the new access system and now that's coming back to bite us. :-) Same for param conversion. We designed those on the assumption that they're only relevant when processing an incoming request, which is an assumption that has proven incorrect. I'd rather not make the same mistake again.

2) The request event itself is able to set a Response object and short-circuit the entire rest of the process, jumping straight to the response event. (No controller, no controller resolver, no view event, no routing system invocation, nothing.) While that's not something we should do a lot of, I think it is a way to help with performance on some of our super-high-traffic routes. Specifically, I'm thinking of the 2-3 extra HTTP callbacks we get on every admin page load right now (for the toolbar, contextual links, etc.). If we can catch and process those with dedicated code in a request event before we even get to routing, great. That should help with performance. However, such special cases MAY involve a call to theme() somewhere (because Drupal), so we need theme() to not crap out pre-routing.

donquixote’s picture

Ok, so then let's have a "COR" (chain of responsibility) that works with and without routing.
This raises some questions though.
Does pre-route also mean pre-request?
Can determineActiveTheme() rely on receiving a request?
Can we have one ThemeNegotiator that requires a route, and another one that does not?
Should we have a separate method + interface for negotiators that don't need a route, or not even a request object?

The other question is in which order things happen.
I could see something like this:
- Theme layer relies on global state. This is ugly, but I think we won't fix this in D8. (Right?)

It would be great if in one request we could
- determine an operation A that needs theming. this could be handling a request, or maybe something else.
- determine a theme for this operation. set the global state accordingly.
- execute operation A.
- determine an operation B that needs theming. this could be handling a request, or maybe something else.
- determine a theme for this operation. set the global state accordingly.
- execute operation B.

Would this be somewhat realistic?
I'm a little concerned that otherwise premature calling of theme() can spoill any further thing we want to do in the PHP process.

donquixote’s picture

I'm a little concerned that otherwise premature calling of theme() can spoil any further thing we want to do in the PHP process.

Or, to ask in a different way:
If we already determined a theme without looking at a route, can it happen that later in the same PHP process we do get a route, and need to determine a new theme?

Crell’s picture

If we already determined a theme without looking at a route, can it happen that later in the same PHP process we do get a route, and need to determine a new theme?

... Damn. A fine point.

So that we don't get stuck in analysis paralysis, can we just move forward with the plan as is and sort out pre-routing theme() calls later? :-)

donquixote’s picture

can we just move forward with the plan as is

I'd say yes.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new23.21 KB

The last test failure seemed to be random.

catch’s picture

I'm not sure, but we didn't think about menu link generation when designing the new access system and now that's coming back to bite us. :-)

Who's "we"? https://drupal.org/node/1793520#comment-6543114

dawehner’s picture

Status:Needs work» Needs review
Issue tags:-API change, -WSCCI, -Approved API change

#79: theme-1954892-79.patch queued for re-testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new25.08 KB

Missed on file due to unrelated gitignore changes.

dawehner’s picture

Status:Needs work» Needs review

#84: theme-1954892-84.patch queued for re-testing.

disasm’s picture

This isn't a through code review, more high level of implementation.

+++ b/core/modules/contextual/contextual.routing.yml
@@ -1,5 +1,7 @@
+    _ajax_base_page_theme: 'TRUE'

Instead of having a separate option for every negotiator, why don't we come up with a default for negotiators to use. Something like _theme: 'ajax_base_page'

Other than that, I really like the approach to this. This is very extensible, and allows any module to create their own service and tag it as a theme negotiator, which I think resolves the issue better than theme_callback ever did.

dawehner’s picture

Had a talk with markus (maintainer of themekey)

He said that the later we determine the active theme the better. The general approach of the issue is a good way forward.
For example one major pain in D7 was that you could not really
use any kind of API like entity_load because modules haven't been booted yet.
With the DIC in D8 we now can use things like entity API and co. as everything is loaded properly.

There are multiple fixes in D8 which also help a lot:

  • L() does not trigger theme() anymore
  • t() does not trigger theme() anymore
  • Render arrays are used in even more places

As crell mentioned before, it will be problematic to ask for the theme when the route has not determined yet. Themekey for example want to be able to have the node object
on the request in order to decide what to do.

David_Rothstein’s picture

For example one major pain in D7 was that you could not really
use any kind of API like entity_load because modules haven't been booted yet.

Huh?

Take a look at _drupal_bootstrap_full() - the theme is definitely set after all modules have been loaded. Or am I missing something here?

dawehner’s picture

Take a look at _drupal_bootstrap_full() - the theme is definitely set after all modules have been loaded. Or am I missing something here?

I can't answer really why, but hook_init() is not called before hook_custom_theme() so if any modules is doing something in hook_init, like load another library etc. this fails.

dawehner’s picture

StatusFileSize
new36.24 KB

Let's see how much this passes.

There are sadly a couple of page callbacks left in the tests so I could not continue to rip out theme callback out of the menu router entirely.

dawehner’s picture

Status:Needs work» Needs review

.

Crell’s picture

In IRC discussion with dawehner, we came to the following conclusions:

- If you do something in a request listener that results in the theme system being called, route-sensitive theme negotiators should fail gracefully and simply not set a theme.
- If you do something in a request listener that results in the theme system being called, it's expected that you will set a Response object yourself, thus bypassing routing, controllers, and view listeners entirely. Vis, "you know what you're doing."
- If you do something in a request listener that results in the theme system being called, and you don't set a Response object, the theme system will not reinitialize later and any route-dependent theme logic will simply not happen. That means the page may render in the wrong theme. That's your own damned fault and we're not going to support you. :-)

David_Rothstein’s picture

Again that sounds similar to Drupal 6 (replace "request listener" with "hook_init"), and the result was a lot of bug reports about the wrong theme appearing on the page.

To solve the original goal of this issue I don't see why anything more than #73 is needed. Rewriting the theme determination system sounds like a separate (and presumably non-critical) issue; and see also #981654: Use several themes during the same page request which strikes me as the root of that problem (the theme being a global state which can only be set once so there's currently a chicken-and-egg problem of when to actually set it).

dawehner’s picture

This comes down to the following statement:

I could see a case for a _theme_callback similar to _title_callback for one-off cases, which would then be used *only* by a specific negotiator that checks for that and delegates to it. That would resolve one-off complex cases while leaving the more common cases to the standard COR. (The _theme_callback would be a callable key, same syntax as _title_callback, _content, and _controller.) That could be a follow-up or done here if we decide to go that route; I'm comfortable either way.

As far as I understand the statement this is optional on top of the system introduced by the patch. There are just so many usecases like themekey where a simple callback on the route definition would not help.

David_Rothstein’s picture

There are just so many usecases like themekey where a simple callback on the route definition would not help.

That is what hook_custom_theme() is for.

The original goal of this issue was just to convert the theme callback (and it's a critical issue because even though the theme callback is not too commonly used, it's part of the overall API change that moves all non-menu-link-related things out of hook_menu()). I don't really see why any other changes are critical, especially because they are getting complicated...

dawehner’s picture

The original goal of this issue was just to convert the theme callback (and it's a critical issue because even though the theme callback is not too commonly used, it's part of the overall API change that moves all non-menu-link-related things out of hook_menu()). I don't really see why any other changes are critical, especially because they are getting complicated...

Yes, this issue was originally just about 'theme callback' but why should this be a reason to not do it in a way how all the other subsystems (for example breadcrumb) is done. At least many involved in WSCCI agreed that we should move forward like that.

Crell’s picture

This issue is about removing theme callback properly; "properly" means "as part of a larger consistent approach to active theme determination." We're not replacing a global and guesswork with a one-off hack in hook_menu with a global and guesswork with a one-off hack somewhere else.

David, I appreciate trying to avoid scope creep, but this isn't scope creep. It's the same model we're using for breadcrumbs and a few other places. Doing half the job is no good.

#73 alone doesn't solve the issue. Fine, we have a callable... but called from where? From a theme negotiator that knows to look for it.

The "wrong theme" bug you're talking about was in Drupal 5, I believe, not Drupal 6, and related to hook_menu()'s dual-mode life. That is long-since resolved and we're not describing a system that would replicate that problem; or rather, it only would if someone does something dumb in a very small edge case. They'd need to initialize the theme system from a request listener and NOT return a Response themselves, AND do so on a request that would have resulted in one of the very few routes that have a theme callback specified. I'm comfortable with that small of a window for problems if it creates a more deterministic and consistent structure overall.

dawehner, can you reroll #92?

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+Needs issue summary update
StatusFileSize
new68.99 KB

The more I work on this patch the more I believe that having such a new architecture dramatically simplifies things.

Just the different ways how which case overrides the other one, depends on access to the theme etc. it is just hard to grok.

This iteration of the patch removes hook_custom_theme(), all 'theme callbacks' as well as cleans up the tables etc.

dawehner’s picture

StatusFileSize
new70.2 KB
new1.22 KB

Some places had 'theme_arguments' left.

David_Rothstein’s picture

It's a good goal (but not critical for Drupal 8) to improve the theme system, but the approach here is making it more fragile.

.... very small edge case. They'd need to initialize the theme system from a request listener and NOT return a Response themselves, AND do so on a request that would have resulted in one of the very few routes that have a theme callback specified.

It's not an edge case; in Drupal core the Overlay module does exactly this on every single admin page. It only doesn't cause a problem with the theme determination because the priority of the Overlay listener happens to be lower than the priority of the Symfony listener which sets the route. It's basically a lucky coincidence.

In any case, of the issues from Drupal 6 and earlier that is actually the smaller one. The bigger one is still #67. With the newest version of the patch, anyone who wants to set a theme without incorrectly interfering with the (former) theme callbacks has to choose an arbitrary priority that is lower than all of them (lower than zero, considering just the core ones in this patch). Once contrib gets into the mix, that isn't going to work.

(By the way, it's a little backwards that in this patch the lower priorities are taking precedence over the higher priorities, although I see why it naturally works out that way.)

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new75.25 KB
new7.86 KB

Some more test fixes.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

I have a couple of fixes I'm working on.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new11.21 KB
new80.71 KB

Drupal\system\Tests\Theme\ThemeTest->testAlter() will still need fixes, the removed drupal_theme_initialize() from LegacyRequestSubscriber means that ModuleHandler::alter() no longer can include themes in alter hooks until much later.

I'm removing the explicit dependency of field for user.module, that's not how we usually handle required modules.

tim.plunkett’s picture

  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
    @@ -157,8 +158,9 @@ function testNoShortcutLink() {
    -    // link appears on admin/content.
    -    $this->drupalGet('admin/content');
    +    // link appears on admin/people.
    +    $this->drupalGet('admin/people');

    So before, it checked admin/content/node, which was a 404 page, which it lets you add as a shortcut (LOL).
    admin/content doesn't work because it already has a shortcut. So I switched it to admin/people

  2. +++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/EventSubscriber/ActiveTrailSubscriber.php
    @@ -0,0 +1,68 @@
    +    if (!$this->trail && $this->state->get('menu_test.record_active_trail') ?: FALSE) {
    +      $this->trail = menu_get_active_trail();
    +      $this->state->set('menu_test.active_trail_initial', $this->trail);

    This is a replacement for what menu_test_custom_theme() was being used for.

tim.plunkett’s picture

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

Yeah, I can't quite figure that one out :(

dawehner’s picture

Well, as you said the theme is not initialized early enough, though the module handler relies on the global to be set. "All" you need is to inject the
theme service into the module handler and use it in order to determine the active theme. This could be problematic, as there might
be some alter hooks executed really early in the "bootstrap".

If you do this you realized that the cached module handler just extends the existing one instead of gets the existing one injected, but this is a follow up.

dawehner’s picture

Oh damn, there is a circular dependency as some of the theme negotiator services rely on the module handler. (AdminTheme => EntityManager => Module handler).

If we make the theme negotiator container aware we could pull the theme negotiators on runtime, so there is no explicit requirement.

tim.plunkett’s picture

From the linked issue about what hooks a theme should be able to implement:

We agreed that it's okay for themes to not have a chance to participate in alter hooks that run during bootstrap (e.g., hook_query_alter(), hook_url_inbound_alter()), but that the whole point of the feature is for them to participate in hook_form_alter(), hook_page_alter(), and similar hooks that relate to rendering.

So it's not necessary to make it happen THAT early, just early enough for route controllers.

dawehner’s picture

StatusFileSize
new3.81 KB

One way to break the cirular dependency theoretically is to not use constructor injection but setter injection.

Nevertheless we still have problems as the global in the module handler is not assumed to be active, so we can't drop the drupal_theme_initialize() yet.

jibran’s picture

I think patch is missing.

catch’s picture

Setter injection on the module handler seems sensible to me to break the circular dependency.

dawehner’s picture

No it is not as this won't work at all. I just wanted to throw out an idea.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
new85.71 KB

First, this is a rerole of the previous patch.

On the other hand this readds the initialization of the theme system to be after the routing system.

#2114321: Lazy inititialize the theme system by injecting the theme negotiator into places with uses global $theme is a follow up to deal with the problem of the circular dependency as well as figure out how to lazy-initialize stuff properly.

xjm’s picture

Issue tags:+Prague Hard Problems
dawehner’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -API change, -WSCCI, -Approved API change, -Prague Hard Problems

#120: theme-callback-1954892-120.patch queued for re-testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new703 bytes
new86.4 KB

Let's put some magic into that.

dawehner’s picture

StatusFileSize
new2.54 KB
new85.52 KB

Let's skip adding the table in all the changed tests in here.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new86.33 KB

Going back to #120 with one additional created users table.

tim.plunkett’s picture

StatusFileSize
new83.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,372 pass(es).
[ View ]

Rerolled, broken by #2046367: Document the internals of the router. No changes.

I think this is the most progress we can make in this issue.

Crell’s picture

Title:Figure out how to deal with 'theme callback'» Replace 'theme callback' with a clean theme negotiation system
  1. +++ b/core/core.services.yml
    @@ -160,6 +160,21 @@ services:
    +      - [setRequest, ['@request=']]

    I don't see this syntax, with the =, in the Symfony DIC docs. What does it do?

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -0,0 +1,134 @@
    +  public function getActiveTheme() {
    +    if (!$this->request->attributes->has('_theme_active')) {
    +      $this->determineActiveTheme($this->request);
    +    }
    +    return $this->request->attributes->get('_theme_active');
    +  }
    +

    I thought we were moving away from storing such data on the request? I don't know where else I'd put it, but it seems odd that we'd be moving things like the user out and then this in.

  3. +++ b/core/modules/edit/edit.routing.yml
    @@ -16,6 +17,9 @@ edit.field_form:
    +    _ajax_base_page_theme: 'TRUE'

    Is this an access check trigger? Usually we prefix them with _access to make that obvious. If it's not that... then I don't know what this is.

I'm sad that we can't get properly lazy-init out of this, but it is what it is for now. Also, retitling.

dawehner’s picture

StatusFileSize
new463 bytes
new84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,538 pass(es).
[ View ]

I don't see this syntax, with the =, in the Symfony DIC docs. What does it do?

The "=" is a syntax for synched services, which is a nice way how to deal with potential subrequest issues, let's skip this for now.
On the other hand thought I believe in passing in the request in a setter so the service can be reused also on possible multiple subrequests.

I thought we were moving away from storing such data on the request? I don't know where else I'd put it, but it seems odd that we'd be moving things like the user out and then this in.

To be honest I don't know a better place to put this, it just seemed fine as a static cache, which though is not exposed as an API. The API is the service, all the time.

Is this an access check trigger? Usually we prefix them with _access to make that obvious. If it's not that... then I don't know what this is.

This routing option is one way how to attach additional behavior that pulls the current theme from the ajax state passed in on drupal_ajax requests.
Do you have a better place to put that. Note: we could also ask the content negotiator once we get rid of subrequests.

catch’s picture

Couldn't the active theme be stored as a class property on the theme negotiation service itself?

Crell’s picture

catch: Then it's not properly a service. (Services should be stateless.) I'm not against it being on the request; I just found it odd.

As for the route option, maybe it's just a naming issue. I looked at that key and didn't have the slightest idea what it was doing. Maybe _theme: "from ajax"? Or something? (And then _theme: some_name_of_a_theme means that specific theme.)

dawehner’s picture

As for the route option, maybe it's just a naming issue. I looked at that key and didn't have the slightest idea what it was doing. Maybe _theme: "from ajax"? Or something? (And then _theme: some_name_of_a_theme means that specific theme.)

The question is also whether this belongs to the defaults or to the options key. It is hard to argue in favor of anything as the theme is kind of part of the content or at least controls
the output.

catch’s picture

catch: Then it's not properly a service. (Services should be stateless.) I'm not against it being on the request; I just found it odd.

We added a current user to avoid exactly the problem here (polluting the request object which was causing bugs all over the place), in a patch you personally RTBCed in #2062151: Create a current user service to ensure that current account is always available. Additionally #1858196: [meta] Leverage Symfony Session components will move session handling to a service. The request itself is a service, albeit a 'special' one - however abusing one service to hold state is not better than having two or three clearly defined services which are honest about what they're doing. So what exactly is the difference here?

tim.plunkett’s picture

Issue tags:+Needs followup

Even with current user, we first had _account on the request in order to make progress. I think we need to do the same thing here.

dawehner’s picture

The point is kind of different here. We never expose the _current_theme as some sort of api, so we just leverage the request as a request specific global attributes cache.
If people want to get the active theme they would always have to go directly through the theme negotiator service, which then could use the cached value on the request if needed.

Crell’s picture

As I said, I don't care that much... :-) The "derive the theme from ajax somehow" marker on routes is a bigger DX question, IMO.

dawehner’s picture

StatusFileSize
new84.33 KB
FAILED: [[SimpleTest]]: [MySQL] 59,408 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Just decided to go with '_theme: ajax_base_page', mainly because I could not find a better name.

catch’s picture

We never expose the _current_theme as some sort of api, so we just leverage the request as a request specific global attributes cache.

Do we actually prevent people from getting at it that way though?

dawehner’s picture

Status:Needs work» Needs review

#139: theme-1954892-139.patch queued for re-testing.

dawehner’s picture

Do we actually prevent people from getting at it that way though?

We could also just store it on the object and reset it everytime setRequest is called.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new399 bytes
new84.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,267 pass(es).
[ View ]

This just fixes the tests.

Crell’s picture

#143 sounds not unreasonable, I think.

(Side note: The presence of this property on the route would fall into the "not an API" category, IMO, and therefore is subject to change in 8.1 without notice since you're supposed to be using the theme negotiation service anyway.)

dawehner’s picture

Status:Needs work» Needs review

#145: theme-1954892-145.patch queued for re-testing.

dawehner’s picture

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

As I said in #129, I think this is as far as we'll get here (which is pretty darn far).

Let's get this in, and open a follow-up to discuss #143. I currently am not convinced its better than the current solution of storing it on the request, we can discuss more later.

webchick’s picture

Assigned:Unassigned» catch

Looks like catch has been all up in this one's bidness.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

#27/#67 is still an issue.

So, how to deal with that. In #67 I suggested adopting standardized priority constants for certain types of theme negotiators. This was not a popular idea, but looking at the responses I don't think they were in response to what I was actually suggesting (in fairness, I explained it pretty poorly).

In fact it could be as simple as a single constant:

<?php
 
// Define this as a very low number (since apparently in this patch low
  // priorities actually take precedence in determining the theme...)
 
const SOME_MAGIC_PRIORITY= -10000;
?>

Most theme negotiators would not use this, and would continue to use normal integers as priority. Only theme negotiators which "must win" for a particular route would use it. Since the priorities are defined in .yml files I have no idea how a constant would work in practice, but in any case, we need something like this.

An alternate solution would be to have two base classes that inherit from ThemeNegotiatorInterface, and one always wins out over the other. Each theme negotiator could choose to extend one or the other. However, that seems more awkward to me.

Some other smaller stuff:

  1.    public function onKernelRequestLegacy(GetResponseEvent $event) {
         if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
    -      menu_set_custom_theme();
    -      drupal_theme_initialize();
    -
           // Tell Drupal it is now fully bootstrapped (for the benefit of code that
           // calls drupal_get_bootstrap_phase()), but without having
           // _drupal_bootstrap_full() do anything, since we've already done the
    @@ -40,6 +37,16 @@ public function onKernelRequestLegacy(GetResponseEvent $event) {
       }

       /**
    +   * Initializes the theme system after the routing system.
    +   *
    +   * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
    +   *   The Event to process.
    +   */
    +  public function onKernelRequestLegacyAfterRouting(GetResponseEvent $event) {
    +    drupal_theme_initialize();
    +  }
    +
    +  /**
        * Registers the methods in this class that should be listeners.
        *
        * @return array
    @@ -47,6 +54,7 @@ public function onKernelRequestLegacy(GetResponseEvent $event) {
        */
       static function getSubscribedEvents() {
         $events[KernelEvents::REQUEST][] = array('onKernelRequestLegacy', 90);
    +    $events[KernelEvents::REQUEST][] = array('onKernelRequestLegacyAfterRouting', 30);

    Per #104 this still seems like trouble, and I don't understand what benefit moving the drupal_theme_initialize() call actually brings? Mostly it just seems to make priorities around 30 more magical (in the sense of "don't set your priority higher than that or you will probably break Drupal"). Then again, maybe the priorities around 30 are already magical in that respect...

  2.    // Only select the user selected theme if it is available in the
       // list of themes that can be accessed.
    -  $theme = !empty($user->theme) && drupal_theme_access($user->theme) ? $user->theme : \Drupal::config('system.theme')->get('default');
    +  $themes = list_themes();

    This comment is presumably out of date?

  3.    protected function doTestThemeCallbackInheritance() {
         theme_enable(array($this->admin_theme));
         $this->drupalGet('menu-test/theme-callback/use-admin-theme/inheritance');
    -    $this->assertText('Custom theme: seven. Actual theme: seven. Theme callback inheritance is being tested.', 'Theme callback inheritance correctly uses the administrative theme.');
    +    $this->assertText('Active theme: seven. Actual theme: seven. Theme negotiation inheritance is being tested.', 'Theme negotiation inheritance correctly uses the administrative theme.');
         $this->assertRaw('seven/style.css', "The administrative theme's CSS appears on the page.");
       }

    Didn't the patch remove inheritance? Why is it being tested?

    In general, I'm suspicious of many of these tests after the patch; I'm not convinced that all of them are meaningfully testing anything anymore.

David_Rothstein’s picture

Title:Replace 'theme callback' with a clean theme negotiation system» Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Issue tags:-Approved API change

Probably we're going ahead with something like this anyway, but I'm not convinced the "Approved API change" tag really applies anymore. The issue has expanded massively beyond what was required to fix the critical issue (and what was originally approved as an API change in #56).

Crell’s picture

Status:Needs work» Needs review

Routing happens at priority 32 right now. It's 32 because that's what it is in Symfony fullstack, and that seemed as good a place as any.

We do not have *any* magic constant priorities right now, and this is not the place to introduce them. There are probably plenty of other places where that might make hypothetical sense, but this is not the issue for them. Please open another one.

If I understand the code correctly, the idea here is not to let modules add their own super-special-no-*I*-win negotiators. Rather, the route can specify a custom theme, and one core-provided negotiator will catch it. The number of negotiators should be fairly small, not increasing per-module. So that's not even necessary here.

Please recheck the priority logic. It should be "big numbers win", because that's how priorities work. If that's not how the theme negotiators work then that's a bug, but I'd be surprised if that's the case.

Crell’s picture

Issue tags:+Approved API change

Also, Alex added the tag in #56, after we were already doing the "theme negotiator" approach. Please respect the decisions of the committers in that regard and let them decide that. Restoring tag.

dawehner’s picture

StatusFileSize
new84.23 KB
FAILED: [[SimpleTest]]: [MySQL] 59,661 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new6.3 KB

Also, Alex added the tag in #56, after we were already doing the "theme negotiator" approach. Please respect the decisions of the committers in that regard and let them decide that. Restoring tag.

You could argue that the approach was not clearly documented at that point, but from the actual history of the patches this is true.

Most theme negotiators would not use this, and would continue to use normal integers as priority. Only theme negotiators which "must win" for a particular route would use it. Since the priorities are defined in .yml files I have no idea how a constant would work in practice, but in any case, we need something like this.

As crell already said, we don't do that anywhere yet, though we could indeed. Alternative we might be able to bake that into the webprofiler toolbar integration, but this are dreams of the future.

Per #104 this still seems like trouble, and I don't understand what benefit moving the drupal_theme_initialize() call actually brings? Mostly it just seems to make priorities around 30 more magical (in the sense of "don't set your priority higher than that or you will probably break Drupal"). Then again, maybe the priorities around 30 are already magical in that respect...

Let's document twice that we want to execute code after the routing system.

Didn't the patch remove inheritance? Why is it being tested?

In general, I'm suspicious of many of these tests after the patch; I'm not convinced that all of them are meaningfully testing anything anymore.

I agree that many of these are pointless given that we don't have the concept of "custom" themes anymore.

catch’s picture

(Side note: The presence of this property on the route would fall into the "not an API" category, IMO, and therefore is subject to change in 8.1 without notice since you're supposed to be using the theme negotiation service anyway.)

Do you mean on the request or on the route?

If it's on the request, then how do you differentiate the non-API theme attribute on the request vs.all the ones that people are encouraged to get? For example the patch on #788900: Deprecate and remove usages of arg() added a tonne of calls to $request->attributes->get() and there's no way to differentiate between which of those are supposed to be retrieved vs. not.

catch’s picture

We could also just store it on the object and reset it everytime setRequest is called.

Wouldn't the request scope be enough?

Crell’s picture

catch: Yes, I meant request, not route. And that goes to the fact we need to document those properties that are API-ish, and probably explicitly note the ones that we know are not.

webchick’s picture

I'm not really up to speed on the particulars of this issue at all, but in the future, if there's a question about whether an "approved API change" tag is legit or not, probably best to assign the issue back to the core maintainer who set it and they can evaluate and make a determination.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new686 bytes
new84.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,863 pass(es).
[ View ]

Doh!

David_Rothstein’s picture

in the future, if there's a question about whether an "approved API change" tag is legit or not, probably best to assign the issue back to the core maintainer who set it and they can evaluate and make a determination.

That was more or less my intention in leaving the issue assigned to @catch, although you'd have to be a mind reader to know that... It needs "reapproval" given how the issue has expanded to include hook_custom_theme() and everything else, but anyway, let's just move on and see if the patch can be made to work. Drupal 8 is not coming out for a long time, and hook_custom_theme() is not a frequently-used API.

As crell already said, we don't do that anywhere yet, though we could indeed

So if we don't want to or can't introduce constants for priorities that basically leaves picking a very large number as a standard and using the number directly (for those that need it). I think it's bad developer experience. In any case, the current patch is still using the fallback number (zero) for those "high priority" ones, and that's wrong since that's how they will accidentally mix with others that aren't actually intended to be high priority.

Please recheck the priority logic. It should be "big numbers win", because that's how priorities work.

Priorities don't normally mean "big numbers win"; they mean "big numbers run first". The patches are respecting that, but the problem was that the last negotiator to set the theme was the one whose theme actually sticks. So if a developer were thinking (quite reasonably) "my attempt to set the theme is the most important so I should use a high priority for it" they'd actually have it backwards.

#162 fixes that by bailing out after the first theme is set (and not invoking any of the subsequent queued up negotiators), thereby making "big numbers win" and "big numbers run first" both true. Traditionally we've avoided that (with hooks) because it means the person who is writing an implementation can never be consistently sure that their code will be invoked on a particular request... In Symfony bailing out midway seems to be more common though (http://api.symfony.com/2.0/Symfony/Component/EventDispatcher/Event.html#...) and it may be the best we can do here. "Priority" is just a really confusing term.

Crell’s picture

Yes, Daniel and I discussed that in the WSCCI meeting this morning. The "first one to answer wins" model is something we're doing in a couple of places; that's how multiple routers works (although we just removed the old one), breadcrumbs do that, etc. It's a standard chain-of-responsibility pattern, nothing exotic unless you're only comparing against Drupal 7. But since we only want one active theme, it makes sense.

I'm not against having some constants for key priorities on certain services, but that's a larger discussion than the scope of this issue. Please open a new issue to discuss if/when/how we want to do that. (Symfony doesn't, but Symfony also tends more toward the whole system being built by one dev team, not hundreds on different modules.)

dawehner’s picture

So if we don't want to or can't introduce constants for priorities that basically leaves picking a very large number as a standard and using the number directly (for those that need it). I think it's bad developer experience. In any case, the current patch is still using the fallback number (zero) for those "high priority" ones, and that's wrong since that's how they will accidentally mix with others that aren't actually intended to be high priority.

The main reason why I went with the current schema of priorities is that new theme negotiators should be as easy as possible, so they don't have to set a priority.
Custom theme negotiators will most often be specific for a certain sets of routes, which ensures that the default ones still work on other pages.

catch’s picture

And that goes to the fact we need to document those properties that are API-ish, and probably explicitly note the ones that we know are not.

I'm not sure that's enough. Really I think we should consider moving all of these custom properties out of request attributes and instead into explicit, request-scoped services with their own propertiers and getters, it's just another big array of doom in there at the moment.

dawehner’s picture

I'm not sure that's enough. Really I think we should consider moving all of these custom properties out of request attributes and instead into explicit, request-scoped services with their own propertiers and getters, it's just another big array of doom in there at the moment.

Can we discuss about that in a followup?

catch’s picture

catch’s picture

Issue summary:View changes

updated summary

dawehner’s picture

162: theme-1954892-162.patch queued for re-testing.

tim.plunkett’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Thanks catch, it's good to know that fixing that here is now out of scope.
And thanks dawehner for keeping the patch green.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

From #163:

So if we don't want to or can't introduce constants for priorities that basically leaves picking a very large number as a standard and using the number directly (for those that need it).

This isn't done yet.

I think it's bad developer experience.

This will still be true once it is done :) A separate issue to discuss adding constants for priorities would probably be interesting, but my own interest for now is just trying to make the developer experience here work well.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new331.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,838 pass(es).
[ View ]
new1.43 KB

Here's a version that fixes the priorities but leaves the developer experience in place.

I get the feeling we could use a test to prove why the old ones wouldn't work. I'll think about how to do that.

David_Rothstein’s picture

I tried but don't have the patience right now to figure out how to write this code, sorry.

But here is what I figured a potential test might look like:

  1. Write a module with a theme negotiator that does not specify a priority (i.e. priority = 0). The module may need to start with the letter "a" so that it sorts ahead of other things that have no priority.
  2. Have it do something where it tries to set the theme on "all" page requests. (Realistic example: set the theme based on the current user's role. Simpler example for tests: set the theme whenever a test variable is TRUE.)
  3. Visit one of the key theme-based pages (e.g. the block demo page or an Ajax request URL) and assert that they still use the correct theme, not the theme that the above module tried to set.

This would be expected to fail with #162 but pass with #172.

It would also be great to document the standard somewhere ("if your page requires a certain theme to function correctly, set the priority on the theme negotiator to 1000" or something) but I'm not sure where that documentation could live, given that everything is just defined in standalone .yml files.

dawehner’s picture

I do get the point for the ajax base theme, as it should win against general theme negotiators but I don't get the point
on both Batch and Block demo. These ones are specific to a certain route, and so be easy override-able if you really need.

webchick’s picture

Issue tags:+alpha target

Since this falls squarely under "complete the routing system" it'd be great to have this in for Alpha 5 (nov 18). Tagging as such.

It seems like people are pretty frustrated in here. David, is there any chance you would be able to hop on IRC sometime soon to talk this over in "real time" with Tim/Daniel? I think that might help resolve the conflict here.

catch’s picture

Assigned:catch» Unassigned

Don't think this needs to be assigned to me at the moment.

David_Rothstein’s picture

I do get the point for the ajax base theme, as it should win against general theme negotiators but I don't get the point on both Batch and Block demo. These ones are specific to a certain route, and so be easy override-able if you really need.

It's the same issue.

If you view the block demo page for the Bartik theme, it needs to be displayed in the Bartik theme (or the page is broken). If there is another module that is trying to set the theme to something else on "all" pages (for example, use the Stark theme for users with a particular role) it needs to lose on this page.

This will be true to varying degrees for any former theme callback, so at least it's easy to write upgrade instructions.

It seems like people are pretty frustrated in here. David, is there any chance you would be able to hop on IRC sometime soon to talk this over in "real time" with Tim/Daniel? I think that might help resolve the conflict here.

I won't be on IRC much for the forseeable future, though I could theoretically schedule a time to jump on if necessary.

Are people frustrated about something here? As far as I can see this issue is pretty close.

David_Rothstein’s picture

StatusFileSize
new84.53 KB
PASSED: [[SimpleTest]]: [MySQL] 60,217 pass(es).
[ View ]

Wow, I just realized that #172 had a billion other files in it. Let's try that again.

tim.plunkett’s picture

Are people frustrated about something here?

Every four or five days you comment here about something keeping us from getting this committed (it has been RTBC twice, and might have been before that).

Every single day @dawehner and I are on IRC discussing how we are blocked on further menu system work by this issue.

So, yeah.

But is is encouraging to hear that you think this issue is pretty close.

David_Rothstein’s picture

@tim.plunkett, I see what you're saying, but I haven't been bringing up new issues; I've been bringing up the same thing I first raised in June and have been trying my best to come up with ideas for how to fix it (including the most basic suggestion of splitting off the non-critical parts of this issue to somewhere else, which would have unblocked the menu system work a long time ago). It's a fundamental problem, and the patch shouldn't have been RTBC while it still introduced this kind of problem.

That is why it would be great to have a test for this behavior... note that the previous code had a generic test for it which is being removed by the patch - see doTestThemeCallbackHookCustomTheme() - but with the way the new system works a generic test would be pointless since it would basically come down to asserting that a priority of 1000 is greater than a priority of 0 :) That is why I suggested more of a functional test in #173. But I might not have time to actually write it, and don't want to hold the issue up over that.

I do think the patch is still missing documentation of the whole system, and that is important; it removes the old documentation but doesn't really replace it. I would be happy to write this, but looking for advice on where it should live. Maybe general documentation should be on the ThemeNegotiatorInterface? That's the best I can think of, but if the documentation is focused more on things you add to a .yml file rather than things you do in the theme negotiator class, it seems a little backwards.

David_Rothstein’s picture

Status:Needs work» Needs review

178: theme-1954892-178.patch queued for re-testing.

David_Rothstein’s picture

(I retested because I tried one of those failed tests locally and it passed just fine for me.)

dawehner’s picture

StatusFileSize
new1.84 KB
new85.9 KB
FAILED: [[SimpleTest]]: [MySQL] 60,095 pass(es), 95 fail(s), and 10 exception(s).
[ View ]

Here is a test which ensures that overriding behavior.

I do think the patch is still missing documentation of the whole system, and that is important; it removes the old documentation but doesn't really replace it. I would be happy to write this, but looking for advice on where it should live. Maybe general documentation should be on the ThemeNegotiatorInterface? That's the best I can think of, but if the documentation is focused more on things you add to a .yml file rather than things you do in the theme negotiator class, it seems a little backwards.

Let's be honest, high level documentation is hard to get right directly in the sourcecode, which you will see ALL over core in Drupal 8.
Once the routing (and related) systems settled down we can think about a proper way and place to do that.

dawehner’s picture

Status:Needs work» Needs review

184: theme-1954892.patch queued for re-testing.

joachim’s picture

> Let's be honest, high level documentation is hard to get right directly in the sourcecode, which you will see ALL over core in Drupal 8.
> Once the routing (and related) systems settled down we can think about a proper way and place to do that.

My concern is that this is the way we end up with documentation written by people who don't understand what they're documenting -- if we get any at all!

Better to have the documentation in at the same time as the code, even if it's not quite in the right place. It's a much easier job to move / merge / edit existing documentation.

dawehner’s picture

Status:Needs work» Needs review

184: theme-1954892.patch queued for re-testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new87.34 KB
PASSED: [[SimpleTest]]: [MySQL] 60,361 pass(es).
[ View ]

The new test negotatior was missing from the actual patch.

I also added a small bit of documentation.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

There's now a test and documentation. Back up.

David_Rothstein’s picture

StatusFileSize
new87.68 KB
PASSED: [[SimpleTest]]: [MySQL] 60,077 pass(es).
[ View ]
new1.51 KB

Patch no longer applied, but it was a trivial reroll (minor conflict in system_menu()).

I also fixed up the documentation a bit and made it document the main thing that needed documenting (the priority). Pretty minor changes, so I think I can leave it at RTBC.

dawehner’s picture

Thank you for the reroll.

My actual idea in the issue was that you specify a really low priority for any kind of generic theme negotiator which should
take over on not all pages, but well I won't fight that any longer.

xjm’s picture

Issue tags:+beta blocker

Blocks #2107533: Remove {menu_router} which is a beta blocker.

xjm’s picture

catch’s picture

Status:Reviewed & tested by the community» Needs work

Overall I think this is OK. I'm not sure on the high priority vs. low priority by default discussions, but this is good enough for me to block router conversions.

However there's several smaller things that are unclear in the patch, CNW for those:

  1. +++ b/core/includes/theme.inc
    @@ -93,16 +93,14 @@ function drupal_theme_initialize() {
    +  // @todo Let the theme.negotiator listen to the kernel request event.

    OK with that @todo but let's please open an explicit follow-up for it.

  2. +++ b/core/includes/theme.inc
    @@ -117,7 +115,7 @@ function drupal_theme_initialize() {
    +  \Drupal::moduleHandler()->resetImplementations();

    This is changing a static cache reset to a cache storage delete operation, and won't that happen every request?

  3. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
    @@ -0,0 +1,69 @@
    +      // Ensure that the user only access a theme they are allowed to see.

    Sorry I don't get this comment. user_theme_access() (which I have to admit I'd never seen until I looked it up) doesn't do this at all. Is this just trying to ensure that the AJAX request uses the same theme as was intended - in that case it's not about access at all, but preventing arbitrary themes from being used for those requests (which might be valid for different requests for the same user).

    If all that's wrong as well, then please explain what's going on.

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
    @@ -0,0 +1,41 @@
    + * To set the active theme, create a new service tagged with 'theme_negotiator'

    We need to find a place to centrally document the tagged services in core, and also some higher level documentation about why certain things are tagged services vs. plugins vs. something else. This feels very inconsistent at the moment.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
    @@ -119,6 +119,17 @@ function testPreprocessForSuggestions() {
    +

    Extra line..

  3. +++ b/core/modules/system/lib/Drupal/system/Theme/AdminNegotiator.php
    @@ -0,0 +1,71 @@
    +    if ($this->entityManager->hasController('user_role', 'storage') && $this->user->hasPermission('view the administration theme') && path_is_admin($path)) {

    Shouldn't this just be in user module then? There's no concept of admin without roles.

  4. +/**
    + * Tracks the active trail.
    + */
    +class ActiveTrailSubscriber implements EventSubscriberInterface {
  5. I can't find this used anywhere in a test. Am I missing it or is this dead code left over from a previous patch?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new9.16 KB
new88.51 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Thank you for this great review!

OK with that @todo but let's please open an explicit follow-up for it.

Here is one: #2136087: Make the theme initialization lazy load or at least accept the need by rename LegacyRequestSubscriber

This is changing a static cache reset to a cache storage delete operation, and won't that happen every request?

This replaced a drupal_static_reset('drupal_alter') which won't work anyway, right? This static cache is not used any longer. Remove this line.

Sorry I don't get this comment. user_theme_access() (which I have to admit I'd never seen until I looked it up) doesn't do this at all. Is this just trying to ensure that the AJAX request uses the same theme as was intended - in that case it's not about access at all, but preventing arbitrary themes from being used for those requests (which might be valid for different requests for the same user).

If all that's wrong as well, then please explain what's going on.

We want to ensure that the user cannot change the active theme without having access to it. I am honest that the access checking in the theme negotiator manager should care about that,
but for now I just reverted to the comment and code as it was before.

      // Prevent a request forgery from giving a person access to a theme they
      // shouldn't be otherwise allowed to see. However, since everyone is allowed
      // to see the default theme, token validation isn't required for that, and
      // bypassing it allows most use-cases to work even when accessed from the
      // page cache.

I think the page cache point makes totally sense.

We need to find a place to centrally document the tagged services in core, and also some higher level documentation about why certain things are tagged services vs. plugins vs. something else. This feels very inconsistent at the moment.
One way could be to accept that drupal.org might be the better place to document such overview topics.

Shouldn't this just be in user module then? There's no concept of admin without roles.

I kept the hasStorage explicit as this code alternative has to be fixed in a lot of drupal unit tests.

I can't find this used anywhere in a test. Am I missing it or is this dead code left over from a previous patch?

This piece of functionality previously lived in a hook_custom_theme(), which does not longer exist.

-function menu_test_custom_theme() {
-  // When requested by one of the MenuTrailTestCase tests, record the initial
-  // active trail during Drupal's bootstrap (before the user is redirected to a
-  // custom 403 or 404 page). See menu_test_custom_403_404_callback().
-  if (\Drupal::state()->get('menu_test.record_active_trail') ?: FALSE) {
-    \Drupal::state()->set('menu_test.active_trail_initial', menu_get_active_trail());
-  }
-  if ($theme = \Drupal::state()->get('menu_test.hook_custom_theme_name') ?: FALSE) {
-    return $theme;
-  }
-}
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new569 bytes
new88.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Damn, middle mouse key.

tim.plunkett’s picture

+++ b/core/modules/user/user.services.yml
@@ -25,3 +25,13 @@ services:
+    class: Drupal\system\Theme\AdminNegotiator

Should be Drupal\user\Theme\AdminNegotiator

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new88.42 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Thank you!

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new552 bytes
new88.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,051 pass(es).
[ View ]

This should now really work.

dawehner’s picture

This should now really work.

dawehner’s picture

StatusFileSize
new87.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,953 pass(es).
[ View ]

Let's have a look at the rerole.

dawehner’s picture

Status:Needs work» Needs review

207: theme-1954892.patch queued for re-testing.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This looks like it adequately addresses catch's feedback.

catch’s picture

Title:Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» Change notice: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

error: core/modules/views/lib/Drupal/views/Tests/ViewPageControllerTest.php: does not exist in index - manually removed this hunk from the patch since it applied fine otherwise.

Committed/pushed to 8.x, thanks!

Will need a change notice.

dawehner’s picture

Status:Active» Needs review

Here is one: [#2139569]

xjm’s picture

Title:Change notice: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» [HEAD OFTEN BROKEN] Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Category:Task» Bug report
Priority:Major» Critical
Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs profiling
Related issues:+#2141501: [HEAD OFTEN BROKEN] Out-of-memory error causes test failures

Unfortunately this seems to have caused #2141501: [HEAD OFTEN BROKEN] Out-of-memory error causes test failures, so we need a revert here. I emailed the four 8.x maintainers asking them to:
git revert fc04601

Then we probably need to do some debugging and profiling on the patch.

xjm’s picture

Title:[HEAD OFTEN BROKEN] Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» [NEEDS ROLLBACK] Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system

Clearer title.

catch’s picture

Status:Reviewed & tested by the community» Needs work
catch’s picture

Title:[NEEDS ROLLBACK] Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Category:Bug report» Task
dawehner’s picture

xjm’s picture

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new86.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Patch just conflicted with some moved use statements, no changes.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new86.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,190 pass(es).
[ View ]

s/plugin.manager.entity/entity.manager

dawehner’s picture

StatusFileSize
new15.63 KB
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,274 pass(es).
[ View ]

Thank you for rerolling this patch.

I decided to join damian on #2150621: Separate applies and build logic for breadcrumb builders and add a second method, to make things cleaner.

dawehner’s picture

StatusFileSize
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,320 pass(es).
[ View ]
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es).
[ View ]
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,215 pass(es).
[ View ]
new89.84 KB
FAILED: [[SimpleTest]]: [MySQL] 59,348 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,278 pass(es).
[ View ]
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,366 pass(es).
[ View ]
new89.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]

Let's see whether they all pass.

jibran’s picture

Some minor issues.

  1. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
    @@ -0,0 +1,90 @@
    +      // shouldn't be otherwise allowed to see. However, since everyone is allowed

    more then 80 chars.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -0,0 +1,142 @@
    +   *   Priority of the breadcrumb builder.
    ...
    +   *   An array of breadcrumb builder objects.

    breadcrumb??

  3. +++ b/core/tests/Drupal/Tests/Core/Theme/ThemeNegotiatorTest.php
    @@ -0,0 +1,191 @@
    + * Tests the theme negotiator.

    @group missing i think.

dawehner’s picture

StatusFileSize
new2.68 KB
new89.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,374 pass(es).
[ View ]

Good points!

dawehner’s picture

Status:Needs work» Needs review

226: theme_callback-1954892.patch queued for re-testing.

dawehner’s picture

Status:Needs work» Needs review

226: theme_callback-1954892.patch queued for re-testing.

Cottser’s picture

Status:Needs work» Needs review

226: theme_callback-1954892.patch queued for re-testing.

joelpittet’s picture

StatusFileSize
new332.73 KB

Looks like we have a new Theme Negotiator: awesome:)

Theme Negotiator

Cottser’s picture

226: theme_callback-1954892.patch queued for re-testing.

The last submitted patch, 226: theme_callback-1954892.patch, failed testing.

The last submitted patch, 226: theme_callback-1954892.patch, failed testing.

The last submitted patch, 223: theme_negotiator-1954892.patch, failed testing.

The last submitted patch, 223: theme_negotiator-1954892.patch, failed testing.

tim.plunkett’s picture

Parent issue:#2107533: Remove {menu_router}» #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
Related issues:-#1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)+#2107533: Remove {menu_router}
dawehner’s picture

226: theme_callback-1954892.patch queued for re-testing.

dawehner’s picture

I just don't get it, this tests needs 14MB of ram on my system.

dawehner’s picture

StatusFileSize
new89.9 KB
FAILED: [[SimpleTest]]: [MySQL] 59,884 pass(es), 14 fail(s), and 156 exception(s).
[ View ]

This is a simple rerole and should finally be more stable.

Status:Needs review» Needs work

The last submitted patch, 243: theme_callback-1954892.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new89.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,798 pass(es).
[ View ]
new1.43 KB

This should be it.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Confirmed in IRC that there's no substantive change here, just head-chasing while we sorted out the memory issues. Back to RTBC.

alexpott’s picture

So do we understand the memory issues that were caused by the original commit?

Crell’s picture

They weren't caused by the original commit. Our testbot memory had been increasing for a while, and this issue just happened to push it over; straw that broke the camel's back and all that. The for-reals fix is in the Doctrine parser; I don't have an issue handy for that, but dawehner and chx should.

Cottser’s picture

catch’s picture

Title:Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» Change notice; Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Let's try this again.

Committed/pushed to 8.x, thanks!

dawehner’s picture

jibran’s picture

Title:Change notice; Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system» Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Priority:Major» Critical
Status:Active» Fixed
Issue tags:-Needs issue summary update

Change notice it well written and easy to understand. so marking as fixed. It only needs Theme Negotiator.

Berdir’s picture

This introduced a performance regression in UserNegotiator and I think it's also broken, see #2163035: Remove $user->theme and Drupal\user\Theme\UserNegotiator.

Status:Fixed» Closed (fixed)

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

larowlan’s picture

Issue tags:+theme service

retro adding the theme service tag

sun’s picture