`\mglaman\PHPStanDrupal\Type\DrupalClassResolverDynamicReturnTypeExtension` is misleading

[Niklan]

Bug report

Basically, this extension narrows the type exactly to the argument type. I think it should either be removed or allowed to be disabled.

Code snippet that reproduces the problem

This is the original implementation, from: \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition

  public function getInstanceFromDefinition($definition) {
    if ($this->container->has($definition)) {
      $instance = $this->container->get($definition);
    }
    else {
      if (!class_exists($definition)) {
        throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $definition));
      }

      if (is_subclass_of($definition, 'Drupal\Core\DependencyInjection\ContainerInjectionInterface')) {
        $instance = $definition::create($this->container);
      }
      else {
        $instance = new $definition();
      }
    }

    return $instance;
  }

Usage of that method leads to a false-positive error, for example:

$foo = $this->classResolver->getInstanceFromDefinition(Foo::class)
\assert($foo instanceof Foo);

→ Call to function assert() with true will always evaluate to true.

But there is no guarantee that it will always be an instance of the Foo class. Note that the original code first checks for a service in the container:

if ($this->container->has($definition)) {
  $instance = $this->container->get($definition);
}

So technically:

• It is possible to call ::getInstanceFromDefinition('foo_service') and get a valid instance. Drupal does not restrict it in any way, if it is a valid service. The class_exists() calls happen only if there is no service defined.

• Because of the previous points, it also means that this "service" can be altered and replaced with a completely different class: Drupal\example\Foo: '@Drupal\example\Bar', which are completely different classes that are not bound by an interface. This means that the resulting class will be a completely different one. Even if it sounds crazy, it is still possible.

• Also, it is an edge case scenario, but it is also possible to have some kind of factory as $definition. In that case, this part $definition::create($this->container); is also not guaranteed to produce the same result as $definition.

  class FooFactory {
  
    public static function create(ContainerInterface $container): FooInterface {
      // It could be anything here.
      return new Bar(); // Which could be instance of FooInterface.
    }
  
  }

Maybe there are other ways it could break, but this extension is not reliable, and assert() or any other $foo instanceof Foo::class is more reliable. This extension can be tricked, which is not a problem with the extension itself, but rather with the Drupal implementation, which allows for such things. The extension assumes it will be used under perfect conditions.

I think maybe it should be configurable? Without the assertion, the IDE screams that the code is incorrect, which is true:

I'de prefer assertion in that case.

[chesn0k]

Part of the issue be solved here https://www.drupal.org/project/drupal/issues/3540625 but in any case a service map is needed in case the service ID is passed as an argument.

I think this is too crazy code to be taken into account:

Because of the previous points, it also means that this "service" can be altered and replaced with a completely different class: Drupal\example\Foo: '@Drupal\example\Bar'

This is unfortunately not restricted at the typehint level, but nevertheless contradicts the description of the ContainerInjectionInterface::create() method.

Also, it is an edge case scenario, but it is also possible to have some kind of factory as $definition.

So I think declaring the return type on the getInstanceFromDefinition method is the best solution.