Introduce an AdminUrlGeneratorInterface

[jdecool]

The AdminUrlGenerator class is currently declared final.

As it's final class, it can't be mocked which can be a problem. In my case, I use the generator in a custom class and I can't simply create a unit test for this one, because I cant' mock it, I need to create a real instance, which depends on another final class (eg. AdminContextProvider).

So I suggest to add a AdminUrlGneratorInterface to solve this issue.

[javiereguiluz]

Thanks Jérémy! Yes, I agree we should add this. I have two comments about this proposal:

1. The interface should be created in src/Contracts/Router/ because Contracts is where we define all our interfaces (this is a similar idea to what Symfony does with its contracts): https://github.com/EasyCorp/EasyAdminBundle/tree/4.x/src/Contracts

2. If I'm right, interfaces should only contain the minimum amount of methods to be useful. Later, classes that implement those interfaces can add useful shortcut methods to improve DX. For instance, this could be enough:

interface AdminUrlGeneratorInterface
{
    public function get(string $paramName);
    public function set(string $paramName, $paramValue): self;
    public function generateUrl(): string;
}

Our implementation adds many specific set...() methods because it's easier to use setController() than set(EA::CRUD_CONTROLLER_FQCN, $crudControllerFqcn) but for the interface, set() and get() should be everything that you need (in addition to generateUrl()).

Before doing any changes in this PR, let's wait to read more comments from other people. Thanks!

[jdecool]

Thanks for your feedback @javiereguiluz

OK the first point.

And I completely agree with the second point. I admit that I extracted the interface from the existing class without asking about what I should put in.

[evertharmeling]

+1 for the interface 😊

What I find often confusing, is how the url generation works in the EasyAdminBundle.

For example:

$this->adminUrlGenerator
    ->setController($crudFqcn)
    ->setAction($action)
    ->setEntityId($entityId)

And the template variant:

ea_url()
    .setController(crudControllerFqcn)
    .setAction('edit')
    .setEntityId(entity.primaryKeyValue)

I'm always searching what I did last time (or even have a helper class with a proxy method).

So since it would be a specific UrlGeneratorInterface for the EasyAdminBundle, why don't we just support the common url generation flows directly? In my opinion interfaces should make (developers) life easier not (only) the code.

To me the following methods would make most sense:

interface AdminUrlGeneratorInterface
{
    public function generate(string $name, array $parameters): string;
    public function generateCrud(string $crudControllerFqcn, string $action, $entityId = null): string;
}

The generate() method just creates a url for a custom controller (with 'ea_context'), and generateCrud would generate a url for the most common used CrudController.

This would also be closer to Symfony's UrlGeneratorInterface.

Or am I the only one facing this 😅.

[michaelKaefer]

So the only purpose of the new interface would be to use it in an application's unit test. But what if the tested class looks like that:

class MyClass
{
    public function __construct(private AdminUrlGenerator $adminUrlGenerator) {}

    public function foo(): string
    {
        // ...
        $url = $this->adminUrlGenerator
            ->setController($controller)
            ->setAction($action)
            ->setEntityId($entityId)
            ->setReferrer($referrer)
            ->removeReferrer()
            ->generateUrl();
        // ...
    }
}

Then it's necessary that the interface includes the same public methods like the implementation if I don't miss something.

If you don't need the real AdminUrlGenerator but only a mock to test the class, is it a lot of effort to just remove this dependency from the class? I mean if the only argument to add the new interface is to use it in custom unit tests then we could add an interface for every other final class as well I guess..

[javiereguiluz]

I'm really sorry we didn't merge this PR on time. Meanwhile, another similar proposal was merged. See #5909. So, I'm closing this PR. I'm sorry.