Feature request - change breadcrumb uri for child admin

[cassianotartari]

Environment

Sonata packages

$ composer show sonata-project/*
sonata-project/admin-bundle              3.3.2 The missing Symfony Admin Generator
sonata-project/block-bundle              3.0.1 Symfony SonataBlockBundle
sonata-project/cache                     1.0.7 Cache library
sonata-project/core-bundle               3.0.3 Symfony SonataCoreBundle
sonata-project/doctrine-orm-admin-bundle 3.0.4 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  1.5.0 Lightweight Exporter library

Subject

Hi!

I don't know if this is the right place but I would like to ask for a feature. Otherwise please delete this.

I would like to set specific uri for a breadcrumb uri. Looking under BreadcrumbsBuilder there is:

if ($childAdmin) {
    $id = $admin->getRequest()->get($admin->getIdParameter());

    $menu = $menu->addChild(
        $admin->toString($admin->getSubject()),
        array(
            'uri' => $admin->hasRoute('edit') && $admin->isGranted('EDIT') ?
            $admin->generateUrl('edit', array('id' => $id)) :
            null,
        )
    );

    return $this->buildBreadcrumbs($childAdmin, $action, $menu);
}

So I guess that the only option is have a edit link in the parent object and I would like to change it to show action.

Is there any way to do it?

[greg0ire]

I don't know if this is the right place but I would like to ask for a feature.

It's the right place, the BreadcrumbsBuilder is part of this package.

Is there any way to do it?

No indeed, it's hardcoded.

Do you want to link to the show action when people don't have EDIT rights, or is it something else?

[cassianotartari]

Do you want to link to the show action when people don't have EDIT rights, or is it something else?

In my mind redirect to edit action is not best option to show information about the object, I am in a list view, the other link in breadcrumb is to list, all about show. And in my case there is much more information to show in the show action that in the edit action. It's a media, so in the show action shows a proxy player, metadata information about the file,...

Could be a semantic approach set link to the action before the actual view. For example: if I configured configureTabMenu to add a link to child list just on the show view of objects. When I click this link I will be redirect to list, so in the list I would like to go back to show parent object clicking on the breadcrumb. I don't know if I make myself clear.

[greg0ire]

Could be a semantic approach set link to the action before the actual view. For example: if I configured configureTabMenu to add a link to child list just on the show view of objects. When I click this link I will be redirect to list, so in the list I would like to go back to show parent object clicking on the breadcrumb.

I don't know if I make myself clear.

To be honest, I did not understand the second § of your answer, but I get that the show action is more interesting, so… how do you propose we handle this? Should there be a preferred action on an admin per admin basis, and for all references to an object of this admin? Or do you want to refer to "show" when and only when you are on the child admin?

[cassianotartari]

:joy::joy::joy::joy::joy::joy::joy::joy:

What a mess... I'll think about during the weekend and back on Monday reply again.

[greg0ire]

What a mess... I'll think about during the weekend and back on Monday reply again.

No problem :)

[cassianotartari]

Should there be a preferred action on an admin per admin basis, and for all references to an object of this admin? Or do you want to refer to "show" when and only when you are on the child admin?

I think this could be a good idea, has a setting to choose the preferred action globally to objects links. Maybe one setting to breadcrumb and other for other links. But to start maybe on child admin object's link should be to show action for now. At the show action there is a link to edit action, so it will cover the previously link action (edit).

In the show and list action we have route option in the field add and I always has to change to show, but on breadcrumbs we do not have this option. Other ideia could be think about a option as this (route). Maybe this could be set on admin service declaration. But even with these options I think globally setting to breadcrumb object link would be nice and easy to do now.

Just a off-topic, or not, would be great to has a easy way to use an filter route, link a object to its admin list filtered view. Sometimes I done this with a ugly concat twig string.

I made myself clear this time?

[greg0ire]

Yes! Can you provide a PR for the global breadcrumb setting?

[cassianotartari]

I will try to code something at my actual project and I will paste here some code before. I totally noob as you may have already know about PRs but I will give my best to help.

[cassianotartari]

What you think about:

Configuration

sonata_admin:
    options:
        breadcrumb_child_admin_route: show

Sonata\AdminBundle\DependencyInjection\Configuration

//....
                ->arrayNode('options')
                    ->addDefaultsIfNotSet()
                    ->children()
                        ->scalarNode('breadcrumb_child_admin_route')->defaultValue('show')->end()
//....

Sonata\AdminBundle\Admin\BreadcrumbsBuilder

//...
        if ($childAdmin) {
            $id = $admin->getRequest()->get($admin->getIdParameter());

            $route = $admin->getConfigurationPool()->getOption('breadcrumb_child_admin_route');

            $menu = $menu->addChild(
                $admin->toString($admin->getSubject()),
                array(
                    'uri' => $admin->hasRoute($route) && $admin->isGranted(strtoupper($route)) ?
                    $admin->generateUrl($route, array('id' => $id)) :
                    null,
                )
            );

            return $this->buildBreadcrumbs($childAdmin, $action, $menu);
        }
//...

Just a doubt, AdminInterface doesn't have getConfigurationPool, why?

[greg0ire]

1. $admin->getConfigurationPool()->getOption('breadcrumb_child_admin_route'); Don't do that, use normal dependency injection: the BreadcrumbsBuilder has a constructor, modify it and change the service definition so that the parameter gets injected
2. I think there might be several options for the breadcrumbs builder, so let's make the configuration look like it

sonata_admin:
       breadcrumbs:
           child_admin_route: show

3 . If you want to stay backwards compatible, have the setting default to edit, not show

[cassianotartari]

Configuration

sonata_admin:
    breadcrumbs:
        child_admin_route: show

Sonata\AdminBundle\DependencyInjection\Configuration

                ->arrayNode('breadcrumbs')
                    ->addDefaultsIfNotSet()
                    ->children()
                        ->scalarNode('child_admin_route')->defaultValue('edit')->end()
                    ->end()
                ->end()

Sonata\AdminBundle\DependencyInjection\SonataAdminExtension

$container->setParameter('sonata.admin.configuration.breadcrumbs', $config['breadcrumbs']);

sonata-project/admin-bundle/Resources/config/core.xml

        <service id="sonata.admin.breadcrumbs_builder" class="Sonata\AdminBundle\Admin\BreadcrumbsBuilder">
            <call method="setBreadcrumbsConfig">
                <argument>%sonata.admin.configuration.breadcrumbs%</argument>
            </call>
        </service>

Sonata\AdminBundle\Admin\BreadcrumbsBuilder

    /**
     * @var string[]
     */
    protected $breadcrumbsConfig = array();

    /**
     * 
     * @param string[] $breadcrumbsConfig
     */
    public function setBreadcrumbsConfig($breadcrumbsConfig) {
        $this->breadcrumbsConfig = $breadcrumbsConfig;
    }

    public function buildBreadcrumbs(AdminInterface $admin, $action, ItemInterface $menu = null)
    {
    //...
        if ($childAdmin) {
            $id = $admin->getRequest()->get($admin->getIdParameter());

            $menu = $menu->addChild(
                $admin->toString($admin->getSubject()),
                array(
                    'uri' => $admin->hasRoute($this->breadcrumbsConfig['child_admin_route']) && $admin->isGranted(strtoupper($this->breadcrumbsConfig['child_admin_route'])) ?
                    $admin->generateUrl($this->breadcrumbsConfig['child_admin_route'], array('id' => $id)) :
                    null,
                )
            );

            return $this->buildBreadcrumbs($childAdmin, $action, $menu);
        }
    //...
    }

