very good! 👏

I'm not very experienced with attributes, wouldn't it make sense to use a separate class for attributes?

Both would work I guess. There was also a similar discussion in symfony/symfony#37474 for the Route attribute. There, now both strategies share a single class: https://github.com/symfony/symfony/blob/896f4a6eb6a986cc29e096dfe1f3f7042b03126f/src/Symfony/Component/Routing/Annotation/Route.php#L14-L26

I like that you can easily refactor your code (not even a new import needed) and the behavior stays identical. On the downside there is less type safety in the constructor. But IMHO this could/should be tweaked in AnnotationReader#getClassAnnotations() if this becomes a problem. 🤔 .

Did you consider using named arguments instead of the array?

#[ServiceTag('target_service', priority: 123, bar: 'baz')]

Should work fine using the constructor signature:

__construct($name, ...$data)

Did you consider using named arguments instead of the array?

Didn't know that this works with arbitrary names. ☺️
I actually like this! Maybe it's even possible to specify type hints for these things in the future.

@Toflar @aschempp Wdyt?

Switched to named arguments in 19c6bf0.

I also renamed the first argument to $serviceName as it's less likely to clash with any of the named data arguments (in contrast to $value or $name).

Very nice! I also didn't know about that 🙃

I also renamed the first argument to $serviceName as it's less likely to clash with any of the named data arguments (in contrast to $value or $name).

I would actually go with $value here, since that's the internal name the annotation reader uses as well, and having a value key on a tag wouldn't work with that either. WDYT?

IMHO the downside to $value is that it doesn't make it obvious what to enter when you're using auto completion in your IDE.

But I'm fine with both.

That's true. But $serviceName doesn't make that either? Wouldn't it be $tagName or just $name then (similar to YAML { name: kernel.event_listener, event: foobar }? Also consider that we have child annotations/attributes in Contao where the value has a different meaning (but they can use a different constructor, so that might not be relevant).

@Toflar just pointed me to https://symfony.com/blog/new-in-symfony-5-3-service-autoconfiguration-and-attributes
maybe that whole bundle will become obsolete?

maybe that whole bundle will become obsolete?

Not sure about this. If I get this correctly, you could get rid of the ServiceTagInterface but would still need the plumbing like so:

$container->registerAttributeForAutoconfiguration(
  ServiceTag::class, 
  static function (ChildDefinition $definition, ServiceTag $attribute): void {
    $definition->addTag($attribute->getName(), $attribute->getAttributes());
  }
);

I guess Contao (or any consumer) could implement their special attributes without this bundle but would then on the other hand need to require the Sf DI component in >= 5.3. Hmm.

(Speaking of Contao: We should consider following the As<Something> naming convention of Symfony. See https://github.com/symfony/symfony/pull/40556…)

yes that's what I mean. Instead of adding attributes trough this bundle, we could add support for attributes when Contao requires Symfony 5.3 (like in 4.12 or 4.13). If we agree we MUST have attribute support e.g. in 4.9, then we need to implement it in this bundle.

Attributes are supported natively by Symfony and Contao, therefore I'm closing this PR.