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

Sitemap dump directory absolute #225

Closed
yann-eugone opened this issue Jan 15, 2020 · 3 comments · Fixed by #234
Closed

Sitemap dump directory absolute #225

yann-eugone opened this issue Jan 15, 2020 · 3 comments · Fixed by #234

Comments

@yann-eugone
Copy link
Member

At the moment, the configured directory where the sitemap is dumped is :

  • web for Symfony < 4.0
  • public for Symfony >= 4.0

But this directory is not absolute.

Commands are usually triggered from the project root dir (where the composer.json is stored) so it makes no difference. But if you trigger your command from anywhere else, you may have dumped your sitemap files to an unwanted dir.

Should this bundle use absolute dir for default values ?

  • %kernel.root_dir%/../web for Symfony < 4.0
  • %kernel.project_dir%/public for Symfony >= 4.0
@ostrolucky
Copy link
Contributor

I agree with this proposal, default paths should not rely where is the command running from

@yamilovs
Copy link

yamilovs commented Jan 16, 2020

You cannot trust Kernel::VERSION constant to define public directory.
Symfony 3.4 works perfectly with the flex system and public directory too.

i think this parameter must be required to fill in configuration (without default value). And of course it must be absolute.

@yann-eugone
Copy link
Member Author

This is just a default value, we had to find a way to define it, and you can still change it if you want.

This is not easy to guess at that moment, and it will be painful to require that every project configure it even if not using anything exotic. So no, we won't go that way.

The public instead of web dir was introduced in symfony 4, then backported to 3.4 (for an easy update path I think).

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 a pull request may close this issue.

3 participants