Problem/Motivation
There are many places where contrib could be invoking methods on objects whose type is unknown to static analysis. Per #3096959-29: Drupal 9 deprecated code report, an example of this (from the Crop API module) is:
$scheme = \Drupal::service('file_system')->uriScheme($uri);
Because the @return
of \Drupal::service()
is documented simply as "mixed", PHPStan does not know what class/interface uriScheme() is being called on, and therefore can't determine whether it's deprecated.
As a result, it does not report a deprecation error, when in fact there is one. Failing to report these means contrib modules that pass Upgrade Status will still fail to work on Drupal 9.
Proposed resolution
What if we commit a file to the codebase of Upgrade Status that contains an index of all of core's method names (across all classes), and for each method name, which interfaces/classes define it. Note that this information is already determined for generating api.drupal.org, so perhaps we can leverage whatever code generates that to also generate this index file?
Then, can we add code to the PHPStan process that detects when a method is being called on an object of unknown type, and for those cases, looks up the method from this index file to determine the possible classes it might be getting called on. And, if the method is deprecated in at least one of these classes, then flag it as a potential deprecation error.
While this could lead to false positives (if it's a method name that's deprecated in one interface/class but not in another), that's preferable to false negatives, and the false positive can be fixed by adding the correct typehint to the flagged code.
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCross-referencing with https://github.com/mglaman/drupal-check/issues/123.
Comment #4
BerdirI talked about it with @mglaman before, according to him phpstan is supposed to see this and understand what service it is, but then somehow it doesn't want to report it as a deprecation.
PS: False negative/the issue title seems like a complicated way to describe this :)
Comment #5
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOk, here's an attempt to simplify it.
Comment #6
mglamanIn phpstan-drupal we try to create a service map so that we can detect calls like this properly.
That is what ServiceDynamicReturnTypeExtension is for.
HOWEVER I just noticed this only runs on direct calls to the container, not against `\Drupal` and its `service` method. So we need a matching dynamic return type for that.
phpstan-drupal issue: https://github.com/mglaman/phpstan-drupal/issues/190
Comment #7
mglamanComment #8
mglamanThis may have been fixed in the latest release: https://github.com/mglaman/phpstan-drupal/releases/tag/0.12.14
It includes a fix for \Drupal::getContainer()->get() or $container->get(). So \Drupal::service calls should be detected.
Comment #9
Gábor HojtsyIn that case feedback would be great from folks after they do a composer update :)
Comment #10
mglamanThe fix for
\Drupal::service
calls has been added to phpstan-drupal:0.12.15, which should provide all the required coverage for detecting the access of deprecated services.Comment #11
Gábor HojtsyThanks @mglaman. This should fix it for Upgrade Status once people do a composer update.
Comment #12
Gábor Hojtsy