Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DependencyInjection] Autoconfigurable attributes on methods, properties and parameters #42039

Merged
merged 1 commit into from Aug 30, 2021

Conversation

@ruudk
Copy link
Contributor

@ruudk ruudk commented Jul 9, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

Introduction

#39897 introduced the possibility auto configure classes that were annotated with attributes:

$container->registerAttributeForAutoconfiguration(
    MyAttribute::class,
    static function (ChildDefinition $definition, MyAttribute $attribute, \ReflectionClass $reflector): void {
        $definition->addTag('my_tag', ['some_property' => $attribute->someProperty]);
    }
);

This works great. But it only works when the attribute is added on the class.

With this PR, it's now possible to also auto configure methods, properties and parameters.

How does it work?

Let's say you have an attribute that targets classes and methods like this:

#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::TARGET_PROPERTY)]
final class MyAttribute
{
}

You have two services that use them:

#[MyAttribute]
class MyService {
}

class MyOtherService {
    #[MyAttribute]
    public function myMethod() {}
}

You can now use registerAttributeForAutoconfiguration in your extension, together with a union of the types that you want to seach for. In this example, the extension only cares for classes and methods, so it uses \ReflectionClass|\ReflectionMethod $reflector:

final class MyBundleExtension extends Extension
{
    public function load(array $configs, ContainerBuilder $container) : void
    {
        $container->registerAttributeForAutoconfiguration(
            MyAttribute::class,
            static function (ChildDefinition $definition, MyAttribute $attribute, \ReflectionClass|\ReflectionMethod $reflector) : void {
                $args = [];
                if ($reflector instanceof \ReflectionMethod) {
                    $args['method'] = $reflector->getName();
                }
                $definition->addTag('my.tag', $args);
            }
        );
    }
}

This will tag MyService with my.tag and it will tag MyOtherService with my.tag, method: myMethod

If the extension also wants to target the properties that are annotated with attributes, it can either change the union to \ReflectionClass|\ReflectionMethod|\ReflectionProperty $reflector or it can just use \Reflector $reflector and do the switching in the callable.

Another example

Let's say you have an attribute like this:

#[Attribute(Attribute::TARGET_CLASS)]
final class MyAttribute
{
}

and you use it like this:

$container->registerAttributeForAutoconfiguration(
    MyAttribute::class,
    static function (ChildDefinition $definition, MyAttribute $attribute, \ReflectionClass|\ReflectionMethod $reflector) : void {
        $definition->addTag('my.tag');
    }
);

you'll get an error saying that ReflectionMethod is not possible as the attribute only targets classes.

@ruudk ruudk force-pushed the ruudk:method-attributes branch 3 times, most recently from ccc99fe to ba31f27 Jul 9, 2021
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 9, 2021

Scanning all methods of all autoconfigured services might be a perf hog on real apps.
We should understand the impact this could have before going this way.
Can you run some benchmark and report your findings please?
IIRC, Doctrine does it with an extra annotation on the class, basically saying "have a look at methods".

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Jul 9, 2021
@ruudk
Copy link
Contributor Author

@ruudk ruudk commented Jul 10, 2021

@nicolas-grekas Not sure how to properly benchmark this, but I used a Stopwatch in my large application and these are the results:
Class attributes took 3.000000 ms for 4469 classes
Method attributes took 18.500000 ms for 4469 classes and total of 41920 methods

Benchmark code
  private Stopwatch $stopwatch;
  private int $classes = 0;
  private int $methods = 0;

  public function __destruct()
  {
      dump(sprintf(
          'Class attributes took %f ms for %d classes',
          $this->stopwatch->getEvent('class')->getDuration(),
          $this->classes
      ));
      dump(sprintf(
          'Method attributes took %f ms for %d classes and total of %d methods',
          $this->stopwatch->getEvent('method')->getDuration(),
          $this->classes,
          $this->methods
      ));
  }

  protected function processValue($value, bool $isRoot = false)
  {
      // ...
      $this->stopwatch->start('class');
      ++$this->classes;
      foreach ($reflector->getAttributes() as $attribute) {
          if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) {
              $configurator($conditionals, $attribute->newInstance(), $reflector);
          }
      }
      $this->stopwatch->stop('class');
      $this->stopwatch->start('method');
      foreach ($reflector->getMethods() as $method) {
          ++$this->methods;
          foreach ($method->getAttributes() as $attribute) {
              if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) {
                  $configurator($conditionals, $attribute->newInstance(), $method);
              }
          }
      }
      $this->stopwatch->stop('method');
      // ...
  }

Not sure if this is acceptable and how it can be improved.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 10, 2021

Thanks for the bench, this looks acceptable!

@ruudk ruudk force-pushed the ruudk:method-attributes branch from c82e0a5 to 59b289d Jul 10, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

I'd suggest extending this to properties and parameters.

In order to play safe and optimize perf a bit, I'd suggest doing some reflection in the process() method to classify configurators in 4 groups: the ones that work on classes (ie the ones that have only two arguments or that have a 3rd arg that accept ReflectionClass), the ones that work on methods, properties, and parameters. If a group is empty, we would skip scanning for the corresponding type of attributes.

@ruudk
Copy link
Contributor Author

