Document why we include controllers in services.yml OR make use of AutowireTrait instead

1. Why not ControllerBase but invokable services

We specifically refactored away from ControllerBase, to adopt the pattern that is possible since https://www.drupal.org/node/2997122 → .

We intentionally converted to that pattern since 📌 Lift all methods out of `SdcController` into separate invokable services-as-controllers Needs work .

The reason: a single massive controller class (that also happened to extend ControllerBase) was resulting in a huge amount of injected services. Besides annoying, this also made writing kernel tests much harder.

@longwave and I both preferred this pattern. The original SdcController class contained all the controllers and was unwieldy. Using invokable services prevents us from ever regressing towards that 😊

2. Why list invokable-services-as-controllers in *.services.yml?

Because the CR at https://www.drupal.org/node/2997122 → implies this is necessary, whereas it is apparently NOT necessary if you're using autowiring for those services, which they all already do!

But instead of relying on them being listed in the *.services.yml file, we apparently can just use AutowireTrait? Cool!

But

AFAICT Drupal\Core\DependencyInjection\AutowireTrait; only works if there's a ::create() method though, which does not exist here.

So the mere presence of

  # Controllers.
  Drupal\experience_builder\Controller\ApiComponentsController: {}
  Drupal\experience_builder\Controller\ApiConfigControllers: ~
  Drupal\experience_builder\Controller\ApiLayoutController: {}
  Drupal\experience_builder\Controller\ApiLogController: {}
  Drupal\experience_builder\Controller\ApiPreviewController: {}
  Drupal\experience_builder\Controller\ApiContentUpdateController: {}
  Drupal\experience_builder\Controller\ComponentStatusController: {}
  Drupal\experience_builder\Controller\ExperienceBuilderController: {}

in the *.services.yml file is what triggers auto-wiring.

If you want to be removed, I think we could do something like https://symfony.com/doc/current/service_container.html#importing-many-se... to make all classes in /src/Controller available as services? 🤔 😊

Controllers only need to implement ContainerInjectionInterface for their create() method to be called, but I tried

final class ApiComponentsController implements ContainerInjectionInterface {

  use AutowireTrait;

but this fails with

Cannot autowire service "%renderer.config%": argument "$rendererConfig" of method "Drupal\\experience_builder\\Controller\\ApiComponentsController::_construct()", you should configure its value explicitly.

because AutowireTrait does not understand

