Problem/Motivation

Note this blocks many issues: this is blocking #2858482, see #2858482-26: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent. Which is in turn blocking #2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle(). Both of which are blocking #2737751: Refactor REST routing to address its brittleness. And finally, this is blocking #2822201: Allow contrib modules to deploy REST resources by creating routes, removing the need for config entities in that special case.

A lot of services use the service collector. This is great, but it loads all dependencies all the time. However, a lot of the time we don't want all the things. It also forces creating services for the whole dependency graph of each of these collected services

Also, as each dependency is added using a setter method, each service using this generally has to take care to sort by priority itself. So this ends up being implmented many times.

Proposed resolution

Create another service collector implementation that just collects an ordered array of service IDs. Services using this can then assume the list is ordered, and use the IDs to lazily create instances. The only down side is that they need to be container aware, that's the trade off but worth it I think.

Remaining tasks

None

User interface changes

N/A

API changes

Addition: service_id_collector tag that can be used to collect an array of service IDs and pass to the implementing service constructor.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Given we have the ServiceCollectorHandlersPass generic and an example in the param converters issue, I think this can be handled by a novice.

fgm’s picture

This is actually the TaggedHandlersPass, not ServiceCollectorHandlersPass. The ParamConverters issue is #2470926: Don't load all the paramconverters on every request..

kfitz’s picture

Title: Provide an lazy alternative to service collectors which just detects service IDs » Provide a lazy alternative to service collectors which just detects service IDs
dawehner’s picture

I still think that this would be super useful.

Fabianx’s picture

I agree 100% that this would be super-useful.

Proxies are nice, but they don't necessarily always make sense just to make something lazy ...

damiankloip’s picture

FileSize
8.22 KB

So we could do something like this maybe. Basically keep it in the handler pass, create two helper methods, one for tagged services and one for just the IDs.

Fabianx’s picture

#6 looks great.

dawehner’s picture

Issue tags: +Needs tests

I'd suggest to convert at least one of the usecases we already have in core so we know that the change fits our needs.

damiankloip’s picture

Agree totally. That's just a rough patch. Needs a conversion and test coverage for sure!

damiankloip’s picture

Working on this now.

damiankloip’s picture

Status: Active » Needs review
Issue tags: -D8 Accelerate Dev Days, -Novice, -Needs tests
FileSize
15.96 KB
9.21 KB

Here is some extended test coverage and a conversion of the ThemeNegotiator. The good point is that we don't really need to worry about the priorities or sorting at run time, as the pass can sort and the array will be ordered.

Status: Needs review » Needs work

The last submitted patch, 11: 2472337-11.patch, failed testing.

damiankloip’s picture

Just need to fix the ThemeNegotiatorTest, figures.

dawehner’s picture

It is quite nice that we can remove parts of ThemeNegotiatorTest now.

  1. +++ b/core/core.services.yml
    @@ -411,8 +411,10 @@ services:
    +    calls:
    +      - [setContainer, ['@service_container']]
    

    We could use parent: container.trait

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -82,90 +82,141 @@ class TaggedHandlersPass implements CompilerPassInterface {
    +      throw new LogicException(sprintf("At least one service tagged with '%s' is required.", $tag));
    

    Let's just use inline vars now, this is IMHO much more readable

damiankloip’s picture

Status: Needs work » Needs review
FileSize
23.36 KB
7.9 KB

How's about that?

EDIT: Forgot the change that exception message! Let's see how the bot gets on now anyway.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

Issue summary looks perfect!

damiankloip’s picture

Issue summary: View changes
damiankloip’s picture

Status: Reviewed & tested by the community » Needs work

Oh, we need to add to the docs for this I guess... :)

dawehner’s picture

Issue tags: +Needs change record

I guess we also need a CR

damiankloip queued 15: 2472337-15.patch for re-testing.

Fabianx’s picture

RTBC + 1 - once we have docs.

Nice work!

