Use nickic/php-parser for attribute-based plugin discovery to avoid errors

MR is up with tests.

The Dependencies attribute stuff may not be strictly needed, but I think it can help make plugin discovery more robust. If it ends up staying in, there will need to be a CR documenting its use.

Thanks for the review, @ghost of drupal past. I'll address the MR comments and update it sometime tomorrow or later, but here's a couple quick replies to #6

I think these limitations need to be documented on the Dependencies::__construct $modules argument. Especially the lack of constants.

This is a fair point, and I will add the documentation.

A depends on B depends on C where C doesn't exist , this code parses A and runs class_exists(B) which autoloads and happily throws a Fatal error: Uncaught Error: Class "C" not found.

Yeah, I tried to acknowledge that in this comment, but it's probably a little opaque:

  // There is a risk that a class or trait uses a missing trait somewhere
  // in its hierarchy and will cause a fatal error.

Let me give some background first. My initial idea was only to introduce the Dependencies attribute, and leave it to plugin developers to purposely designate which modules the plugin depends on, in order to avoid the fatal error on missing traits. While it is an additional DX hurdle, my thought was that this amount of friction is relatively small. It also really only applies to classes using missing traits, since we are already catching the exception for missing interfaces and classes.

While I was working with nikic/php-parser on this, I realized that the interfaces, classes, and trait dependencies were parsed and available, so I thought it was worth identifying those specifically as dependencies as well. Running the _exists() functions does run the risk of some trait in the class hierarchy being missing and causing the fatal. The chance of this happening is lower than a missing trait being used directly by the plugin class, and even in case there is a fatal error, that's when a plugin developer would use the Dependencies attribute.

There is a gap if a missing dependency in the hierarchy comes from some PHP library that is not a module, but I think this is a fringe use case.

Whole hog. Make DrupalKernel::$classLoader public readonly, use $kernel->classLoader->findFile() to find any classes/traits/interfaces being depended on, read 'em, parse 'em and recurse with a static cache.
...
And/or use statements could easy be parsed for non-existent modules.

Migrate source discovery by annotations does this. Trying to provide BC for this annotated discovery specifically for migrate source, on top of adding attribute discovery, results in some complex code. And recreating this functionality, again only for migrate source, makes the code even more complex. Handling recursive file parsing in the attribute discovery base class would eliminate the specific complexity for migrate source. In fact, an issue about moving this functionality from migrate source to core was created, even before attribute-based discovery was a thing, but it's been unresolved for years: https://www.drupal.org/project/drupal/issues/2786355.

