Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle DrupalKernel synthetic services #818

Open
Niklan opened this issue Jan 21, 2025 · 3 comments · May be fixed by #823
Open

Handle DrupalKernel synthetic services #818

Niklan opened this issue Jan 21, 2025 · 3 comments · May be fixed by #823
Labels
enhancement New feature or request

Comments

@Niklan
Copy link
Contributor

Niklan commented Jan 21, 2025

DrupalKernel registers 3 synthetic services and instantly hardcodes aliases for two of them.

https://github.com/drupal/drupal/blob/c6bc6ebf27480b08b4c9ed5a7099ff5eef7e4348/core/lib/Drupal/Core/DrupalKernel.php#L1338-L1345

mglaman\PHPStanDrupal\Drupal\DrupalAutoloader::register() only provides information about services defined in the *.services.yml file, so those two services aren't detected.

This simple example triggers a problem:

<?php

use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

class Foo implements ContainerInjectionInterface {

  public function __construct(
    private ContainerInterface $container,
  ) {}

  public static function create(ContainerInterface $container): self {
    return new self(
      $container->get(ContainerInterface::class),
    );
  }

}

produces on PHPStan level 7:

  14     Parameter #1 $container of class Foo constructor expects Symfony\Component\DependencyInjection\ContainerInterface, object given.  
         🪪  argument.type 

If create is replaced by:

    return new self(
      $container,
    );

there is no error.

This is a kind of edge case scenario, but it raises a question: should this package handle it in some way? Considering that the services are not detected, this means that anything that is registered or changed by a service provider or compiler pass will not be detected. Perhaps it would be possible to improve things in general, rather than just this specific case?

@Niklan Niklan added the enhancement New feature or request label Jan 21, 2025
@mglaman
Copy link
Owner

mglaman commented Jan 23, 2025

In #721 I tried to improve the service map. That would help handle the service providers as well.

@Niklan
Copy link
Contributor Author

Niklan commented Jan 24, 2025

Hah! That one (#693), I also reported it, well, things have gotten even worse (or better) since then because now we can decorate services without having to explicitly do it in the *.services.yml file.

Drupal\foo\Bar: { }
#[AsDecorator(decorates: 'foo')]
class Bar {

But this approach doesn't have any problems from #693. At least, I didn't encounter it again with attributes and new ones didn't appear.

It sounds like this package should basically emulate all of that stuff in order to get proper information about the service container, as it can't directly retrieve that information from Drupal. This would require a full bootstrap and a configured environment, so the container would contain all the definitions of the desired modules.

Maybe it is better to add a kind of meta/epic issue for improving the service container's handling in general, and add subtasks for such situations. Because it seems like too much work for one issue, it will be much easier to solve it based on the specific needs of the container or its logic.

Also, as I understand the changes from #721, it seems that it will not fix this particular issue, because it has been hardcoded into DrupalKernel, and it is not called, so it is impossible to get that data from there.

But I did found this:

// Attach synthetic services
// @see \Drupal\Core\DrupalKernel::attachSynthetic
$this->serviceMap['kernel'] = ['class' => DrupalKernelInterface::class];
$this->serviceMap['class_loader'] = ['class' => ClassLoader::class];

It looks like the solution to this problem is simple. We just need to add a new static map for ContainerInterface.

        // Attach synthetic services
        // @see \Drupal\Core\DrupalKernel::attachSynthetic
        $this->serviceMap['kernel'] = ['class' => DrupalKernelInterface::class];
+       $this->serviceMap[DrupalKernelInterface::class] = ['alias' => 'kernel'];
        $this->serviceMap['class_loader'] = ['class' => ClassLoader::class];
+       $this->serviceMap['service_container'] = ['class' => \Drupal\Component\DependencyInjection\Container::class];
+       $this->serviceMap[\Symfony\Component\DependencyInjection\ContainerInterface::class] = ['alias' => 'service_container'];

Tested locally, and it worked as expected. The problem disappeared.

^ array:6 [
  "service_provider.core.service_provider" => array:1 [
    "class" => "\Drupal\Core\CoreServiceProvider"
  ]
  "kernel" => array:1 [
    "class" => "Drupal\Core\DrupalKernelInterface"
  ]
  "Drupal\Core\DrupalKernelInterface" => array:1 [
    "alias" => "kernel"
  ]
  "class_loader" => array:1 [
    "class" => "Composer\Autoload\ClassLoader"
  ]
  "service_container" => array:1 [
    "class" => "Drupal\Component\DependencyInjection\Container"
  ]
  "Symfony\Component\DependencyInjection\ContainerInterface" => array:1 [
    "alias" => "service_container"
  ]
]
^ mglaman\PHPStanDrupal\Drupal\DrupalServiceDefinition^ {#662
  -id: "Symfony\Component\DependencyInjection\ContainerInterface"
  -class: "Drupal\Component\DependencyInjection\Container"
  -public: true
  -deprecated: false
  -deprecationTemplate: null
  -alias: null
  -decorators: []
}

Basically, this would align DrupalKernel hardcoded service definitions with their aliases and the hardcoded solutions from this package. Added as #823

@Niklan
Copy link
Contributor Author

Niklan commented Jan 24, 2025

Btw, one thing came to my mind: since we can't bootstrap Drupal fully with a proper environment (and it would be very slow) during PHPStan analysis, especially in a CI environment, perhaps we could add some kind of optional PHPStorm adapter for metadata that can be generated using Chi-teck/drupal-code-generator, which is included in Drush and can be accessed via the drush generate phpstorm-meta command. This is static information in PHP format that can easily be parsed and used (I guess 😄).

Image

It can be stored in the project's repo, which also solves the problem of CI. Not to mention that it provides a lot of useful information, including field information and more which can be used for other cases.

This can be just an optional configuration for this project. If it is provided through phpstan.neon, we can use it, if not, well, parse it like right now.

I think it should be much easier to simply build an adapter and use the information from it rather than try to fully emulate the service container's logic. Additionally, compiler passes can be difficult to emulate in that package, if it is even possible (they may be built based on external data)

Or simply use the same approach: providing a command to run in a fully bootstraped environment, which will generate static information with all the necessary data for PHPStan extension, like the phpstan baseline with static ignores, but for hard-to retrieve information (phpstan-drupal-baseline.php or something like that). Since there is already a generator available for every project using Drush, and a custom generator would simply duplicate that functionality, we can simply use it.

@Niklan Niklan linked a pull request Jan 24, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants