- Issue created by @donquixote
- Merge request !10399Issue #3490713: Introduce Drupal::serviceByClass(). β (Open) created by donquixote
- π·πΊRussia Chi
unless they use some kind of magic plugin that reads the services.yml files
For PhpStorm you can generate meta information about services.
drush gen phpstorm-meta
Psalm also reads it. Not sure about PhpStan. I suppose phpstan-drupal can help on this.
- Merge request !10401Draft: Issue #3490713: POC: Convert some Drupal::service() calls. β (Open) created by donquixote
- π©πͺGermany donquixote
For PhpStorm you can generate meta information about services.
Thanks!
In the past I tried the symfony plugin for phpstorm which was also supposed to do this.
But it slowed everything down.I think what I am proposing here is nicer, and more universally available.
- π©πͺGermany donquixote
I would like to add tests, but not sure where they would go.
- π©πͺGermany donquixote
For $container->getByClass() this is going to be more difficult, if we want to maintain full BC.
I was going to introduce a new interface ServiceByClassInterface and ConvenientContainerInterface, which would be implemented by our container classes.
But then in every place we need to check if the container we have is actually implementing that interface.
If instead we add the method directly to our ContainerInterface, any custom implementations will break which don't have this method.----
Alternatively we could introduce a function like this:
$time = service(TimeInterface::class, $container); assert($time instanceof TimeInterface);
so the same as above but we pass the $container object, so it is cleaner.
or
$time = ServiceFromClass::create($container)->getByClass(TimeInterface::class); assert($time instanceof TimeInterface);
or we have something that returns a closure.
$time = resolver($container)(TimeInterface::class); assert($time instanceof TimeInterface); $resolve = resolver($container); assert($resolve(TimeInterface::class) instanceof TimeInterface);
But currently a lot of the $container->get() calls should rather be replaced by some kind of autowire solution.
- π©πͺGermany donquixote
Seems we are suffering this one, π [random test failure] BlockCacheTest Active
And also another random test failure which happened earlier. - π©πͺGermany donquixote
Changing issue title to be more findable, hopefully
- π¬π§United Kingdom james.williams
If the service registry allows swapping/overriding/decorating the class for a service, how does that interact with this? If you pass a decorated class to serviceByClass(), should it return the decorator, or the decorated class? If you pass the class of a service that has been swapped for a different class, which do you get? I'm not convinced this is a good idea, I'm concerned it could bring confusing DX for the sake of helping tooling rather than anything functional.
- πΊπΈUnited States bradjones1 Digital Nomad Life
Re: #13 - I can at least answer the question of, what service do you get? You get the decorated service. This is as it works now, at least if the service's name is the same as its original class.
The bigger issue is that what you really want is a response containing a predictable interface. That's harder to typehint. If we could solve for that edge case (or it can be explained a bit better what you get in various circumstances, e.g. a truth table) and it works with IDE completion, this would be amazing.
- π¬π§United Kingdom james.williams
OK, that makes sense - but then makes the method name rather confusing. Getting a 'service by class' which isn't even the class you asked for, is straight-up confusing. So maybe a name of Drupal::serviceByInterface() would be better? Or, since many services are already aliased to an interface name, could something like phpstan-drupal be made to understand that \Drupal::service(MyInterface::class) returns an instance of MyInterface to avoid the need for this at all?
Making changes for the sake of the tooling still feels a bit like the 'tail wagging the dog'.
- πΊπΈUnited States bradjones1 Digital Nomad Life
In my practice, I definitely find myself wanting something like this but also my IDE knows what to expect once I put in an
assert()
on the next line. This has the benefit, also, of validating that I am getting the service I expect during tests and local development. So personally this is in the lower-priority bucket as there are other Drupalisms in the service container I'd like to tackle first... BUT if this did all the things out of the box as we'd expect, then that's great. But I agree if the edge cases are just as tricky as the status quo, there's no benefit. - πΊπΈUnited States bradjones1 Digital Nomad Life
To clarify what I mean in the prior comment - I use assert() in particular to check the order of services when I have multiple layers of decoration. So niche case but it was unclear the way I phrased it.
- π©πͺGermany donquixote
The bigger issue is that what you really want is a response containing a predictable interface.
So maybe a name of Drupal::serviceByInterface() would be better?
Indeed Drupal::serviceByInterface() would be a more accurate.
Basically you use the type that you would also use as a parameter type in a constructor that expects that service.
So it is the equivalent to autowire.The reason I named it serviceByClass() in the MR is that:
- Some services don't have an interface. Especially in custom code or in early versions of a module this might be the case. If constructor parameter types use the class name, then any decorators also need to extend that.
- The term 'class' is used as a catch-all term in many places, where traits and interfaces are also included. E.g. "ReflectionClass" (php native), "class-string" (phpstan), "ClassLoader" (composer), etc.
In my practice, I definitely find myself wanting something like this but also my IDE knows wha to expect once I put in an assert() on the next line.
I tend to do either assert(* instanceof *) or an inline /** @var */ doc.
The problem with both of these is that they are more verbose, and they require a local variable, where otherwise I could use method chaining like Drupal::service(Abc::class)->foo();.This has the benefit, also, of validating that I am getting the service I expect during tests and local development.
I use assert() in particular to check the order of services when I have multiple layers of decoration
Ok there are two different goals / cases here:
- In a test, I may want to assert the exact class of the service, to make sure my decorators work correctly. In that case I would use $this->assertSame(MyClass::class, get_class($service)); rather than native assert(* instanceof *) or PhpUnit ->assertInstanceOf().
- In my regular code, I would only ever assert($service instanceof SomethingInterface), because other modules might want to add further decorators. This verifies that the DI is not completely giving me the wrong service, but it does not verify that specific decorators are applied.
- Status changed to Needs work
about 1 month ago 10:38am 27 February 2025 The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.