@ruudk ruudk commented Jul 10, 2021

In order to play safe and optimize perf a bit, I'd suggest doing some reflection in the process() method to classify configurators in 4 groups: the ones that work on classes (ie the ones that have only two arguments or that have a 3rd arg that accept ReflectionClass), the ones that work on methods, properties, and parameters. If a group is empty, we would skip scanning for the corresponding type of attributes.

@nicolas-grekas I applied this feedback and pushed a WIP commit.

Since Attributes only work on PHP 8 I thought it would be nice to support Union types when you want to register an attribute for multiple methods.

So for my example of the AsEventListener I want to typehint that with ReflectionClass | ReflectionMethod.

This works, but it will produce a fatal error on PHP 7.4 because it doesn't support unions there. How can we fix that?
Or should we target the AsEventListener change for Symfony 6 as that requires PHP 8 and up?

I'm also not sure how ReflectionParameter should be implemented. Should that work on all methods? Or only promoted parameters in the constructor? Aren't those already picked by ReflectionProperty?

Todo

  • How to handle AsEventListener on method change? Move to Symfony 6 or find alternative for Union?
  • Add and fix all tests
  • ReflectionParameter

@ruudk ruudk force-pushed the ruudk:method-attributes branch from ab4d96a to 866bbec Jul 10, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

LGTM, I only have 2 minor comments.
Thanks for completing this quickly!

@ruudk ruudk force-pushed the ruudk:method-attributes branch from 0144f4b to b60f575 Jul 15, 2021
@nicolas-grekas nicolas-grekas force-pushed the ruudk:method-attributes branch from 19eabc3 to eb77c5e Aug 25, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

@ruudk I force-pushed on your fork with the filter we talked about.
An exception will now be thrown if eg a ReflectionClass type is used with an attribute that cannot target a type, and one can use the generic Reflector type to get reflectors for all positions supported by an attribute.

@Tobion this should solve your concerns.

@nicolas-grekas nicolas-grekas force-pushed the ruudk:method-attributes branch 2 times, most recently from 6911e60 to c532028 Aug 25, 2021
@nicolas-grekas nicolas-grekas dismissed stof’s stale review Aug 25, 2021

outdated

@nicolas-grekas nicolas-grekas force-pushed the ruudk:method-attributes branch from c532028 to b449833 Aug 25, 2021
@nicolas-grekas nicolas-grekas force-pushed the ruudk:method-attributes branch from b449833 to 917fcc0 Aug 25, 2021
@ruudk
Copy link
Contributor Author

@ruudk ruudk commented Aug 27, 2021

Much better!

@fabpot
Copy link
Member

@fabpot fabpot commented Aug 27, 2021

@ruudk Can you work on a PR for the docs, or at least describe the feature with examples in the PR description? Thank you.

foreach ($reflector->getAttributes() as $attribute) {
if ($configurator = $autoconfiguredAttributes[$attribute->getName()] ?? null) {
$configurator($conditionals, $attribute->newInstance(), $reflector);
$conditionals = $instanceof[$classReflector->getName()] ?? new ChildDefinition('');

This comment has been minimized.

@Tobion

Tobion Aug 27, 2021
Member

I would also put all of this new code into a new method

This comment has been minimized.

@ruudk

ruudk Aug 28, 2021
Author Contributor

Will it really make it more clear? I don't agree, but if you give me a name for the method I'll do the change.

@Tobion
Tobion approved these changes Aug 27, 2021
Copy link
Member

@Tobion Tobion left a comment

I like it now. It will probably have some performance impact on container building. But I think using attributes for this might be the future. So there is no way around it. This will probably replace the marker interface solutions currently in place when there are such attributes as alternatives.

@ruudk
Copy link
Contributor Author

@ruudk ruudk commented Aug 28, 2021

@fabpot I updated the PR description and added a few examples. Hope this helps!

@fabpot
Copy link
Member

@fabpot fabpot commented Aug 30, 2021

@fabpot I updated the PR description and added a few examples. Hope this helps!

That's perfect, thank you.

@fabpot
fabpot approved these changes Aug 30, 2021
@fabpot
Copy link
Member

@fabpot fabpot commented Aug 30, 2021

Thank you @ruudk.

@fabpot fabpot merged commit a05b6a3 into symfony:5.4 Aug 30, 2021
11 checks passed
11 checks passed
@github-actions
Tests (7.2)
Details
@github-actions
Tests (7.2)
Details
@github-actions
Verify
Details
@github-actions
Psalm
Details
@github-actions
Tests (8.0)
Details
@github-actions
Tests (8.0)
Details
@github-actions
Tests (7.4, high-deps)
Details
@github-actions
Tests (8.0, low-deps)
Details
@github-actions
Tests (8.1, experimental) Tests (8.1, experimental)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
@ruudk ruudk deleted the ruudk:method-attributes branch Aug 30, 2021
fabpot added a commit that referenced this pull request Sep 1, 2021
This PR was merged into the 6.0 branch.

Discussion
----------

remove unneeded eval given PHP 8 in 6.0

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| License       | MIT
| Doc PR        |

Remove eval() added in #42039 for 6.0

Commits
-------

1a3ec8c remove unneeded eval given PHP 8 in 6.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants