Investigate possibilities to parse attributes without reflection

We had a discussion about DrupalCon Lille about this.

One idea was to create an 'attribute registry' - i.e. to essentially parse out all attributes from a file at once and store them somewhere in a format.

That way if a single file contains (or might contain) multiple attributes types that all need to be discovered, we would only check that file once instead of once-per attribute type.

However it would be complex to implement so we thought better to see how we're doing after conversions.

Ideas like checking for #( worth doing first too if they're effective.

Have you looked at https://github.com/olvlvl/composer-attribute-collector?

Move nikic/php-parser up from dev dependency to normal and run something like https://gist.github.com/chx/9f289ffd541701228ba98243dd7eaff7

@mstrelan it uses reflection https://github.com/olvlvl/composer-attribute-collector/blob/a3d45a2e9bab... it just does it when the file gets installed by composer, it would be a very different development workflow compared to what we have now.

This might be less necessary with 📌 Add a filecache-backed attribute parser Active which will persist attribute parsing between full cache clears (but not yet between deployments, but see related issues).

One thing to consider besides performance issues, a problem that's come up in 📌 Convert MigrateSource plugin discovery to attributes Active and 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review is that attributes can't be read via Reflection on classes that implement missing interfaces, extend missing classes, or use missing traits, in the case that the missing dependency comes from an uninstalled module. While Reflection will throw an exception for a missing interface or class, PHP will fatal on a missing trait. The exception thrown during plugin discovery was handled in 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review , but a solution for handling missing traits gracefully is still outstanding.

Using something like nikic/php-parser to retrieve attribute values without might be a viable way forward to retrieve at least some attribute values in the case of missing dependencies.

#12 is interesting. And a possible counterpoint to #11 is that the caching rather than cancelling this issue out, gives us more options to potentially adopt a slower but more memory-efficient attribute parser.

Created 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active to explore using nikic/php-parser to help with plugin discovery per #12 📌 Investigate possibilities to parse attributes without reflection Active and put up an MR.
Hat tip to @ghost of drupal past for the library suggestion and gist.

Surfacing a comment I made in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active about missing dependencies that I think warrants mention here outside the specific context of plugin discovery:

That being said, I think the scope of the missing dependencies issue will expand as attribute-based discovery expands to routes ( 📌 Use PHP attributes for route discovery Needs review ), and services ( ✨ Directory based automatic service creation Needs work , 📌 Support #[AsEventListener] attribute on classes and methods Active , ✨ Support hooks (Hook attribute) in components Active ), etc. It might actually make sense to provide some sort of missing-dependency-safe wrapper for Reflection to address all these use cases.

Note both autowire and autoconfigure means every class defined as a service will be loaded and reflected during rebuild by Symfony. I think this issue is completely moot in light of that.

For hooks, a performance improvement would be to pass down the container as far as getHookAttributesInClass and use $container->getReflectionClass() to save on reflecting the classes twice -- ContainerBuilder keeps the reflections in a property.

Btw the same is true for TwigExtensionPass it's just even simpler I will include it because it shows what I am talking about:

      $class_name = $container->getDefinition($service_id)->getClass();
      $reflection = new \ReflectionClass($class_name);

change to

      $class_name = $container->getDefinition($service_id)->getClass();
      $reflection = $container->getReflectionClass($class_name);

Created 📌 Use container builder to track and reuse reflection classes in compiler passes Active to reuse reflection classes in the container builder per #16. Changes were small and straightforward, so MR is ready.

I suspect that the main cost of reflection is loading and parsing the class file. And of course scanning the filesystem.
After that, I suspect we can call "new ReflectionClass" and "$reflector->getAttributes()" repeatedly without much extra cost.
So we should be really careful about the cost/benefit.

I tried to see what kind of caching symfony does.
I see a runtime cache (in a variable), I don't see anything persistent.
Besides, that symfony code looks quite scary to me, like it does a lot of stuff we don't need.

Maybe the benefit depends on how many times we want attributes from the same class for different purposes.

Regarding a parser instead of php:
In the past I created https://github.com/donquixote/quick-attributes-parser/-
It is definitely faster than nikic php-parser, at least it was at the time.
But I still would not recommend this for performance gains. I only did this because I wanted to support attributes in older php versions.

Here is the link to the repo: https://github.com/donquixote/quick-attributes-parser/ the one in 18 404s for me.

I tried to see what kind of caching symfony does.
I see a runtime cache (in a variable), I don't see anything persistent.

Yes, but Symfony by default compiles the container to PHP in a build step, which is then pushed to production, whereas because of modules installation vs uninstallation, Drupal has to build its container dynamically on production. This has been the disconnect between Drupal and the Symfony container since it was adopted.