Alternative to Drupal::service() or $container->get() with return type promise

Created on 29 November 2024, 4 months ago

Problem/Motivation

Currently we still need to call `Drupal::service()` or `$container->get(..)` explicitly in some places.
Especially in tests, we won't be able to remove these calls.

Currently these have the problem that static analysis (IDE, phpstan) has no idea what the return type would be.
(unless they use some kind of magic plugin that reads the services.yml files)
Even if we use the interface name alias for the service id, static analysis cannot know for sure that the return value will be that.

We either need to put a `/** @var ... */` doc above the variable, or we don't get static anasysis.
Also, this does not work well with method chaining, e.g. `Drupal::service('foo_service')->foo()`, here static analysis has no way to know if the ->foo() call is valid.

There is also ClassResolverInterface, and Drupal::classResolver(), but these are useless for static analysis, because there is no guarantee about the return type.
Also, Drupal::classResolver() has a conditional return type, which makes it harder to document.

Changing these would be a BC break.

Steps to reproduce

Proposed resolution

Provide alternatives to Drupal::service() and $container->get().
The first parameter is a class or interface name.
The second, optional, parameter can specify a service id, if that differs from the class or interface name from the first parameter.

The return type is documented to be the same as the class name provided in the first parameter, using `@template T of object`, and `@param class-string $class` and `@return object&T` or just `@return T`.

E.g.

use Drupal\Component\Datetime\TimeInterface;

$time = Drupal::serviceByClass(TimeInterface::class);
assert($time instanceof TimeInterface);

// This would be redundant, but works.
$time = Drupal::serviceByClass(TimeInterface::class, 'datetime.time');
assert($time instanceof TimeInterface);

// The method will fail if the service instance does not have the expected type.
$this->expectException(UnexpectedServiceClass::class);
Drupal::serviceByClass(TimeInterface::class, 'file_system');

The same can be done as an object method, possibly as part of Drupal's ContainerInterface, or as a separate service.

Remaining tasks

We need to agree on suitable method names.
We need to agree whether the alternative to $container->get() should be part of Drupal's ContainerInterface, or it should live in a separate interface, and perhaps separate class.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.1 πŸ”₯

Component

base system

Created by

πŸ‡©πŸ‡ͺGermany donquixote

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @donquixote
  • Pipeline finished with Canceled
    4 months ago
    Total: 169s
    #354623
  • Pipeline finished with Failed
    4 months ago
    Total: 608s
    #354625
  • πŸ‡·πŸ‡Ί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.

  • πŸ‡©πŸ‡ͺ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.

  • Pipeline finished with Failed
    4 months ago
    Total: 844s
    #354639
  • Pipeline finished with Success
    4 months ago
    Total: 1869s
    #354640
  • πŸ‡©πŸ‡ͺ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.

  • Pipeline finished with Failed
    4 months ago
    Total: 3511s
    #354660
  • Pipeline finished with Failed
    4 months ago
    Total: 723s
    #354699
  • Pipeline finished with Failed
    4 months ago
    Total: 777s
    #354700
  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • Pipeline finished with Failed
    4 months ago
    Total: 600s
    #358064
  • Pipeline finished with Failed
    4 months ago
    Total: 613s
    #358065
  • Pipeline finished with Failed
    4 months ago
    Total: 261s
    #364945
  • Pipeline finished with Failed
    4 months ago
    Total: 146s
    #364946
  • Pipeline finished with Failed
    4 months ago
    Total: 597s
    #364949
  • Pipeline finished with Failed
    4 months ago
    Total: 1035s
    #365677
  • πŸ‡©πŸ‡ͺ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
  • 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.

Production build 0.71.5 2024