[RFC][WIP] Add an event to allow easy decoration of routes

[lemoinem]

Hello,

Here is a first tentative of implementation of the event I described in #98

The point of this event is to easily allow for UrlDecoration.

For now, I seem to have found an implementation, but the tests are still missing.

I tried to add the event dispatch in AbstractGenerator::addUrl, but I'm missing several informations, mainly the route's name, and possibly the option set. An option would be to add these informations in UrlConcrete/Url. It seems cleaner and easier to test.

What do you think? I would be more than happy for feedback

• Write tests
• Update documentation

[yann-eugone]

Hello,

Thank you for suggesting such evolution.
The idea seems great, but I already see a drawback with this idea : performance.
As the event will be triggered once per URL, in a website with many referenced URLs, this will have a cost.

As I said, I like the idea, but I think that we should discuss a bit about.
The first missing things I can see are :

• a way to disable (disabled as default ?) the event dispatching
• a way to only trigger events for certain URLs (using a dynamic event name, for example)

Then, I see an other (minor) problem with the idea of the SitemapRouteListenerInterface. We have an other issue (see #131) on which we decided to deprecate the SitemapListenerInterface.
I will open a pull request this week. We think that describing which events are described, and what are the purposes of these events is enough (and that way we can use event priorities).

[lemoinem]

@yann-eugone Thanks for the feedback!

Well... I'm not sure it would be so bad performance-wise... Without any event listener registered, dispatching an event is nothing more than a method call and an array lookup. I don't think it would it would impact performance that much, especially compared to the pre-processing required for the options, which is run for each route (independently of whether it should be used to generate an entry in the sitemap or not).

I understand your point about the deprecated interface. I will remove the listener and the compiler pass.

However, the unit test for the event is rather intricate and involves a lot of Mocks (and I meand A LOT)... Which is usually a sign of bad design...

What would you think about adding the route name and the options to the Url interface (although I understand it would be a BC break, so maybe we could find a way to implement it without it and plan for its inclusion in the next API break)...
With this modification, we could take care of the event dispatch in the addUrl method of the AbstractGenerator and greatly simplify the complexity.

[yann-eugone]

I'm not that confident with performances. If you have no listeners, well I think I will be ok. But if you have at least 1 listener, it will be called for each node, and in my opinion, this is to much, really.

To talk about design, well I think that we should find something more like "decorated url collector" that trigger the event :

use Presta\SitemapBundle\Sitemap\Url\Url;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;

class EventUrlContainer implements UrlContainerInterface
{
    private $dispatcher;
    private $urlContainer;

    public function __construct(EventDispatcherInterface $dispatcher, UrlContainerInterface $urlContainer)
    {
        $this->dispatcher = $dispatcher;
        $this->urlContainer = $urlContainer;
    }

    public function addUrl(Url $url, $section)
    {
        $event = new ReplaceUrlEvent($url);
        $this->dispatcher->dispatch(ReplaceUrlEvent::REPLACE_URL, $event);

        if ($event->isReplaced()) {
            $url = $event->getReplaced();
        }

        $this->urlContainer->addUrl($url, $section);
    }
}

use Symfony\Component\EventDispatcher\Event;

class ReplaceUrlEvent extends Event
{
    const REPLACE_URL = 'presta_sitemap.replace_event';

    private $url;
    private $replaced;

    public function __construct(Url $url)
    {
        $this->url = $url;
    }

    public function getUrl()
    {
        return $this->url;
    }

    public function getReplaced()
    {
        return $this->replaced;
    }

    public function isReplaced()
    {
        return $this->replaced instanceof Url;
    }

    public function replace(Url $url)
    {
        $this->replaced = $url;
    }
}

Then, we could just replace the definitions of services with this decorator.

What do you think ?

[lemoinem]

The `EventUrlContainer` sounds good so we can easily skip the event. I'll try to provide a semantic configuration so it's easy to enable. Regarding the `ReplaceUrlEvent` though, I'm not sure how this will go if we have more than one listener. They will need to be implemented with `$url = $event->isReplaced() ? $event->getReplaced() : $event->getUrl();` each time... I think it's easier to just have a get/set Url and finish the EventUrlContainer with `if (null !== $event->getUrl()) parent::addUrl($event->getUrl(), $section);`. That way, it's easy to provide several listeners providing various decorations (e.g. one for multilingual, one for images, etc.). It's also easy to remove Urls from the sitemap through listeners. The setter could enforce that we only submit `null` or Url Decorator. Plus we still need to have the route name and options in the event... The best way would be to put them in the Url(Interface)... WDYT?

[yann-eugone]

I just wrote this code in 2 minutes, so yes we should enhance it, I agree.

Still, I'm not sure to understand what you are trying to achieve with this mechanism we are talking about.
Can you peek me a simple example, so I understand your needs ?

[lemoinem]

Sure,

My goal is twofold :

1°) Decoration of the Urls using an event rather than having to rewrite the Listener on sitemap event.

So I could have a listener on the replace_event to generate alternative locales URLs, and another one to generate images, and so on:

/**
 * @Tag("kernel.listener", event="presta.route", method="decorate")
 */
public class AddL10nUrls
{
  protected $locales;
  protected $router;

  public function __constructor($locales, $router)
  {
    $this->locales = $locales;
    $this->router = $router;
  }

  public function decorate(RouteEvent $event)
  {
    $url = new GoogleMultilangUrlDecorator($event->getUrl());

    foreach ($this->locales as $locale) {
      $url->addLink($this->router->generate($event->getRouteName(), array_merge($event->getRouteParams(), array('_locale' => $locale)));
    }

    $event->setUrl($url);
  }
}

And so on...

2°) Second point, is more like a nice to have: Creating listeners that could provide a second layer of filtering over which routes should be included and which shouldn't (It is really to fix a problem I have with JMSI18nRouting, which duplicate all my routes, so I can remove the duplication).

The point is really to make the code for Url Decoration easier to write and re-use.

Does that help?

[yann-eugone]

@lemoinem can we talk about this once more ? I'm trying to clean things.

[lemoinem]

@yann-eugone Sure, I'd need to read the documentation and code again to remember what it was all about, but I'm definitely open and happy to answer any question and discuss the use-case further.

I could probably start by rebasing my PR so I'm up-to-date and remove any conflicts.

[yann-eugone]

As an example, I added some code for url decoration in my demo project.
Please have a look to the commits :

• Image decoration : fd85a4a
• Video decoration : fcfc14d

[yann-eugone]

Hello @lemoinem !
If you still want to work on this, please reopen the merge request when ready.