Dynamically add robots.txt and favicon.ico per root page #717
Conversation
| @@ -16,6 +16,13 @@ services: | |||
| tags: | |||
| - { name: kernel.event_listener, event: kernel.terminate, method: onKernelTerminate } | |||
|
|
|||
| contao.listener.robots_txt: | |||
There was a problem hiding this comment.
We have agreed on using the class name as service key for all new services, so this needs to become Contao\CoreBundle\EventListener\RobotsTxtListener.
…allow them to be extended using an event
f7faea0 to
1b4c817
Compare
1b4c817 to
750facb
Compare
…egistry if there are options
|
Thank you @Toflar. |
| } | ||
|
|
||
| /** | ||
| * @Route("/favicon.ico", name="contao_favicon") |
There was a problem hiding this comment.
does the route really need a route name? I mean noone will ever generate that?
There was a problem hiding this comment.
Imho a route should have a name yes.
There was a problem hiding this comment.
https://symfony.com/doc/current/routing.html#generating-urls
If you don't set the route name explicitly with the name option, Symfony generates an automatic name based on the controller and action.
I think that would be totally fine in this case, noone ever will override the named route, as it is not used to generate a route anyway.
| * | ||
| * @see RobotsTxtEvent | ||
| */ | ||
| public const ROBOTS_TXT = 'contao.robots_txt'; |
There was a problem hiding this comment.
It is deprecated in Symfony to use event names. The event class should be the name…
There was a problem hiding this comment.
Yes but I cannot just change that atm as we do not require 4.4 yet.
There was a problem hiding this comment.
why not? Just use event::class as the event name?
| - '@contao.framework' | ||
| - '@?fos_http_cache.http.symfony_response_tagger' | ||
| tags: | ||
| - controller.service_arguments |
There was a problem hiding this comment.
The controller.service_arguments tag is not needed if you don't get services in your action methods.
There was a problem hiding this comment.
afaik it is still needed for dependency injection in the constructor.
There was a problem hiding this comment.
No, what you mean is the service locator. But this controller does not extend from AbstractController so service locator is not used at all.
There was a problem hiding this comment.
My custom controllers do not extend from AbstractController either and without the controller.service_arguments, no services would be injected into the constructor of the controller.
There was a problem hiding this comment.
Hm, or not, just checked it again. Setting the controller to public: true is enough.
There was a problem hiding this comment.
Guys, this PR is merged. If you want changes, create a new PR. Nothing will happen here.
There was a problem hiding this comment.
I'm still following the discussion though.
There was a problem hiding this comment.
The __invoke() method needs the Request object. Will this still work without the tag?
Contao 4.9 introduces extra palette for root pages with fallback, see contao/contao#717
As described [here] (contao/contao#717), since Contao 4.9 you need to add your fields to the new page type `rootfallback` in order to make them appear. Otherwise your fields will only appear, if the language fallback checkbox isn't selected.
This PR provides two new routes:
/favicon.icowhich provides the icon you can now select in the root page settings (the fallback one)./robots.txtwhich provides therobots.txtfor any root page (the fallback one). Users can manually enter content plus we have an event that allows us to extend it dynamically. The core uses it to dynamically add allsitemap.xmlentries making it super convenient for users 🎉