had to fix this in the tests because the container method wouldn't return like this, same goes for the other test.

Was unused

Do we consider private methods in trait as part of the api and of the bc promises?

I think so as they are public to the class they are used in

this would need an update on the bc policy!

This PR made me think about handling priorities for formatters in #18450, but even if I love @javiereguiluz proposal to use \SplPriorityQueue (btw thanks for the heads-up :), I did not use it because I want to check that the same class is not used for many instances.

So I'm wondering even if the context is a bit different here, what about checking classes of tagged services to prevent one class to be registered with many service ids? Is this an expected possibility? Shouldn't we throw an exception in such case and add a test for it?

We could hold a $classes = array() before the foreach loops to do something like:

$services = $container->findTaggedServiceIds($tagName);
$queue = new \SplPriorityQueue();
$classes = array();

foreach ($services as $serviceId => $tags) {
    $serviceClass = $container->findDefinition($serviceId)->getClass();
    if ($serviceClass && isset($classes[$serviceClass]) {
        throw new InvalidArgumentException(sprintf('The service "%s" has the same "%s" class as service "%s"', $serviceId, $serviceClass, $classes[$serviceClass]);
    }
    $classes[$attributes['class']] = $serviceId;
    foreach ($tags as $attributes) {
        $priority = isset($attributes['priority']) ? $attributes['priority'] : 0;
        $queue->insert(new Reference($serviceId), $priority * -1);
    }

    return iterator_to_array($queue);
}

What do you think ?

I've thought about the situation but that would be a BC break as that case is possible right now

We could still trigger a warning as silenced error to throw an exception in 4.0 (if it worths it)?

I think there can be legit use-cases where you would want to something twice with a different priority.. I just don't think it's a smart idea. @stof usually you have a very good idea about things like this, what do you think?

Adding a deprecation warning and adding it anyway (behavioral change only in 4.0) seems fine to me.

Having several instances of the same class is a reasonable expectation. Think in terms of dependency injection: you can define 2 services using the same class and having a different configuration.

What I think @HeahDude is referring to is not once a class, but once a service

@GromNaN @phansys, I understand your POV, and I agree in a global scope and I don't know about cache warmers.
It can also be very common when you think about factories. But regarding this trait, it's about collecting collections of services (mainly sharing the same helper interface).
We use their ids but we actually need an instance of each class. It sounds really weird to me that a service in the cases used here (e.g serializer, argument resolver) uses the same helper class (e.g encoders, value resolvers) with two different instances and configuration.

If there is a legit use case for it, my guess is that there is also a chance that happens while misconfiguration or unexpected duplication (or overriding) with different id, so the user should know about it.

Or maybe we could just add a test for duplicated class in debug:container command, so we can easily check such case? I mean their are factories for those use cases, it could help debugging definitions issues.

Or what about using a parameter like:

private function findAndSortTaggedServices($tagName, ContainerBuilder $container, $uniqueClasses = false)

To perform a check only if necessary mandatory for a service's tag collection?

@HeahDude

services:
    app.listener.log_request_mail:
        class: App\Listener\LogRequest
        arguments: ["@logger.mail"]
        tags: [{name: kernel.request, priority: 100}]
    app.listener.log_request_udp:
        class: App\Listener\LogRequest
        arguments: ["@logger.udp"]
        tags: [{name: kernel.request, priority: 50}]

This is what would register only the first one with your example. If it was unique per service, only the first encountered of each would be logged:

services:
    app.listener.log_request_mail:
        class: App\Listener\LogRequest
        arguments: ["@logger.mail"]
        tags: [{name: kernel.request, priority: 100}, {name: kernel.request, priority: 50}]
    app.listener.log_request_udp:
        class: App\Listener\LogRequest
        arguments: ["@logger.udp"]
        tags: [{name: kernel.request, priority: 50}, {name: kernel.request, priority: 100}]

So by service id is feasible as this example makes no sense in the first place. However, it would also mean that the mail would be registered as 100 and the udp with 50.

Imo regarding this PR, I think I'll leave this as is. If it's a behavior that needs to be changed, a PR can be opened to deprecated it as 3.4 and 4.0 are still 1.5 year away.

This PR is indeed very good as is, thanks for that. I've just been confused by this behavior, sorry for this digression :)

Just to be sure (ref #18450 (comment)).

This PR make some passes use a trait to collect tagged services, are concerned:

1. Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface
2. Symfony\Component\Config\Resource\ResourceInterface\ResourceCheckerInterface
3. Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface
4. Symfony\Component\PropertyInfo\PropertyAccessExtractorInterface
5. Symfony\Component\Serializer\Encoder\EncoderInterface

Excepted 1 and 4, these interfaces have kind of a supports method (serializer encoders have supportsEncoding).

After a deep look at each of them, I ask again, does it really make sense to allow duplicated service classes with different service IDs or with different priorities? Should't this be tested?

Or is it the responsibility of each service using them to perform that check? Because currently, it seems nothing is preventing it.

Could we use SplPriorityQueue?

I have to admit that I've not used it before, but it looks interesting.
I'll check it out next week, thanks!

On Fri, 8 Apr 2016 16:32 Javier Eguiluz, notifications@github.com wrote:

In
src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
#18482 (comment):

+trait PriorityTaggedServiceTrait
+{

• /**

• \* Finds all services with the given tag name and order them by their priority.
  

• *
  

• \* @param string           $tagName
  

• \* @param ContainerBuilder $container
  

• *
  

• \* @return Reference[]
  

• */
  

• private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
• {

•    $services = $container->findTaggedServiceIds($tagName);
  

•    $sortedServices = array();
  

•    foreach ($services as $serviceId => $tags) {
  

Could we use SplPriorityQueue
http://php.net/manual/en/class.splpriorityqueue.php?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/symfony/symfony/pull/18482/files/3eb300496ea0c239dae35520d174830cef878e71#r59033296