sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Add priority for admin extensions #4036

Closed hason closed 7 years ago

hason commented 8 years ago

I want to register a global admin extension as the last extension.

greg0ire commented 8 years ago

Doesn't the order in which you declare the extension services help you do just that?

hason commented 8 years ago

No. The problem is in https://github.com/sonata-project/SonataAdminBundle/blob/3.x/DependencyInjection/Compiler/ExtensionCompilerPass.php#L62-L73 The universal extensions are registered as the first, and custom extensions as the second. Furthermore, universal extensions are not sorted. I suggest this enhancement:

$extensions = $universalExtensions; // [ priority1 => [...], priority2 => [...]]
$extensions = merge($extension, $this->getExtensionsForAdmin($id, $admin, $container, $extensionMap);
krsort($extensions);
$extensions = call_user_func_array('array_merge', $extensions);

foreach ($extensions as $extension) {
    $admin->addMethodCall('addExtension', array(new Reference($extension)));
}
greg0ire commented 8 years ago

Do you really care about the order, or would you rather establish dependencies between extensions (that is, all you want is extension A to be executed before extension B)?

greg0ire commented 8 years ago

This could be implemented like ordered fixtures are, maybe?

OskarStark commented 8 years ago

Sometimes the order is important

A getOrder() Method could be very helpful

hason commented 8 years ago

I prefer set order in DI. This is the standard method - http://symfony.com/doc/current/reference/dic_tags.html. DI tags with priority:

We can use a trait https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php

greg0ire commented 8 years ago

:+1: @hason , that would be mean a lot less code. Less is more. Are you planning on making a PR?

OskarStark commented 8 years ago

indeed, great idea @hason 👍