Skip to content

Conversation

@sanderdlm
Copy link

@sanderdlm sanderdlm commented Oct 11, 2021

Updated the breadcrumbs to avoid the following situation in our latest framework projects:

class ProfileController extends AbstractController
{
    /**
     * @Breadcrumb("profile")
     */
    #[Route('/login', name:'login')]
    public function __invoke(

Should be reviewed thoroughly.

I've refactored quite a big chunk of the breadcrumb functionality to solve previously reported bugs. Recent project had some issues with the parent chaining part of the current breadcrumb implementation.

I've removed the ability to pass parameters to getter methods as they only allowed static values, e.g. {item.name:nl} and not dynamic properties like {item.name:locale} <- would't work, which makes them not really useful and we had to include a lot more code to support them. By removing getter parameters we can rely on the Symfony ProperyAccessor component, which is more robust and mature. If the situation calls for them we should look into how to make these things work in a clean way, but for now you can simply rewrite the method for the breadcrumb. For example:

private function getName(Locale $locale): string
{
    return $this->nameTranslations->get($locale);
}
// won't work anymore, {item.name} will crash

private function getBreadcrumb(): string
{
    return $this->nameTranslations->default();
}
// rewrite a new method like this and use as {item.breadcrumb}

Having a fallback for these kidsn of methods, like a regular getName which returns a default is a good idea anyway.

I've also removed the following example from the current docs:

/**
 * @Route("/{author}/{book}", name="book")
 * @Breadcrumb("{book.title}", parent={"name"="author", "parameters"={"author"="{author.id}"}})
 */

The nesting of parameters (parent={"name"="author", "parameters"={"author"="{author.id}"}}) when we already have the {author} parameter in the current URL isn't necessary in my opinion. For now we fallback to getId to perform the matching of the objects, eliminating the need to define any parameters for a parent route as long as the object names match and the ID is used for paramconversion. If custom paramconverters are used (for example hashes in the front-end), we could support those with a simpler attribute syntax, for example 'mapping' => 'hash'.

In short, using a logical URL structure and only referring to a parent route if your URL extends from it will work:

    //parent:
    #[Route('/order/{order}', name:'order_detail')]
    #[Breadcrumb('{order.number}')]
    public function __invoke(

    //child:
    #[Route('/order/{order}/update', name:'order_update')]
    #[Breadcrumb('update', parent:['name' => 'order_detail'])]
    public function __invoke(

This will work because {order} is present in the URL at all times during the breadcrumb chain.

The following will not:

    //parent:
    #[Route('/order/{order}', name:'order_detail')]
    #[Breadcrumb('{order.number}')]
    public function __invoke(

    //child (note the missing {order} in the URL):
    #[Route('/order/update', name:'order_update')]
    #[Breadcrumb('update', parent:['name' => 'order_detail'])]
    public function __invoke(

This might seem like a pretty big limitation, but the workaround that is needed to accommodate these kinds of breadcrumbs is huge, and the URL wouldn't even make sense.

To do:

  • Determine if we can simplify the route/parent syntax even more (make it a string, omit the possibility to pass parameters). This would make breadcrumbs rely 100% on the URL to resolve parameters, which would be less flexible, but in my opinion semantically correct.
  • Fix the docs
  • Remove the Twig global and use a Twig extension to render the breadcrumbs
  • Figure out if we even need the route option. When do we use this? Isn't parent always a better option? When would we link to a previous breadcrumb that isn't a parent?

@sanderdlm sanderdlm changed the title WIP: Breadcrumb annotations WIP: Breadcrumb with PHP8 attributes Oct 11, 2021
@sanderdlm sanderdlm force-pushed the breadcrumb-annotations branch from 4cf914d to 03b75af Compare November 26, 2021 12:08
@sanderdlm sanderdlm changed the title WIP: Breadcrumb with PHP8 attributes Breadcrumb with PHP8 attributes Nov 26, 2021
@sanderdlm sanderdlm merged commit 007d46c into master Nov 26, 2021
@tijsverkoyen tijsverkoyen deleted the breadcrumb-annotations branch December 8, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants