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

Deprecate constructing AdminPoolLoader with more than one argument #6978

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions UPGRADE-3.x.md
Expand Up @@ -4,6 +4,11 @@ UPGRADE 3.x
UPGRADE FROM 3.xx to 3.xx
=========================

### `Sonata\AdminBundle\Route\AdminPoolLoader`

- Deprecated constructing it passing more than one argument.
- Deprecated `Sonata\AdminBundle\Route\AdminPoolLoader` service alias.

## Deprecated not setting "sonata.admin.audit_reader" tag in audit reader services

If you are using [autoconfiguration](https://symfony.com/doc/4.4/service_container.html#the-autoconfigure-option),
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Expand Up @@ -46,7 +46,7 @@
"symfony/event-dispatcher-contracts": "^1.1 || ^2.0",
"symfony/expression-language": "^4.4 || ^5.1",
"symfony/form": "^4.4.12",
"symfony/framework-bundle": "^4.4",
"symfony/framework-bundle": "^4.4.6",
"symfony/http-foundation": "^4.4 || ^5.1",
"symfony/http-kernel": "^4.4",
"symfony/options-resolver": "^4.4 || ^5.1",
Expand Down
Expand Up @@ -238,9 +238,6 @@ static function (array $a, array $b): int {
$pool->replaceArgument(1, $admins);
$pool->replaceArgument(2, $groups);
$pool->replaceArgument(3, $classes);

$routeLoader = $container->getDefinition('sonata.admin.route_loader');
$routeLoader->replaceArgument(1, $admins);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These $admins are the same that the ones injected to the Pool (5 lines above).

}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/Resources/config/core.php
Expand Up @@ -78,11 +78,13 @@
->tag('routing.loader')
->args([
new ReferenceConfigurator('sonata.admin.pool'),
[],
new ReferenceConfigurator('service_container'),
])

->alias(AdminPoolLoader::class, 'sonata.admin.route_loader')
->deprecate(...BCDeprecationParameters::forConfig(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also deprecated the alias since I don't think it's useful, the original service is meant to be loaded as routing.loader, not to inject it in another service.

'The "%alias_id%" alias is deprecated since sonata-project/admin-bundle 3.x and will be removed in 4.0.',
'3.x'
))

->set('sonata.admin.helper', AdminHelper::class)
->public()
Expand Down
43 changes: 37 additions & 6 deletions src/Route/AdminPoolLoader.php
Expand Up @@ -34,18 +34,33 @@ class AdminPoolLoader extends Loader
protected $pool;

/**
* NEXT_MAJOR: Remove this property.
*
* @var array
*/
protected $adminServiceIds = [];

/**
* @var ContainerInterface
* NEXT_MAJOR: Remove this property.
*
* @var ContainerInterface|null
*/
protected $container;

public function __construct(Pool $pool, array $adminServiceIds, ContainerInterface $container)
// NEXT_MAJOR: Remove $adminServiceIds and $container parameters.
public function __construct(Pool $pool, array $adminServiceIds = [], ?ContainerInterface $container = null)
{
$this->pool = $pool;
// NEXT_MAJOR: Remove next line.
if (\func_num_args() > 1) {
@trigger_error(sprintf(
'Passing more than one argument to "%s()" is deprecated since'
.' sonata-project/admin-bundle 3.x.',
__METHOD__
), \E_USER_DEPRECATED);
}

// NEXT_MAJOR: Remove the following two lines.
$this->adminServiceIds = $adminServiceIds;
$this->container = $container;
}
Expand All @@ -58,7 +73,8 @@ public function supports($resource, $type = null)
public function load($resource, $type = null)
{
$collection = new SymfonyRouteCollection();
foreach ($this->adminServiceIds as $id) {
// NEXT_MAJOR: Replace $this->getAdminServiceIds() with $this->pool->getAdminServiceIds()
foreach ($this->getAdminServiceIds() as $id) {
$admin = $this->pool->getInstance($id);

foreach ($admin->getRoutes()->getElements() as $code => $route) {
Expand All @@ -71,11 +87,26 @@ public function load($resource, $type = null)
}
}

$reflection = new \ReflectionObject($this->container);
if (file_exists($reflection->getFileName())) {
$collection->addResource(new FileResource($reflection->getFileName()));
// NEXT_MAJOR: Remove this block.
if (null !== $this->container) {
$reflection = new \ReflectionObject($this->container);
if (file_exists($reflection->getFileName())) {
$collection->addResource(new FileResource($reflection->getFileName()));
}
}

return $collection;
}

/**
* @return string[]
*/
private function getAdminServiceIds(): array
{
if ([] !== $this->adminServiceIds) {
return $this->adminServiceIds;
}

return $this->pool->getAdminServiceIds();
}
}
38 changes: 35 additions & 3 deletions tests/Route/AdminPoolLoaderTest.php
Expand Up @@ -18,6 +18,7 @@
use Sonata\AdminBundle\Admin\Pool;
use Sonata\AdminBundle\Route\AdminPoolLoader;
use Sonata\AdminBundle\Route\RouteCollection;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\Routing\Route as SymfonyRoute;
use Symfony\Component\Routing\RouteCollection as SymfonyRouteCollection;
Expand All @@ -27,12 +28,15 @@
*/
class AdminPoolLoaderTest extends TestCase
{
// NEXT_MAJOR: Remove next line.
use ExpectDeprecationTrait;

public function testSupports(): void
{
$container = new Container();
$pool = new Pool($container);
$pool = new Pool($container, ['foo_admin', 'bar_admin']);

$adminPoolLoader = new AdminPoolLoader($pool, ['foo_admin', 'bar_admin'], $container);
$adminPoolLoader = new AdminPoolLoader($pool);

$this->assertTrue($adminPoolLoader->supports('foo', 'sonata_admin'));
$this->assertFalse($adminPoolLoader->supports('foo', 'bar'));
Expand All @@ -43,7 +47,7 @@ public function testLoad(): void
$container = new Container();
$pool = new Pool($container, ['foo_admin', 'bar_admin']);

$adminPoolLoader = new AdminPoolLoader($pool, ['foo_admin', 'bar_admin'], $container);
$adminPoolLoader = new AdminPoolLoader($pool);

$routeCollection1 = new RouteCollection('base.Code.Route.foo', 'baseRouteNameFoo', 'baseRoutePatternFoo', 'baseControllerNameFoo');
$routeCollection2 = new RouteCollection('base.Code.Route.bar', 'baseRouteNameBar', 'baseRoutePatternBar', 'baseControllerNameBar');
Expand Down Expand Up @@ -73,4 +77,32 @@ public function testLoad(): void
$this->assertInstanceOf(SymfonyRoute::class, $collection->get('baseRouteNameBar_bar'));
$this->assertInstanceOf(SymfonyRoute::class, $collection->get('baseRouteNameBar_bar'));
}

/**
* NEXT_MAJOR: Remove this method.
*
* @group legacy
*/
public function testThrowsADeprecationConstructingWithContainer(): void
{
$container = new Container();
$pool = new Pool($container);

$this->expectDeprecation('Passing more than one argument to "Sonata\AdminBundle\Route\AdminPoolLoader::__construct()" is deprecated since sonata-project/admin-bundle 3.x.');
new AdminPoolLoader($pool, ['foo_admin', 'bar_admin'], $container);
}

/**
* NEXT_MAJOR: Remove this method.
*
* @group legacy
*/
public function testThrowsADeprecationConstructingWithAdminServicesIds(): void
{
$container = new Container();
$pool = new Pool($container);

$this->expectDeprecation('Passing more than one argument to "Sonata\AdminBundle\Route\AdminPoolLoader::__construct()" is deprecated since sonata-project/admin-bundle 3.x.');
new AdminPoolLoader($pool, ['foo_admin', 'bar_admin']);
}
}