Contrib.social

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

Open on Drupal.org →
Created on 29 November 2024, 22 days 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
  • Comment 22 days ago →
    🇩🇪Germany donquixote
  • Merge request !10399Issue #3490713: Introduce Drupal::serviceByClass(). → (Open) created by donquixote
  • Pipeline finished with Canceled
    22 days ago
    Total: 169s
    #354623
  • Pipeline finished with Failed
    22 days ago
    Total: 608s
    #354625
  • Comment 22 days ago →
    🇷🇺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
  • Comment 22 days ago →
    🇩🇪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
    22 days ago
    Total: 844s
    #354639
  • Pipeline finished with Success
    22 days ago
    Total: 1869s
    #354640
  • Comment 22 days ago →
    🇩🇪Germany donquixote

    I would like to add tests, but not sure where they would go.

  • Comment 22 days ago →
    🇩🇪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
    22 days ago
    Total: 3511s
    #354660
  • Pipeline finished with Failed
    22 days ago
    Total: 723s
    #354699
  • Pipeline finished with Failed
    22 days ago
    Total: 777s
    #354700
  • Comment 21 days ago →
    🇳🇿New Zealand quietone
  • Pipeline finished with Failed
    18 days ago
    Total: 600s
    #358064
  • Pipeline finished with Failed
    18 days ago
    Total: 613s
    #358065
  • Pipeline finished with Failed
    11 days ago
    Total: 261s
    #364945
  • Pipeline finished with Failed
    11 days ago
    Total: 146s
    #364946
  • Pipeline finished with Failed
    11 days ago
    Total: 597s
    #364949
  • Pipeline finished with Failed
    11 days ago
    Total: 1035s
    #365677
  • Comment 10 days ago →
    🇩🇪Germany donquixote

    Seems we are suffering this one, 📌 [random test failure] BlockCacheTest Active
    And also another random test failure which happened earlier.

  • Comment 10 days ago →
    🇩🇪Germany donquixote
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024