bug #20995 [DependencyInjection] Fix the priority order of compiler pass trait (francoispluchino)

nicolas-grekas · nicolas-grekas · commit bd626bc23a00 · 2017-01-03T18:12:19.000+01:00
This PR was merged into the 3.2 branch. Discussion ---------- [DependencyInjection] Fix the priority order of compiler pass trait | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20332, #20993 | License | MIT | Doc PR | ~ A regression has appeared between the version `3.1` and `3.2` on the compilation passes using a priority option (see #20332). Indeed, the order is no longer preserved for service definitions with the same priority. This is caused by the `SplPriorityQueue` class that does not have a FIFO implementation (see this [comment](symfony/symfony#20332 (comment))). The PR #20993 fixes the problem but only for Forms, not for all compiler passes using the `PriorityTaggedServiceTrait` trait since `3.2`. Commits ------- aac9a7e Fix the priority order of compiler pass trait
diff --git a/Compiler/PriorityTaggedServiceTrait.php b/Compiler/PriorityTaggedServiceTrait.php
@@ -24,24 +24,34 @@ trait PriorityTaggedServiceTrait
     /**
      * Finds all services with the given tag name and order them by their priority.
      *
+     * The order of additions must be respected for services having the same priority,
+     * and knowing that the \SplPriorityQueue class does not respect the FIFO method,
+     * we should not use this class.
+     *
+     * @see https://bugs.php.net/bug.php?id=53710
+     * @see https://bugs.php.net/bug.php?id=60926
+     *
      * @param string           $tagName
      * @param ContainerBuilder $container
      *
      * @return Reference[]
      */
     private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
     {
-        $services = $container->findTaggedServiceIds($tagName);
-
-        $queue = new \SplPriorityQueue();
+        $services = array();
 
-        foreach ($services as $serviceId => $tags) {
+        foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $tags) {
             foreach ($tags as $attributes) {
                 $priority = isset($attributes['priority']) ? $attributes['priority'] : 0;
-                $queue->insert(new Reference($serviceId), $priority);
+                $services[$priority][] = new Reference($serviceId);
             }
         }
 
-        return iterator_to_array($queue, false);
+        if ($services) {
+            krsort($services);
+            $services = call_user_func_array('array_merge', $services);
+        }
+
+        return $services;
     }
 }
diff --git a/Tests/Compiler/PriorityTaggedServiceTraitTest.php b/Tests/Compiler/PriorityTaggedServiceTraitTest.php
@@ -33,6 +33,12 @@ public function testThatCacheWarmersAreProcessedInPriorityOrder()
             'my_service11' => array('my_custom_tag' => array('priority' => -1001)),
             'my_service12' => array('my_custom_tag' => array('priority' => -1002)),
             'my_service13' => array('my_custom_tag' => array('priority' => -1003)),
+            'my_service14' => array('my_custom_tag' => array('priority' => -1000)),
+            'my_service15' => array('my_custom_tag' => array('priority' => 1)),
+            'my_service16' => array('my_custom_tag' => array('priority' => -1)),
+            'my_service17' => array('my_custom_tag' => array('priority' => 200)),
+            'my_service18' => array('my_custom_tag' => array('priority' => 100)),
+            'my_service19' => array('my_custom_tag' => array()),
         );
 
         $container = new ContainerBuilder();
@@ -47,15 +53,21 @@ public function testThatCacheWarmersAreProcessedInPriorityOrder()
 
         $expected = array(
             new Reference('my_service2'),
+            new Reference('my_service17'),
             new Reference('my_service1'),
+            new Reference('my_service18'),
             new Reference('my_service8'),
+            new Reference('my_service15'),
             new Reference('my_service4'),
+            new Reference('my_service19'),
             new Reference('my_service5'),
+            new Reference('my_service16'),
             new Reference('my_service9'),
             new Reference('my_service7'),
             new Reference('my_service6'),
             new Reference('my_service3'),
             new Reference('my_service10'),
+            new Reference('my_service14'),
             new Reference('my_service11'),
             new Reference('my_service12'),
             new Reference('my_service13'),

Original file line number	Diff line number	Diff line change
`@@ -24,24 +24,34 @@ trait PriorityTaggedServiceTrait`
`24`	`24`	`/**`
`25`	`25`	`* Finds all services with the given tag name and order them by their priority.`
`26`	`26`	`*`
	`27`	`+ * The order of additions must be respected for services having the same priority,`
	`28`	`+ * and knowing that the \SplPriorityQueue class does not respect the FIFO method,`
	`29`	`+ * we should not use this class.`
	`30`	`+ *`
	`31`	`+ * @see https://bugs.php.net/bug.php?id=53710`
	`32`	`+ * @see https://bugs.php.net/bug.php?id=60926`
	`33`	`+ *`
`27`	`34`	`* @param string $tagName`
`28`	`35`	`* @param ContainerBuilder $container`
`29`	`36`	`*`
`30`	`37`	`* @return Reference[]`
`31`	`38`	`*/`
`32`	`39`	`private function findAndSortTaggedServices($tagName, ContainerBuilder $container)`
`33`	`40`	`{`
`34`		`- $services = $container->findTaggedServiceIds($tagName);`
`35`		`-`
`36`		`- $queue = new \SplPriorityQueue();`
	`41`	`+ $services = array();`
`37`	`42`
`38`		`- foreach ($services as $serviceId => $tags) {`
	`43`	`+ foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $tags) {`
`39`	`44`	`foreach ($tags as $attributes) {`
`40`	`45`	`$priority = isset($attributes['priority']) ? $attributes['priority'] : 0;`
`41`		`- $queue->insert(new Reference($serviceId), $priority);`
	`46`	`+ $services[$priority][] = new Reference($serviceId);`
`42`	`47`	`}`
`43`	`48`	`}`
`44`	`49`
`45`		`- return iterator_to_array($queue, false);`
	`50`	`+ if ($services) {`
	`51`	`+ krsort($services);`
	`52`	`+ $services = call_user_func_array('array_merge', $services);`
	`53`	`+ }`
	`54`	`+`
	`55`	`+ return $services;`
`46`	`56`	`}`
`47`	`57`	`}`