bojanz’s picture

My silly docs fix landed (#2504013: Remove @see statements from the ThemeNegotiator docblock) and now this patch doesn't apply.

Here's a reroll.

damiankloip’s picture

Status: Needs work » Needs review
damiankloip’s picture

FileSize
27.64 KB
4.73 KB

Docs, eventually. Needs some review, and very likely some tweaking.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -15,24 +15,28 @@
    + * service definition arguments (constructor injection) in that a consuming
    + * service MAY allow further processors to be added dynamically at runtime. This
    + * is why the called method (optionally) receives the priority of a processor as
    

    Ah there we talk about priorities.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -40,26 +44,34 @@ class TaggedHandlersPass implements CompilerPassInterface {
        * Example (YAML):
        * @code
        * tags:
        *   - { name: service_collector, tag: breadcrumb_builder, call: addBuilder }
    +   *   - { name: service_id_collector, tag: theme_negotiator }
        * @endcode
    

    It seems to be that we better split up the example into one for service_collector and one for service_id_collector

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -82,90 +94,141 @@ class TaggedHandlersPass implements CompilerPassInterface {
    +      else if ($param->getName() === 'priority') {
    +        $priority_pos = $pos;
    

    So we certainly support it

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2472337-24.patch, failed testing.

damiankloip queued 24: 2472337-24.patch for re-testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Performance

This was green once but yeah we need a CR.

Using this class also improves the performance of a site, so adding that particular tag.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2472337-24.patch, failed testing.

Crell’s picture

Question, since I couldn't figure it out from the patch: Are we *adding* an ID-based automatic collector in addition to the full-service one, or are we *replacing* the full-service one? The former is reasonable, as we have a couple of places that we use ID-based fronting services for performance. The latter is most definitely not.

dawehner’s picture

: Are we *adding* an ID-based automatic collector in addition to the full-service one, or are we *replacing* the full-service one?

We aren't replacing, just adding.

The last submitted patch, 24: 2472337-24.patch, failed testing.

damiankloip’s picture

Reroll, conflict in ThemeNegotiatorTest.

dawehner’s picture

So we certainly identified in Montpellier that those tagged services are problematic for things like /node/1 or REST requests,
but I totally agree, we need a change record.

damiankloip’s picture

Issue tags: -Needs change record

Here is a draft change notice : https://www.drupal.org/node/2598944

dawehner’s picture

Issue tags: +rc target triage

So one reason why you could argue that this issue can be committed is that it has some potential to improve performance and maybe even better for bigger sites, scalability in regards to how many modules are there.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Wanted to RTBC it

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling, +Needs issue summary update

The issue summary needs to include a statement of why we need to make this change during RC. It'd be great to have profiling to prove that this is worth doing.

dawehner’s picture

The issue summary needs to include a statement of why we need to make this change during RC. It'd be great to have profiling to prove that this is worth doing.

So to be clear, this is just an abstraction level for what we already did on multiple issues:

dawehner’s picture

So in other words we did profiling already in the past ...

damiankloip’s picture

Status: Needs work » Needs review
xjm’s picture

Issue tags: -rc target triage

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Version: 8.1.x-dev » 8.2.x-dev

Let's move this to 8.2.x

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

damiankloip’s picture

Semi-random failure.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Issue tags: +Needs reroll
jofitz’s picture

Re-rolled.

Wim Leers’s picture

Issue tags: -Needs reroll

Thanks!

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -41,42 +36,9 @@ class ThemeNegotiator implements ThemeNegotiatorInterface {
        */
    -  public function __construct(ThemeAccessCheck $theme_access) {
    +  public function __construct(ThemeAccessCheck $theme_access, array $negotiators) {
         $this->themeAccess = $theme_access;
    

    This needs some docs addition.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -41,42 +36,9 @@ class ThemeNegotiator implements ThemeNegotiatorInterface {
    -  public function addNegotiator(ThemeNegotiatorInterface $negotiator, $priority) {
    

    This method was never exposed via the interface, so I think removing it could be fine. The alternative is to make this method throw a trigger_error()

jofitz’s picture

Added the missing docs.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I really think this issue is good to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: 2472337-54.patch, failed testing.

almaudoh’s picture

Issue tags: +Needs reroll
dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
27.4 KB

Here is one.

Wim Leers’s picture

Code review
+++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
@@ -40,43 +35,12 @@ class ThemeNegotiator implements ThemeNegotiatorInterface {
+   * @param array $negotiators

Nit: string[].

Needs profiling
This was RTBC'd in #55 despite this tag still being there. Do we no longer need profiling?
Blocker
This is now blocking #2858482, see #2858482-26: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent. Which is in turn blocking #2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle(). Both of which are blocking #2737751: Refactor REST routing to address its brittleness.
damiankloip’s picture

Issue tags: -needs profiling

Here is that nit addressed. Hopefully we're good now. RE the profiling - I don't think we need to here, see #40

damiankloip’s picture

ha! and the patches!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

The issue summary needs to include a statement of why we need to make this change during RC.

Given that, we no longer needs this tag.

Wim Leers’s picture

Yay, glad to see this RTBC :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This results in doing quite a lot less during a standard install. https://blackfire.io/profiles/compare/e360eb1b-9a4b-4797-a5c2-65041a4bd0...

Nice.

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -77,91 +89,141 @@ class TaggedHandlersPass implements CompilerPassInterface {
       public function process(ContainerBuilder $container) {
         foreach ($container->findTaggedServiceIds('service_collector') as $consumer_id => $passes) {
           foreach ($passes as $pass) {
    -        $interface = NULL;
    -        $tag = isset($pass['tag']) ? $pass['tag'] : $consumer_id;
    -        $method_name = isset($pass['call']) ? $pass['call'] : 'addHandler';
    -        $required = isset($pass['required']) ? $pass['required'] : FALSE;
    +        $this->processServiceCollectorPass($pass, $consumer_id, $container);
    +      }
    +    }
    +
    +    foreach ($container->findTaggedServiceIds('service_id_collector') as $consumer_id => $passes) {
    +      foreach ($passes as $pass) {
    +        $this->processServiceIdCollectorPass($pass, $consumer_id, $container);
    +      }
    +    }
    +  }
    

    I wonder if we could do this better i.e should loop round the definitions once here...

        foreach ($container->getDefinitions() as $consumer_id => $definition) {
          foreach ($definition->getTag('service_collector') as $pass) {
            $this->processServiceCollectorPass($pass, $consumer_id, $container);
          }
          foreach ($definition->getTag('service_id_collector') as $pass) {
            $this->processServiceIdCollectorPass($pass, $consumer_id, $container);
          }
        }
    
  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -12,23 +14,16 @@
    +  use ContainerAwareTrait;
    
    @@ -40,43 +35,12 @@ class ThemeNegotiator implements ThemeNegotiatorInterface {
    +  public function __construct(ThemeAccessCheck $theme_access, array $negotiators) {
    

    It's a shame that this will result in more things being container aware. Might it not be an idea to have a ServiceCollection object or something that is container aware and that lazy loads rather than just injecting the IDs. Or actually can't we just inject the class resolver here and use that to get the services. And then the ThemeNegotiator can remain blissfully unaware of the container.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
28.05 KB

Patch addresses #64

dawehner’s picture

The one thing I really like about using the class resolver instead is that its interface: \Drupal\Core\DependencyInjection\ClassResolverInterface is basically a factory, and well this is all you want to have there.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
    @@ -87,14 +88,11 @@
    +    foreach ($container->getDefinitions() as $consumer_id => $definition) {
    

    This initially seems like a worse idea, as it iterates over all definintions in the container, which can be a lot. However, when findTaggedServiceIds() is called anyway, it loops over all services anyway.. :)

    I do think semantically we should call hasTag() first maybe? Symfony would do similar too I think.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
    @@ -31,16 +28,26 @@
    +  public function __construct(ThemeAccessCheck $theme_access, ClassResolverInterface $class_resolver, array $negotiators) {
    

    This is the only down side I can see to this approach. It's nice having something injected that contains an interface with the only method we need, but it is nice just being able to use ContainerAware and be done too.

alexpott’s picture

@damiankloip I think we need to avoid the spread of container awareness... once everything is container aware you basically doing \Drupal::service() everywhere.

Another idea is something that is just a container aware object with a list of service ids it can get. Something like

class ServiceProxyArray(
  use ContainerAware;

  public function  __construct(array $services) {
    $this->services = $services;
  }

  public function get($service_id) {
    return $this->container->get($service_id);
  }
)

Patch attached addresses #67.1 and hasTag()

dawehner’s picture

I agree ContainerAware is a bit of a cancer. When we introduce the class resolver though I would like to see whether the additional layer adds some significant overhead.

damiankloip’s picture

I get why container awareness is not ideal. I am saying from a DX point of view it's really simple and people understand it. Injecting or setting the container is also not the same as using the drupal static call IMO :) either way, to be clear, I am all for the idea of not injecting the container directly. I like the idea of the simple object that can only access a subset of services though. That seems slightly more fitting than using the class resolver?

dawehner’s picture

An alternative could be to provide a non container aware base class for all these services with a factory method a subclass can implement.

Wim Leers’s picture

Issue summary: View changes

Updating IS to clarify how much of a blocker this is.

Wim Leers’s picture

So do we want to move forward with #68? See the IS changes in #72, this is blocking a lot of issues.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think #68 is still much better than some other object in the middle. I also agree that using the class resolver is better than using the container.

Wim Leers’s picture

Issue tags: +API-First Initiative
damiankloip’s picture

I agree it would be good to just get this in now. I think it would need a separate issue to formulate some plan for an intermediate proxy object.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

I think the patch looks great, but the CR needs to be updated to reflect the change in direction from ContainerAware to class_resolver (per #64.2).

I think the CR should also more prominently list which core collectors are changed to ID collectors. So for now, just ThemeNegotiator, but maybe that list will grow before 8.4's release? Since making that change removes the ability to add theme negotiators at runtime.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record updates

Done.

dawehner’s picture

Thank you @Wim Leers!!

effulgentsia’s picture

Thanks! Ticking the credit box for @Wim Leers.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/TaggedHandlersPassTest.php
@@ -61,6 +61,26 @@ public function testProcessRequiredHandlers() {
+   * @expectedException \Symfony\Component\DependencyInjection\Exception\LogicException
+   * @expectedExceptionMessage At least one service tagged with 'consumer_id' is required.

I tried to commit this, but PHPCS returned:

FILE: ...sts/Core/DependencyInjection/Compiler/TaggedHandlersPassTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 66 | WARNING | @expectedException tags should not be used, use
    |         | $§this->setExpectedException() or
    |         | $this->expectException() instead
 67 | WARNING | @expectedExceptionMessage tags should not be used,
    |         | use $§this->setExpectedException() or
    |         | $this->expectException() instead
----------------------------------------------------------------------
Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.19 KB
28.25 KB

@effulgentsia pointed out that the latest patch still violates coding standards. Fixed that.

EDIT: cross-posted.

  • effulgentsia committed ea67660 on 8.4.x
    Issue #2472337 by damiankloip, alexpott, Jo Fitzgerald, dawehner, bojanz...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4.0 release notes

Thanks. Pushed to 8.4.x and published the CR.

Tagging for release notes mention, since this is a new tool that developers can leverage for performance optimizing.

Wim Leers’s picture

This unblocked #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent, which has 3 issues depending on it, so this unblocked an entire chain of 4 issues! Hurray :)

Status: Fixed » Closed (fixed)

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