mortenson / psalm-plugin-drupal

A project to add Psalm support for Drupal for security testing, focused only on taint analysis.
Other
43 stars 7 forks source link

InvalidArgumentException: Could not get class storage for drupal\core\entity\entitytypebundleinfo #4

Open driskell opened 3 years ago

driskell commented 3 years ago

Just trying this out with the default config (with the generate bits added for a few modules). And I seem to get stuck at this when running threads=1. With more threads it hits other things (same error classification but different class name).

I'm unsure what causes it as I'm honestly fairly new to psalm (been using phpstan a while). Any ideas what might cause it? Guessing something missing from the container? Tracking it with debug-by-line and threads=1 shows below:

Analyzing /www/modules/custom/CUSTOM_MODULE/src/Form/CUSTOMEntityDeleteForm.php
/www/modules/custom/CUSTOM_MODULE/src/Form/CUSTOMEntityDeleteForm.php:12
/www/core/lib/Drupal/Core/Entity/ContentEntityForm.php:57
/www/core/lib/Drupal/Core/Entity/ContentEntityForm.php:58
/www/core/lib/Drupal/Core/Entity/ContentEntityForm.php:59
/www/core/lib/Drupal/Core/Entity/EntityForm.php:81
/www/core/lib/Drupal/Core/Entity/EntityForm.php:82
/www/core/lib/Drupal/Core/Entity/EntityForm.php:83
/www/core/lib/Drupal/Core/Entity/EntityForm.php:88
/www/core/lib/Drupal/Core/Entity/ContentEntityForm.php:61
/www/core/lib/Drupal/Core/Entity/ContentEntityForm.php:62

The ContentEntityForm:62 is the $this->entityTypeBundleInfo below

  public function __construct(EntityRepositoryInterface $entity_repository, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, TimeInterface $time = NULL) {
    if ($entity_repository instanceof EntityManagerInterface) {
      @trigger_error('Passing the entity.manager service to ContentEntityForm::__construct() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Pass the entity.repository service instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
      $this->entityManager = $entity_repository;
    }
    $this->entityRepository = $entity_repository;
    $this->entityTypeBundleInfo = $entity_type_bundle_info ?: \Drupal::service('entity_type.bundle.info');
    $this->time = $time ?: \Drupal::service('datetime.time');
  }
InvalidArgumentException: Could not get class storage for drupal\core\entity\entitytypebundleinfo
mortenson commented 3 years ago

@driskell Your guess about the container is probably right. Could you try running psalm with the --no-cache option to see if it's caching related?

driskell commented 3 years ago

@mortenson I tried clearing cache and also tried —no-diff so I don’t think was cache but I can try a —no-cache if I can next time.

If it helps it’s on Drupal 8.9.14 so not v9.

If I do a debug by line should I see this class’s file being parsed at some point prior to the bit that scans and causes the error?

driskell commented 3 years ago

@mortenson I tracked it down to the fact that the psalm Symfony plugin was not queueing the ClassLikeStorage scan for the services in the container. This is because it waits until it encounters ContainerInterface. However, this never seems to happen in my scan and so at the first encounter of a \Drupal::service call although the psalm Drupal plugin returns the correct service, it wasn't populated in the ClassLikeStorage so thus the error.

As a quick test I added to ContainerHandler.php afterClassLikeVisit hook to psalm Drupal plugin (similar to the Symfony plugin) and just made it check for Drupal class and when it encounters it, queue all of the classes in the container, kind of how the Symfony plugin does. I then added to Plugin.php to add the core/lib/Drupal.php as a stub to make sure it encounters it (because it wasn't for me.) It then successfully added all the containers classes and the scan completed!

I'll tinker a little more if I can.

mortenson commented 3 years ago

Thanks! If you have time to submit PR I'll gladly review.

driskell commented 3 years ago

@mortenson Before I do any PR I just wondered if you had any ideas why it might not have been working without this?

Specifically, inside the Symfony Plugin it automatically queues crawling for "ClassLikeStorage" as soon as it encounters ContainerInterface from the Symfony Dependency Injection component.

Clearly I do not understand how Psalm works or misunderstand the ClassLikeStorage's purpose, as I would have thought it would have seen the "Drupal" class reference, and therefore the service method, and therefore the ContainerInterface and therefore crawl that and trigger the container to be populated into the ClassLikeStorage. However, what I see is that during ClassLikeStorage it never touches Drupal class at all, thus doesn't. I can only get it to touch Drupal class it by adding the Drupal.php file as a stub. Even then, it does not see the ContainerInterface, thus the need for afterClassLikeVisit to handle the container population on the Drupal class.

I think I may try to run this on a stock Drupal install to test but it seems to me this project would never have worked. That seems unlikely to me but I can't work out what is different - why Drupal class and ContainerInterface don't get touched in ClassLikeStorage. I guess I just want to make sure I'm not contributing to any technical debt in the project by contributing something that actually, I do not understand why it was needed!

Unfortunately I didn't get very far than the above which just widened my knowledge of not knowing how Psalm works :) If I find more time in the coming weeks I will try to dig further if you don't get any further - would be amazing to get this working easily as I saw two pretty good Taint detections (both extremely wild but possible) in my own code base! Maybe I just need to try dig documentation on how the hooks work and what they do as they seem very reliant on you knowing how Psalm works

mortenson commented 3 years ago

@driskell The Symfony Psalm plugin only supports a specific hardcoded list of container-like classes, which is why https://github.com/mortenson/psalm-plugin-drupal/blob/master/ContainerHandler.php exists.

It seems logical that we may need to copy more from that codebase, which is sounds like you did in testing. This is the "hardcoded" line in the Symfony Psalm plugin that isn't detecting Drupal, I think https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Handler/ContainerHandler.php#L141

Alternatively, and I did mean to do this originally, we could PR the Symfony Psalm plugin repo to make this configurable: https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Handler/ContainerHandler.php#L33-L38

driskell commented 3 years ago

It seems logical that we may need to copy more from that codebase, which is sounds like you did in testing. This is the "hardcoded" line in the Symfony Psalm plugin that isn't detecting Drupal, I think https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Handler/ContainerHandler.php#L141

When I debugged it this line was never reached with “Drupal” as the class name. When I added it as a stub it hit but I expected it to hit ContainerInterface in that file. To be honest I don’t know where these classes are found they clearly aren’t crawled by looking in the files?