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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SeoBundle] Added event to populate robots.txt #2937

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

waaghals
Copy link
Contributor

@waaghals waaghals commented Sep 9, 2021

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets n/a

I've used events to generate a robots.txt. This makes for an easy extension point for dynamically modifing the robots.txt.
I'm having doubts about the working of Kunstmaan\SeoBundle\EventListener\FileRobotsTxtListener.
It no longer uses the request's basepath. But I do not understand how it could have worked previously, as the controller would never be called when the file exists in te public directory.

@ProfessorKuma ProfessorKuma added this to the 5.9.0 milestone Sep 9, 2021
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @, your PR needs some changes

  • This PR seems to need a milestone of a minor release.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @, your PR passed all our requirements.

Thank you for contributing!

@acrobat acrobat assigned acrobat and unassigned acrobat Sep 11, 2021
@acrobat acrobat self-requested a review September 11, 2021 18:59
@acrobat acrobat modified the milestones: 5.9.0, 5.10.0 Oct 10, 2021
Copy link
Member

@acrobat acrobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason every action/check for robots file/content is moved to an event listener? I would think it would enough to keep the current behaviour and just dispatch an event at the end that would allow you to hook into the robots content and modify it or event fully replace it.

Is there a specific need for this setup?

@acrobat acrobat removed this from the 5.10.0 milestone Oct 12, 2021
@waaghals
Copy link
Contributor Author

My reasoning was to allow for easier changing of the logic.
For example, lets say I would like the content from the file robots.txt to be appended to the existing content from the entity
instead of acting as a fallback.

By making smaller listeners this could easily be done by only replacing FileRobotsTxtListener. Otherwise the new implementation would also need to implement the existing logic of the Entity fetching etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants