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

Created a trait to sort tagged services #18482

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
10 participants
@linaori
Copy link
Contributor

commented Apr 8, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

When writing the ControllerArgumentValueResolverPass, I needed a sorting on priority. I ended up copying a method from another class. I noticed this was done more often and 99% of the code was the same. I've moved the most common notation into the trait and "used" the trait in the priority aware passes. This increases horizontal re-use and means people can also use this in their bundles.

The cases that were slightly different, are still working completely. I had to fix some tests because they returned an invalid value from the mocked find method.

@@ -21,7 +21,7 @@ public function testThatCacheWarmersAreProcessedInPriorityOrder()
$services = array(
'my_cache_warmer_service1' => array(0 => array('priority' => 100)),
'my_cache_warmer_service2' => array(0 => array('priority' => 200)),
'my_cache_warmer_service3' => array(),
'my_cache_warmer_service3' => array(0 => array()),

This comment has been minimized.

Copy link
@linaori

linaori Apr 8, 2016

Author Contributor

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

@@ -52,7 +52,6 @@ public function testThatCheckersAreProcessedInPriorityOrder()
public function testThatCheckersCanBeMissing()
{
$definition = $this->getMock('Symfony\Component\DependencyInjection\Definition');

This comment has been minimized.

Copy link
@linaori

linaori Apr 8, 2016

Author Contributor

Was unused

$services = $container->findTaggedServiceIds($tagName);
$sortedServices = array();
foreach ($services as $serviceId => $tags) {

This comment has been minimized.

Copy link
@javiereguiluz

This comment has been minimized.

Copy link
@linaori

linaori Apr 8, 2016

Author Contributor

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

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

@javiereguiluz I've updated it to contain the SplPriorityQueue, had to inverse the insert priority in order to get the proper sorting though.

failure in travis is unrelated

*
* @return Reference[]
*/
private function findAndSortTaggedServices($tagName, ContainerBuilder $container)

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Apr 20, 2016

Contributor

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

This comment has been minimized.

Copy link
@linaori

linaori Apr 21, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 19, 2016

Member

this would need an update on the bc policy!

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

LGTM 👍

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

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 21, 2016

Member

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 ?

This comment has been minimized.

Copy link
@linaori

linaori Apr 21, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 21, 2016

Member

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

This comment has been minimized.

Copy link
@linaori

linaori Apr 21, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@GromNaN

GromNaN Apr 22, 2016

Contributor

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.

This comment has been minimized.

Copy link
@linaori

linaori Apr 22, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 22, 2016

Member

@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?

This comment has been minimized.

Copy link
@linaori

linaori Apr 22, 2016

Author Contributor

@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 comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 22, 2016

Member

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

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 25, 2016

Member

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.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

@iltar Can you rebase on the latest master branch to make all tests pass?

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

Thank you @iltar.

@fabpot fabpot closed this in f8dc459 Jun 14, 2016

nicolas-grekas added a commit that referenced this pull request Jun 14, 2016

bug #19049 [DependencyInjection] fix the sorting by priority (xabbuh)
This PR was merged into the 3.2-dev branch.

Discussion
----------

[DependencyInjection] fix the sorting by priority

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18482
| License       | MIT
| Doc PR        |

Commits
-------

6f72657 [DependencyInjection] fix the sorting by priority

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

@linaori linaori deleted the linaori:feature/priority-pass branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.