ThemeNegotiator::determineActiveTheme() should not require a RouteMatch to be passed in

+++ b/core/lib/Drupal/Core/Theme/ChainThemeNegotiatorInterface.php
@@ -0,0 +1,35 @@
+/**
+ * Defines an interface for a chained service which determines the active theme.
+ */
+interface ChainThemeNegotiatorInterface extends ThemeNegotiatorInterface {

The naming is consistent with both ChainRouter and ChainBreadcrumbBuilder, so +1 from me.

Having the extra getActiveTheme() method worries me a little, though. I can see why it's there and what the advantage is, but one of the nice things about reusing the interface directly is that I can wire a single router or a single breadcrumb builder up directly, without any negotiation, and everything still works. With this additional method that's not the case here.

Back in #2134513: [policy, no patch] Trait strategy, some people expressed concerns about both trait proliferation and setter injection. I'm not really all that bothered by either, but this probably needs a review from someone who is.

If we're going to remove RouteMatch from the passed parameters, just make it a constructor parameter for the negotiators that care. #3 makes the negotiators non-stateless, even though they're registered as services. We can simplify it by just passing the RouteMatch to each negotiator in its constructor, no trait, no muss, no fuss. And then we have a unified interface between the negotiators and the aggregating service, which we want; we probably should still add a chain interface anyway as with breadcrumbs and the patch from #1, it just won't need anything but an addX() method.

No chain interface?

Other than that, #9 looks good to me.

Assuming the bot is still happy I think we're good here. Thanks, Tim!

The issue summary and issue title seems wrong. The patch itself looks good to me though.

Updated a relevant change record for this and the original RouteMatch issue: https://www.drupal.org/node/2158619/revisions/view/6783809/7398343

In addition to all of the other classes implementing ThemeNegotiatorInterface, ThemeNegotiator is the one acting as the service, and has the extra methods to gather and delegate. This is similar to BreadcrumbManager, which implements BreadcrumbBuilderInterface.

Doesn't that mean that there should be two separate interfaces? As a developer of a theme negotiator I cannot imagine that you would ever NOT want to have the route match passed along.

As a developer of a theme negotiator I cannot imagine that you would ever NOT want to have the route match passed along.

@dawehner: What's the benefit of having it passed-in vs. constructor injected. I don't see a down side to #12's approach of using CI, but I'd like to understand your concern more.

The way I see it, it's about explicit configuration/state of a service vs. call-time/context-specific arguments.

When moving RouteMatch + Request into the constructor of theme negotiators, the same change must include a new architectural facility to "reset" all theme negotiators, so that a new context can be negotiated.

Thus far, I interpreted theme negotiators as simple I/O processing plugins. You get some context in, you return a result based on that context.

I should be able to trigger theme negotiation for an arbitrary/ad-hoc request/context and get a proper result back. This may happen and be necessary for rendering e.g. theme-specific blocks (HTML fragments) as part of a master request. Sub-requests may or may not be another use-case.

I was confused by the previous Request → RouteMatch injection change already. (Even more so by losing access to the current Request that corresponds to the matched Route - which doesn't make sense.)

Already in HEAD, the most trivial theme negotiator implementation possible (?theme= GET query parameter matching) suddenly had to (1) introduce state and (2) became considerably more complex:

+use Drupal\Core\Routing\RouteMatchInterface;
+use Symfony\Component\HttpFoundation\RequestStack;
...
+ protected $requestStack;
+
+ public function __construct(RequestStack $request_stack) {
+   $this->requestStack = $request_stack;
+ }
...
- public function applies(Request $request) {
-   return (bool) $request->query->has('theme');
+ public function applies(RouteMatchInterface $route_match) {
+   return (bool) $this->requestStack->getCurrentRequest()->query->has('theme');
  }
...
- public function determineActiveTheme(Request $request) {
-   return $request->query->get('theme');
+ public function determineActiveTheme(RouteMatchInterface $route_match) {
+   return $this->requestStack->getCurrentRequest()->query->get('theme');

IMHO, theme negotiators should be atomic I/O plugins; i.e.:

1. The theme negotiator gets a RouteMatch context in, it returns the negotiation result (if any).
2. The RouteMatch context supplies access to all ancestors + related data, including the Request.
3. A theme negotiator service MAY be stateful, but its persistent state may only hold persistent configuration.

There was a major regression for anyone trying to find out what the current active theme is.

Noone disagrees with that and the route match issue was one of these "let's push it through because this is one less beta blocker" issues.

If my (working) approach is going to be held up, it'd be nice if someone else proposed an alternative.

I hope it is okay to criticize other people.

Well, my approach would be: Have one interface for the main theme negotiator and one for the sub-thingies. Here is an excerpt of HEAD april 23th (last checkout on this machine):

class ThemeNegotiator ... {
  public function getActiveTheme() {
    $request = $this->requestStack->getCurrentRequest();
    if (!$request->attributes->has('_theme_active')) {
      $this->determineActiveTheme($request);
    }
    return $request->attributes->get('_theme_active');
  }

  /**
   * {@inheritdoc}
   */
  public function applies(Request $request) {
    return TRUE;
  }

  /**
   * {@inheritdoc}
   */
  public function determineActiveTheme(Request $request) {
  }
}

I think we should certainly change internal details of our code if it helps both the DX of end-developers and if needed add complexity, but yeah I am so out of things that I would not trust me.

I continue to think the patch in #12 is correct, and I disagree with dawehner and sun.

We have multiple independent services for getting data about or derived from a request: 'request_stack', 'current_route_match', 'current_user', etc. At an API level, these are independent, meaning none of them have methods for getting another. ThemeNegotiatorInterface doesn't know which of these a particular negotiator will base its decision on. Therefore, it makes no sense to pass any of them to determineActiveTheme(). Instead, each negotiator can specify what it uniquely needs in its constructor.

I should be able to trigger theme negotiation for an arbitrary/ad-hoc request/context

What's an actual use case of that?

This may happen and be necessary for rendering e.g. theme-specific blocks (HTML fragments) as part of a master request. Sub-requests may or may not be another use-case.

Is there any legitimate use case to do the first without a sub-request? As to sub-requests, constructor injection is 100% compatible with that. Each negotiator's *.services.yml entry can either wire in services that are internally sub-request compatible ('request_stack', 'current_route_match', 'current_user' all are), or else, the service can be flagged as having request scope, in which case it will be automatically re-instantiated for subrequests (this is no longer Symfony's recommended approach, but it's an available option for contrib to use if necessary).

I agree with the reasoning in comment #24. It doesn't feel like now is the time to dramatically change RouteMatch (per #19.2). It is certainly worth exploring having a 'mega object' that gives access to everything, but maybe that is best left for Drupal 9?

Data always wins. Here is a list of theme negotiators in core (left out the test ones and ThemeNegiator itself):

We have multiple independent services for getting data about or derived from a request: 'request_stack', 'current_route_match', 'current_user', etc. At an API level, these are independent, meaning none of them have methods for getting another. ThemeNegotiatorInterface doesn't know which of these a particular negotiator will base its decision on. Therefore, it makes no sense to pass any of them to determineActiveTheme(). Instead, each negotiator can specify what it uniquely needs in its constructor.

As said before, the common case is to decide based upon the current request, so information coming from the $request and extracted into the $route_match. Applying your point the other way round, you could also argue that access(AccountInterface $account) should NOT get passed in the account. We added the $account on there because it is the better DX

The table in #28 is for the applies() method. However, for determineActiveTheme() there's no such pattern. AdminDemo uses $route_match, Batch and Ajax use $request, and the rest use nothing. So, how about we continue to pass $route_match to applies(), but remove it from determineActiveTheme(), which is all this issue title is asking for anyway?

Reviewing #1 because it is my favourite

1. +++ b/core/lib/Drupal/Core/Theme/ChainThemeNegotiatorInterface.php
   @@ -0,0 +1,35 @@
   +  /**
   +   * Determines the active theme.
   +   *
   

   We should explicit document that this method is the method you want to use instead of determineActiveTheme

2. +++ b/core/lib/Drupal/Core/Theme/ChainThemeNegotiatorInterface.php
   @@ -0,0 +1,35 @@
   +}
   \ No newline at end of file
   

   ups

3. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -108,4 +113,11 @@ public function determineActiveTheme(RouteMatchInterface $route_match) {
   +   * {@inheritdoc}
   +   */
   +  public function getActiveTheme() {
   +    return $this->determineActiveTheme($this->routeMatch);
   +  }
   +
   

   Should we maybe store the result somewhere?

Stealing @webchick's tag, seems appropriate here.

#29 makes sense

Re-roll of #1 though it looks like part of this has been done already because there is no current_route_match on cache_context.theme service.

A quick re-roll due to a coding standards change.
#2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

I'm quite confused as the title doesn't match the actual patch at all!

Note: #2472337: Provide a lazy alternative to service collectors which just detects service IDs will also make this interface not useful anymore.

Rerolling this patch

Btw https://www.drupal.org/pift-ci-job/1153771 shows 7 coding standards messages

Solved, problems with coding standard.

> I'm quite confused as the title doesn't match the actual patch at all!

I'm seeing several approaches in patches. Tagging for this.

This also seems to be potentially at odds with #2471657: Just load the theme negotiators which are needed, which says:

> Convert \Drupal\Core\Theme\ThemeNegotiatorInterface::applies to accept a route object