Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RTM] Dynamically add robots.txt and favicon.ico per root page #717

Merged
merged 8 commits into from Sep 3, 2019

Conversation

@Toflar
Copy link
Member

commented Sep 3, 2019

This PR provides two new routes:

  • /favicon.ico which provides the icon you can now select in the root page settings (the fallback one).
  • /robots.txt which provides the robots.txt for 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 all sitemap.xml entries making it super convenient for users 馃帀

@Toflar Toflar requested a review from leofeyer Sep 3, 2019

@@ -16,6 +16,13 @@ services:
tags:
- { name: kernel.event_listener, event: kernel.terminate, method: onKernelTerminate }

contao.listener.robots_txt:

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 3, 2019

Member

We have agreed on using the class name as service key for all new services, so this needs to become Contao\CoreBundle\EventListener\RobotsTxtListener.

@Toflar Toflar force-pushed the feature/robots-txt-favicon branch from f7faea0 to 1b4c817 Sep 3, 2019

@leofeyer leofeyer added the feature label Sep 3, 2019

@leofeyer leofeyer added this to the 4.9 milestone Sep 3, 2019

CS

@Toflar Toflar force-pushed the feature/robots-txt-favicon branch from 1b4c817 to 750facb Sep 3, 2019

@Toflar Toflar changed the title Dynamically add robots.txt and favicon.ico per root page [RTM] Dynamically add robots.txt and favicon.ico per root page Sep 3, 2019

leofeyer added 3 commits Sep 3, 2019

@leofeyer leofeyer self-requested a review Sep 3, 2019

@leofeyer leofeyer merged commit 21919f6 into master Sep 3, 2019

5 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.1%) to 86.318%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Thank you @Toflar.

@leofeyer leofeyer deleted the feature/robots-txt-favicon branch Sep 3, 2019

}
/**
* @Route("/favicon.ico", name="contao_favicon")

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

does the route really need a route name? I mean noone will ever generate that?

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Author Member

Imho a route should have a name yes.

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

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';

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

It is deprecated in Symfony to use event names. The event class should be the name鈥

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Author Member

Yes but I cannot just change that atm as we do not require 4.4 yet.

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

why not? Just use event::class as the event name?

- '@contao.framework'
- '@?fos_http_cache.http.symfony_response_tagger'
tags:
- controller.service_arguments

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

The controller.service_arguments tag is not needed if you don't get services in your action methods.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 5, 2019

Collaborator

afaik it is still needed for dependency injection in the constructor.

This comment has been minimized.

Copy link
@aschempp

aschempp Sep 5, 2019

Contributor

No, what you mean is the service locator. But this controller does not extend from AbstractController so service locator is not used at all.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 5, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Sep 5, 2019

Collaborator

Hm, or not, just checked it again. Setting the controller to public: true is enough.

This comment has been minimized.

Copy link
@Toflar

Toflar Sep 5, 2019

Author Member

Guys, this PR is merged. If you want changes, create a new PR. Nothing will happen here.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 5, 2019

Member

I'm still following the discussion though.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Sep 7, 2019

Member

The __invoke() method needs the Request object. Will this still work without the tag?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.