That being said, I think the scope of the missing dependencies issue will expand as attribute-based discovery expands to routes (#3311365: Use PHP attributes for route discovery), and services (#3422359: Directory based automatic service creation, #3376163: Support #[AsEventListener] attribute on classes and methods, #3481903: Support hooks (Hook attribute) in any registered service), etc. It might actually make sense to provide some sort of missing-dependency-safe wrapper for Reflection to address all these use cases. Recursively iterating through all the files of all the classes in the hierarchies might be really slow, so that could eliminate it as an option.

And/or use statements could easy be parsed for non-existent modules.

Migrate source discovery by annotations does this.

Adding a correction that migrate source discovery does not quite do this. It does the recursion, but it checks the namespaces of only parent and ancestor classes and does not look at namespaces of traits or interfaces.

Rebased and addressed comments on the MR. I also moved all the "providers" logic to the core AttributeClassDiscovery class, since that's where it should happen.

good to see the constant expression evaluator here but without the helpers described in "Unsupported expressions and evaluator fallback" under https://github.com/nikic/PHP-Parser/blob/master/doc/component/Constant_e... it is quite limited and I think these limitations need to be documented on the Dependencies::__construct $modules argument. Especially the lack of constants.

Added this documentation for the modules parameter in the Dependencies constructor.

I'm continuing with the hybrid approach (option 2) for now. Hopefully can get this pushed forward and help get #3421014: Convert MigrateSource plugin discovery to attributes unblocked. It should be fairly easy to remove the *_exists() checks and rely solely on the Dependencies attribute if requested to go with option 1. I think option 3 of going through all the class hierarchies is too slow.

Along those lines, if nickic/php-parser is adopted here, and since it looks like BC support for plugin discovery by Annotations will continue for a while, plugin classes will be essentially parsed three times during discovery: once via the Doctrine-based StaticReflectionParser, then with nikic PhpParser, and then with Reflection. I think a reasonable follow up would be to replace or refactor StaticReflectionParser with PhpParser, and then somehow pass the parsed result into parseClass(), so that would remove one parsing pass. An eventual end goal could be parsing plugin Attributes with PhpParser as well, but as mentioned before, there is a lot of complexity in some of the Attribute classes.

Only gave a cursory investigation into https://github.com/Microsoft/tolerant-php-parser, which documents itself as "early-stage":

This is an early-stage PHP parser designed, from the beginning, for IDE usage scenarios (see Design Goals for more details). There is still a ton of work to be done, so at this point, this repo mostly serves as an experiment and the start of a conversation.

@catch Yes, that was part of my thinking of whether there'd need to be something like this for Hook discovery, or as attribute discovery may expand to services or routes, such as #3311365: Use PHP attributes for route discovery, #3422359: Directory based automatic service creation, #3376163: Support #[AsEventListener] attribute on classes and methods, and #3481903: Support hooks (Hook attribute) in any registered service.

Refactored this to try to make the functionality available to be used outside plugin discovery.

Read the php-parser docs some, and they recommend reusing the Parser class (https://github.com/Roave/BetterReflection/blob/6.53.x/docs/compatibility...), so I updated the MR to keep the Parser statically after instantiation. I'm not sure what memory or speed impact it has. Running drush si demo_umami -vvv consecutively multiple times did not produce consistent enough metrics to gauge a consistent difference to HEAD.

If this is driven by performance concerns

The performance concern is #3395260: Investigate possibilities to parse attributes without reflection . This one is about avoiding fatal errors when using reflection to read class attribute data. Leveraging a library here to do that can maybe provide a complete alternative to reflection in the parent issue, but that's not the goal here.

If this is driven by the need not to load the classes, then roave/better-reflection would be IMHO a better solution

I investigated this a little. The current approach here is not to replace Reflection, but to use static analysis (via PhpParserr) to determine whether the class is safe for Reflection, meaning no exceptions or fatal errors because of missing dependencies. Once the class is determined to be safe to reflect, then Reflection is used to get the attribute values. PhpParser works fine for this, and given that BetterReflection is built on top of PHP Parser, it seems unnecessary to use. I did look at whether BetterReflection could be used to replace Reflection in a way that is safe from missing dependencies. Unfortunately, it BetterReflection does not allow creating instances of the attributes (https://github.com/Roave/BetterReflection/blob/6.53.x/docs/compatibility...), so it's not really a fit for this issue.

I gave the suggestions in #25 a shot.

First, I tried registering an autoloader that just throws an exception. That way, all missing classes, interfaces, and traits throw the same exception that can be caught. Unfortunately this does not work for traits. While the exception is thrown, PHP still fatal errors, which looks something like this: PHP Fatal error: During class fetch: Uncaught Drupal\Component\Discovery\DiscoveryException: Plugin class dependency Drupal\a_module_that_does_not_exist\Plugin\CustomTrait is missing. in /var/www/html/core/lib/Drupal/Component/Discovery/DiscoveryClassLoader.php:19

Next, I tried aliasing any missing trait to some placeholder trait. This kind of works, but there are issues:

• The only way I could figure to do trait detection is to see whether the $class string being passed to the autoloader callback ends in "Trait". While there are only one trait in core that does not end in "Trait" (Drupal\Core\Menu\MenuLinkFieldDefinitions, though there are two other test traits: Drupal\Tests\dblog\Functional\FakeLogEntries, Drupal\Tests\workspaces\Functional\WorkspaceTestUtilities), there's nothing preventing people from creating traits in contrib or custom modules that do not end in Trait. Nor anything preventing people from unfortunately creating classes or interfaces with names that end in Trait, though likely very edge-casey
• Trait use statements with method aliases still fatal error, because the placeholder trait does not have the method:
  

  use CustomTrait {
      theMethod as theTraitMethod;
  }
  

• When the fatal error is avoided by aliasing, the plugin can then be erroneously discoverable. We can get around this by add a constant in the placeholder trait, then using Reflection to detect whether the plugin class has the constant.
  

  trait PlaceholderTrait {
  
    const PLUGIN_HAS_MISSING_DEPENDENCY = TRUE;
  
    public function __call(string $name, array $arguments) {}
  
  }
  
  // in AttributeClassDiscovery.
  protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
      // @todo Consider performance improvements over using reflection.
      // @see https://www.drupal.org/project/drupal/issues/3395260.
      $reflection_class = new \ReflectionClass($class);
  
      if ($reflection_class->hasConstant('PLUGIN_HAS_MISSING_DEPENDENCY')) {
        // Throw an exception or something.
      }
  

  There would still be an issue in the case where the module containing the trait is installed, so the plugin is discovered. But when the module is uninstalled, the plugin definition would still exist in the file cache, and any unfortunate attempt to instantiate the plugin class would hit a fatal error.

Just saw the comment about the new issue #3502913: Add a fallback classloader that can handle missing traits for attribute discovery. Will cross-post abbreviated thoughts from #27 there.

Did #3502913: Add a fallback classloader that can handle missing traits for attribute discovery to address missing traits instead.