Return the correct typehint when Drupal::service() is called with a class string

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.

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.

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.

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'.

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.

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.

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.

Thanks to the autowiring work, given we can already do

> \Drupal::service(\Drupal\Core\Entity\EntityTypeManagerInterface::class);
= Drupal\Core\Entity\EntityTypeManager {#4185}

is there something clever we can do with the docblock of \Drupal::service() so PHPStan can infer the type if you pass an interface name to the existing method, without having to add some new API? Or can we add a custom PHPStan extension that helps it map service names to classes?

I was about to comment the same as #20. I think it's worth exploring.

In https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types there is this example:

/**
 * @template T of object
 * @param class-string<T> $class
 * @return ($throw is true ? T : T|null)
 */
function getService(string $class, bool $throw = true): ?object

I wonder if we can do this:

/**
 * @template T of object
 * @param class-string<T>|string $id
 * @return ($id is class-string ? T : object)
 */
public static function service($service)

(edit: posted at the same time as #21)

Could we merge this docblock with the existing \Drupal::service() docblock somehow?

*  @template T of object
*   
*  @param class-string<T> $class
*
*   @return object&T
*       A service that is an instance of $class.

Added a new MR that just adds the class-string typehints to \Drupal::service().

Added the conditional return type as per #21

Testing in PHPStorm this seems to work well.

Agreed this works well in phpstorm. I also ran phpstan on max level before and after and there was no difference in the total number of violations, not sure what to make of that.

I notice this now closely matches \Symfony\Component\DependencyInjection\ContainerInterface::get, except that includes conditions for the $invalidBehavior param that we don't need to worry about here.

    /**
     * @template C of object
     * @template B of self::*_REFERENCE
     *
     * @param string|class-string<C> $id
     * @param B                      $invalidBehavior
     *
     * @return ($id is class-string<C> ? (B is 0|1 ? C|object : C|object|null) : (B is 0|1 ? object : object|null))
     *
     * @throws ServiceCircularReferenceException When a circular reference is detected
     * @throws ServiceNotFoundException          When the service is not defined
     *
     * @see Reference
     */
    public function get(string $id, int $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE): ?object;

If we're happy with this change of directions then this needs an issue summary update, otherwise RTBC for me.

Updated the title and IS to reflect the new approach.

I can also confirm this works for me in PHPStorm.

I think the IS might need updating under "Proposed resolution," but otherwise this looks good to me. Concern about BC was addressed.

Updated IS

> I also ran phpstan on max level before and after and there was no difference in the total number of violations, not sure what to make of that.

I think a lot of work went into the phpstan integration to actually make it understand those services, whether that's a class/interface or any other string. So I'm fairly certain that the claim in the issue summary ("However there is no way for PHPStan to know that.") is actually wrong.

The same is not true for phpstorm and possibly other tools though. It doesn't work as well there, even with the symfony plugin and so on.

FWIW, this did a considerable shift in direction in the last day, I do want to point out that @donquixote explicitly argued that it should be separate in earlier discussions here. I don't agree with that option, and I think we shouldn't add more API endpoints for non-injected code when we are now at a point with autowire+autodiscovery+relaxed doc standards that it's actually becoming easier to do DI than not (or maybe restricted to tests where we don't want to use DI). I'm just saying that we should consider that opinion/direction change.

One reason that PHPStan doesn't see any difference is that we aren't calling \Drupal::service() with a class string very often - for historical reasons most tests will use the custom service name, and this change doesn't help with that.

That's a good point in #35. There is actually a slight drop in numbers after a recent commit adding <T> to the conditional return.

Was needs work intentional? Maybe back to needs review for #34?

Didn't mean to change this to NW. Also tested in PHPStorm and this works well.

Given nobody should be subclassing \Drupal then I think this is safe to backport to 10.6.x. We could also backport the docs only part to 11.2.x/10.5.x but at this stage I don't think it's worth the effort.

Committed and pushed 76a93eece07 to 11.x and a55b4e7d6f5 to 11.3.x and 451faa86060 to 10.6.x. Thanks!

If you use PHPStorm + Symfony plugin and services autocompletion lags, its because of this php doc. Just delete the doc locally anyway, it doesn't do anything if you use Symfony plugin.