    #[Autowire(param: 'renderer.config')]

which I guess needs fixing in core first.

Opened 📌 Allow AutowireTrait to wire parameters Active

I don't have an opinion either way, other than the performance impact of controller definitions being in the container so taking up some memory at runtime rather than when needed
But now we have all hooks in the container too (from 11.1) it's probably not
Either way it would be good to document the decision somewhere

wim leers → credited undefined → .

https://events.drupal.org/sites/default/files/slides/drupalcon.2019.auto... → slides 27–30 show how #2 could work, but Drupal doesn't yet support. He does show how the equivalent DX can be achieved with a dozen LoC in a service provider. (Also implemented in https://www.drupal.org/project/autoservices → .) So did that.

See https://git.drupalcode.org/project/experience_builder/-/merge_requests/4....

Related: 📌 Enable service autoconfiguration for core event subscribers Fixed — I think XB could/should do that too, WDYT, @larowlan, as a person who helped make that happen in core? 😄

wim leers → credited weaverryan → .

LOL @ #8 — I tried crediting https://www.drupal.org/u/weaverryan → — perhaps that happened because he's not got a d.o GitLab profile? 🤔

Trying once more.

I am not sure this is a huge improvement.

Personally I would like to complete 📌 Use PHP attributes for route discovery Needs review and then we can drop these from both services.yml and routing.yml.

I agree all of this is minor, but it was important enough for @larowlan to create an issue for it, so I wanted to convey he's being heard 😊

🤩 Thanks for making me aware of #3311365!

#3311365 sounds nice, but I think it cannot land in Drupal 10, which per @lauriii XB must support for now.

The solution here is neat, but I'm not sure it resolves the original motivation here, which was to document why we have the controllers listed in services.yml. I actually think the current setup is more discoverable than the service provider, I was just looking to see it documented as a pattern.

However, as mentioned in #5 I don't feel strongly either way, so if this is something you'd like to introduce, go for it, maybe just put a README.md in the Controller folder pointing to the service provider for people searching for the wiring?

I prefer self-enforcing "docs", where the code provides a helpful error message when some code is added/modified "incorrectly".

Although this MR isn't doing that perfectly yet, so I think you're totally right for wanting a README! Done: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

I think both of you (@larowlan and @longwave) are working towards a future where neither *.services.yml (see: all the autowiring core issues y'all worked on!) nor *.routing.yml (see 📌 Use PHP attributes for route discovery Needs review ) exist, right?

So, went ahead and besides the README I also made it easy to remove this as soon as #3311365 is available to us: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... 😊

I don't see how this is the same as the proposal in 📌 Make controllers invokable for DX and maintainability Active which is about a single method name for routes to save having to specify one every time (and potentially to enforce one class per route). I would assume from that issue that controllers continue to be instantiated the same way they are now.

Adding every single controller to the dependency injection container will make it unreasonably massive if such a pattern was adopted by core. Do we need to load the existence of system.linkset_settings on every page? Might was well use the Symfony massive array router if we do that instead of the router table, but we're talking about potentially multiple thousands of items here.

If the goal is to remove the method from the router definition, this could probably be achieved with a route provider. If the goal is to remove ::create() then that's a massive diversion from core and will make XB harder to commit when it comes time for that.

For example a medium-sized site I am working on has 1595 routes in the database. It has 180 modules installed. I have seen sites with over 300 modules installed and over 3000 routes before.

@catch Pleasantly surprised to see you chime in! 😄 And … interesting. 🤔

I read in #3317792 and concluded it was the same as "invokable services as controllers". Thanks for pointing out that is too simplistic and hence wrong 👍

That suggests https://www.drupal.org/node/2997122 → is not actually is an anti-pattern?

I got here from 📌 Make controllers invokable for DX and maintainability Active and I wondered 'how is that possible if core doesn't support it yet', then read through this issue and realised it was different.

This is the first time I've ever seen [#2997122 ] before it was committed, I would have pushed back a bit - however it is one of those things where if one module does it for one controller, it's fine, the problem is when 300 modules do it for 2000 controllers. No-one ever opened a follow-up to apply that pattern to the rest of core so I never realised we supported it.

My overall concern in general is the number of 'massive arrays that have to be loaded from the database into memory on every request before Drupal can do anything', of which the container is the one that is needed for literally everything even on page cache hits.

OOP hook discovery for example just increased the amount of memory required to install Drupal on the cli from about 40mb to around 200mb, a lot of this is the bc layer, but putting even more things into the container is just frightening at this point, and ::create() methods aren't the end of the world. We could even look at discovering the services during route discovery and storing them with the route object maybe to get similar DX - but that would only require loading what we need for the routes actually used in routing, not all the routes.

I think the best solution here is do 📌 Allow AutowireTrait to wire parameters Active then implement ContainerInjectionInterface and AutowireTrait in each of the controllers (or just extend ControllerBase) and then we get the same result without adding to the container; the autowiring is done at runtime instead of build time but in practice I imagine the performance impact is minimal.

Yeah runtime would be fine at least for controllers - only need to do it once.

one class per controller to keep dependency injection under control makes perfect sense though, it's just the extra step of registering on the container I'm worried about.

❤️ Thanks for stating that, that is+was my sole motivation 😄

Sounds like the best course of action here is to postpone this on 📌 Allow AutowireTrait to wire parameters Active .

Postponing on an upstream core issue because refactoring multiple times is wasted effort. It's fine to keep it as-is until core improves.

Alternatively, we could update this MR to point to #3490300, rephrase the README to reflect @catch's concerns, and then do the refactor once #3490300 is in.