[greg0ire]

Looks great! You should definitely make a PR on the 3.x branch for that, it would be easier to discuss. Something with docs and tests, of course ;)

Also, why not use the constructor and make the dependency non-optional? After all, since you use defaults in the config, you will always have something to provide to the breadcrumbs builder…

[cassianotartari]

Looks great! You should definitely make a PR on the 3.x branch for that, it would be easier to discuss. Something with docs and tests, of course ;)

That's my problem, tests, I don't code based on tests, too much time waste in my view, so I didn't know even how to proceed. Docs could be easy to add.

Also, why not use the constructor and make the dependency non-optional? After all, since you use defaults in the config, you will always have something to provide to the breadcrumbs builder…

Here I have a doubt, I don't remember/know what happens if I set config under constructor and others instanciate it because under AbstractAdmin has:

/**
     * NEXT_MAJOR : remove this method.
     *
     * @return BreadcrumbsBuilderInterface
     */
    final public function getBreadcrumbsBuilder()
    {
        @trigger_error(
            'The '.__METHOD__.' method is deprecated since version 3.2 and will be removed in 4.0.'.
            ' Use the sonata.admin.breadcrumbs_builder service instead.',
            E_USER_DEPRECATED
        );
        if ($this->breadcrumbsBuilder === null) {
            $this->breadcrumbsBuilder = new BreadcrumbsBuilder();
        }

        return $this->breadcrumbsBuilder;
    }

