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.
Comment | File | Size | Author |
---|---|---|---|
#82 | 2472337-81.patch | 28.25 KB | Wim Leers |
#68 | 2472337-68.patch | 28.31 KB | alexpott |
#68 | 65-68-interdiff.txt | 1.52 KB | alexpott |
#65 | 2472337-65.patch | 28.05 KB | alexpott |
#65 | 60-65-interdiff.txt | 5.72 KB | alexpott |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx for Acquia commentedGiven we have the ServiceCollectorHandlersPass generic and an example in the param converters issue, I think this can be handled by a novice.
Comment #2
fgmThis is actually the TaggedHandlersPass, not ServiceCollectorHandlersPass. The ParamConverters issue is #2470926: Don't load all the paramconverters on every request..
Comment #3
kfitz CreditAttribution: kfitz at Acro Commerce commentedComment #4
dawehnerI still think that this would be super useful.
Comment #5
Fabianx CreditAttribution: Fabianx as a volunteer commentedI agree 100% that this would be super-useful.
Proxies are nice, but they don't necessarily always make sense just to make something lazy ...
Comment #6
damiankloip CreditAttribution: damiankloip commentedSo 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.
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer commented#6 looks great.
Comment #8
dawehnerI'd suggest to convert at least one of the usecases we already have in core so we know that the change fits our needs.
Comment #9
damiankloip CreditAttribution: damiankloip commentedAgree totally. That's just a rough patch. Needs a conversion and test coverage for sure!
Comment #10
damiankloip CreditAttribution: damiankloip commentedWorking on this now.
Comment #11
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedJust need to fix the ThemeNegotiatorTest, figures.
Comment #14
dawehnerIt is quite nice that we can remove parts of ThemeNegotiatorTest now.
We could use parent: container.trait
Let's just use inline vars now, this is IMHO much more readable
Comment #15
damiankloip CreditAttribution: damiankloip commentedHow's about that?
EDIT: Forgot the change that exception message! Let's see how the bot gets on now anyway.
Comment #16
dawehnerNice!
Issue summary looks perfect!
Comment #17
damiankloip CreditAttribution: damiankloip commentedComment #18
damiankloip CreditAttribution: damiankloip commentedOh, we need to add to the docs for this I guess... :)
Comment #19
dawehnerI guess we also need a CR
Comment #21
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1 - once we have docs.
Nice work!
Comment #22
bojanz CreditAttribution: bojanz at Centarro commentedMy silly docs fix landed (#2504013: Remove @see statements from the ThemeNegotiator docblock) and now this patch doesn't apply.
Here's a reroll.
Comment #23
damiankloip CreditAttribution: damiankloip commentedComment #24
damiankloip CreditAttribution: damiankloip commentedDocs, eventually. Needs some review, and very likely some tweaking.
Comment #25
dawehnerAh there we talk about priorities.
It seems to be that we better split up the example into one for service_collector and one for service_id_collector
So we certainly support it
Comment #28
dawehnerThis was green once but yeah we need a CR.
Using this class also improves the performance of a site, so adding that particular tag.
Comment #29
dawehnerBack to RTBC
Comment #31
Crell CreditAttribution: Crell as a volunteer commentedQuestion, 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.
Comment #32
dawehnerWe aren't replacing, just adding.
Comment #34
damiankloip CreditAttribution: damiankloip commentedReroll, conflict in ThemeNegotiatorTest.
Comment #35
dawehnerSo 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.
Comment #36
damiankloip CreditAttribution: damiankloip commentedHere is a draft change notice : https://www.drupal.org/node/2598944
Comment #37
dawehnerSo 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.
Comment #38
dawehnerWanted to RTBC it
Comment #39
alexpottThe 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.
Comment #40
dawehnerSo to be clear, this is just an abstraction level for what we already did on multiple issues:
Comment #41
dawehnerSo in other words we did profiling already in the past ...
Comment #42
damiankloip CreditAttribution: damiankloip commentedComment #43
xjmComment #45
dawehnerLet's move this to 8.2.x
Comment #47
dawehnerThis blocks a nice improvement in the jsonapi module: #2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API
Comment #48
damiankloip CreditAttribution: damiankloip commentedSemi-random failure.
Comment #50
Wim LeersComment #51
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #52
Wim LeersThanks!
Comment #53
dawehnerThis needs some docs addition.
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()
Comment #54
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded the missing docs.
Comment #55
dawehnerI really think this issue is good to go.
Comment #57
almaudoh CreditAttribution: almaudoh commentedComment #58
dawehnerHere is one.
Comment #59
Wim LeersNit:
string[]
.Comment #60
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is that nit addressed. Hopefully we're good now. RE the profiling - I don't think we need to here, see #40
Comment #61
damiankloip CreditAttribution: damiankloip at Acquia commentedha! and the patches!
Comment #62
dawehnerGiven that, we no longer needs this tag.
Comment #63
Wim LeersYay, glad to see this RTBC :)
Comment #64
alexpottThis results in doing quite a lot less during a standard install. https://blackfire.io/profiles/compare/e360eb1b-9a4b-4797-a5c2-65041a4bd0...
Nice.
I wonder if we could do this better i.e should loop round the definitions once here...
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.
Comment #65
alexpottPatch addresses #64
Comment #66
dawehnerThe 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.Comment #67
damiankloip CreditAttribution: damiankloip at Acquia commentedThis 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.
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.
Comment #68
alexpott@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
Patch attached addresses #67.1 and hasTag()
Comment #69
dawehnerI 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.
Comment #70
damiankloip CreditAttribution: damiankloip at Acquia commentedI 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?
Comment #71
dawehnerAn alternative could be to provide a non container aware base class for all these services with a factory method a subclass can implement.
Comment #72
Wim LeersUpdating IS to clarify how much of a blocker this is.
Comment #73
Wim LeersSo do we want to move forward with #68? See the IS changes in #72, this is blocking a lot of issues.
Comment #74
dawehnerI 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.
Comment #75
Wim LeersComment #76
damiankloip CreditAttribution: damiankloip at Acquia commentedI 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.
Comment #77
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think the patch looks great, but the CR needs to be updated to reflect the change in direction from
ContainerAware
toclass_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.Comment #78
Wim LeersDone.
Comment #79
dawehnerThank you @Wim Leers!!
Comment #80
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks! Ticking the credit box for @Wim Leers.
Comment #81
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI tried to commit this, but PHPCS returned:
Comment #82
Wim Leers@effulgentsia pointed out that the latest patch still violates coding standards. Fixed that.
EDIT: cross-posted.
Comment #84
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. 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.
Comment #85
Wim LeersThis 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 :)