Feature Request: Validate that `.info.yml` contains modules from which classes or functions are used

[Kingdutch]

In larger projects it's possible that modules are already enabled and thus that a class exists. However, it's possible that when the module being worked on is installed in a different project that the dependency isn't actually there.

It would be great since PHPStan knows what classes and functions are used and where they come from that it can check based on the namespace or path that the module to which the class or function belongs is in the .info.yml file of the module that's using it.

[mglaman]

@Kingdutch what level are you running PHPStan at? Because this should "just work" if running at higher levels. If you're only doing deprecation checks (drupal-check, upgrade_status) that isn't even level: 0. It is a custom runner.

[Kingdutch]

We're running PHPStan at level 8, although we do have an extensive baseline. I didn't see any of the warnings I'd expect in the baseline.

I'm not sure I explained correctly. If the feature is implemented I would expect, using Open Social as example, a use of a class from social_core in social_user to emit an error if social_core is not listed as dependency. Such error is not currently part of PHPStan Drupal as far as I could find.

[mglaman]

@Kingdutch oh! so try to report that you're accessing namespaces which may not actually be available at runtime.

I think this got up before. Because right now this projects consumes all possible namespaces. So an idea would be to allow configuring what modules should be scanned – either manually or scanning the core.extensions.yml.

It sounds like that is what you need. I think it's smarter to try and parse core.extensions.yml than to make someone manually track in phpstan.neon and equally dangerous to become out of date

[Kingdutch]

so try to report that you're accessing namespaces which may not actually be available at runtime.

Yes!

I think this got up before. Because right now this projects consumes all possible namespaces. So an idea would be to allow configuring what modules should be scanned – either manually or scanning the core.extensions.yml.

Not entirely! I think that's what I saw in #4 ?

I don't want to change the PHPStan configuration. What I want PHPStan to tell me is "Hey this module A is using code from this other module B, but the module A doesn't tell Drupal (using A's .info.yml file) that B is a dependency, that's an error".

[mglaman]

Ah, now I understand! Detecting if a dependent namespace is not available because its dependency on it is not declared.

I think that might be feasible but also very complex.

The first step would be https://github.com/mglaman/phpstan-drupal/issues/249, so we can easily get information about the current extension. The Extension object should have the dependencies list.

Then we need to determine what nodes to listen on

• MethodCall
• StaticCall
• StaticPropertyFetch
• New_

BUT! What about when modules have a ModuleHandlerInterface::moduleExists check? That might be hard to determine.

See this JSON:API module dynamic injection from content_moderation, which it exists:

https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/jsonapi/jsonapi.services.yml#L154

  # Access Control
  jsonapi.entity_access_checker:
    class: Drupal\jsonapi\Access\EntityAccessChecker
    public: false
    arguments: ['@jsonapi.resource_type.repository', '@router.no_access_checks', '@current_user', '@entity.repository']
    calls:
      # This is a temporary measure. JSON:API should not need to be aware of the Content Moderation module.
      - [setLatestRevisionCheck, ['@?access_check.latest_revision']] # This is only injected when the service is available.

So when setLatestRevisionCheck is checked, it'll throw an error when it shouldn't.

In all honestly, I think it would be worth adding and folks can ignore those errors when relevant and then maybe we can find a way to be smart about it.

[mglaman]

This PR appears to be related – checking cross-namespace usage: https://github.com/mglaman/phpstan-drupal/pull/248. It checks if something is marked @internal, but lays a good groundwork.

[mglaman]

We may need this to be a feature flag as it could cause problems due to Drupal's ability to have checks to see if a module is installed. For example, Commerce Log adds EventSubscribers for various Commerce modules if they exist.

See https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/log/src/CommerceLogServiceProvider.php

[mglaman]

I just thought about one interesting problem. What about dependencies from a dependency? It's not common for a module to be extremely verbose about it's dependencies, especially for ones inferred by its own dependencies.

Example:

foobar module relies on editor which ensures filter exists. This could cause errors if foobar doesn't also define a dependency onfilter.

Unless we rebuilt the dependency graph.

[Kingdutch]

We may need this to be a feature flag as it could cause problems due to Drupal's ability to have checks to see if a module is installed. For example, Commerce Log adds EventSubscribers for various Commerce modules if they exist.

See https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/log/src/CommerceLogServiceProvider.php

Ah this example tripped my up a bit. The issue is not actually in ComemrceLogServceProvider. That code will work just fine :D I suppose the analytical issue is in e.g. CartEventSubscriber itself, or more generally in plugins and event subscribers.

What you're rightly saying is that those classes use code from modules that may be optional but the code will only be executed (e.g. plugins loaded) if the optional modules are enabled. That's a tricky one indeed.

I just thought about one interesting problem. What about dependencies from a dependency? It's not common for a module to be extremely verbose about it's dependencies, especially for ones inferred by its own dependencies.

I think this is actually something that phpstan-drupal could improve in the ecosystem. Though that would certainly be a point for debate.

I know that indirect dependencies can cause problems. For example should it be a breaking change of module A when it removes a module B used for internal purposes just because module C depended only on A while actually also using code from B? I think it shouldn't be. So not declaring a dependency on B from C is actually the issue -- a bug (that this change could catch).

In larger projects we've also seen that not declaring all dependencies can easily lead to hidden cyclic dependencies which listing everything would've made explicit.

I thought Drupal core was quite good at listing all their dependencies but it seems that that is also not (or no longer) true.

One place where you must provide all modules you depend on is in a Kernel test. This is actually non-trivial for higher up modules that rely on a lot of transient dependencies. Those usually don't just allow copy pasting the info.yml dependencies but require actively diving into all the dependencies and also extracting their dependencies. This can make setting up initial tests quite difficult.

[mglaman]

@Kingdutch good find on Drupal core. I think it is worth pursuing, but not a Rule that is enabled by default.