So I preferred call a independent method.

[greg0ire]

You should really write tests, start having a look at the BreadcrumbsBuilderTest. The BreadcrumbsBuilder is instanciated twice in there, you could try to use your option once in it, then see how it fails, and fix the corresponding expectations (you will have to change some 'edit' to 'show'. Tests might feel like a waste of time in projects, but in big libraries, they actually save us a lot of headaches / make us confident in our changes.

As per the constructor problem, you can do $this->getConfigurationPool()->getContainer()->getParameter(…), it's a bit ugly, but this piece of code will be removed, so… ugliness not a problem.

You could also do nothing and have the people who use this deprecated method not benefit from your changes :trollface:

Also, the constructor may look like this :

public function __construct(array $options = array())
{
}

and you may use the OptionsResolver. There is just one option at the moment, but I think there might be many others in the future.

[cassianotartari]

You should really write tests, start having a look at the BreadcrumbsBuilderTest. The BreadcrumbsBuilder is instanciated twice in there, you could try to use your option once in it, then see how it fails, and fix the corresponding expectations (you will have to change some 'edit' to 'show'. Tests might feel like a waste of time in projects, but in big libraries, they actually save us a lot of headaches / make us confident in our changes.

I'll give a shot.

As per the constructor problem, you can do $this->getConfigurationPool()->getContainer()->getParameter(…), it's a bit ugly, but this piece of code will be removed, so… ugliness not a problem.

This is to fix breadcrumbsBuilder instanciates under AbstractAdmin, right?

    /**
     * NEXT_MAJOR : remove this method.
     *
     * @return BreadcrumbsBuilderInterface
     */
    final public function getBreadcrumbsBuilder()
    {
        @trigger_error(
            'The '.__METHOD__.' method is deprecated since version 3.2 and will be removed in 4.0.'.
            ' Use the sonata.admin.breadcrumbs_builder service instead.',
            E_USER_DEPRECATED
        );
        if ($this->breadcrumbsBuilder === null) {
            $this->breadcrumbsBuilder = new BreadcrumbsBuilder($this->getConfigurationPool()->getContainer()->getParameter('sonata.admin.configuration.breadcrumbs'));
        }

        return $this->breadcrumbsBuilder;
    }

and you may use the OptionsResolver. There is just one option at the moment, but I think there might be many others in the future.

NICE! Always looking to this and never stopped to read about.

sonata-project/admin-bundle/Resources/config/core.xml

        <service id="sonata.admin.breadcrumbs_builder" class="Sonata\AdminBundle\Admin\BreadcrumbsBuilder">
            <argument>%sonata.admin.configuration.breadcrumbs%</argument>
        </service>

Sonata\AdminBundle\Admin\BreadcrumbsBuilder

use Symfony\Component\OptionsResolver\OptionsResolver;

//...

    /**
     * @var string[]
     */
    protected $breadcrumbsConfig = array();

    /**
     * 
     * @param string[] $breadcrumbsConfig
     */
    function __construct(array $breadcrumbsConfig = array())
    {
        $resolver = new OptionsResolver();
        $this->configureOptions($resolver);

        $this->breadcrumbsConfig = $resolver->resolve($breadcrumbsConfig);
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(array(
            'child_admin_route' => 'edit'
        ));
    }

[greg0ire]

This is to fix breadcrumbsBuilder instanciates under AbstractAdmin, right?

Yes, exactly!

If I may suggest, please create a PR with just the documentation, without coding anything first, and add the rest later. That way, if something is wrong with the PR, it will be caught before you code anything, sparing you a lot of headaches.

Also, I created the breadcrumbs builder test after if was coding, it's not TDD, that's why the file is so ugly IMO, it's really not the most beautiful test you will see in your life :P . So don't hesitate to ask for some help with this test. You should first try to run it before changing anything : phpunit Tests/Admin/BreadcrumbsBuilderTest.php

[cassianotartari]

If I may suggest, please create a PR with just the documentation, without coding anything first, and add the rest later.

Tried this on #4034