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

Root should not be inside location block #142

Closed
schkovich opened this issue Sep 16, 2013 · 8 comments
Closed

Root should not be inside location block #142

schkovich opened this issue Sep 16, 2013 · 8 comments

Comments

@schkovich
Copy link

Bad practice is currently enforced: fail('Cannot create a location reference without a www_root [...]).

Instead option to add www_root should be removed and error raised if user tries to add root inside location block.

Nginx Pitfalls: Root inside location block

BAD:

server {
    server_name www.domain.com;
    location / {
      root /var/www/nginx-default/;
      [...]
    }
}

GOOD:

server {
  server_name www.domain.com;
  root /var/www/nginx-default/;
  location / {
    [...]
  }

Root moved to vhost_header

@abraham1901
Copy link
Contributor

👍

@hackzilla
Copy link

👍

@schkovich
Copy link
Author

solution #576

@khaefeli
Copy link

👍

but I think #576 is a workaround. the module should do it correctly by default, to avoid the wrong behavior, if the DevOP is careless / inexperienced

@schkovich
Copy link
Author

I agree. However see @jfryman comment. I do understand his noble democratic desire to allow people doing stupidity but I still think that by default root should be in server block. Case that user from whatever reasons still wants to place root in location block give her/him option to do it.

@khaefeli
Copy link

@schkovich I totally agree.

@jfryman
Copy link
Contributor

jfryman commented Apr 24, 2015

@schkovich I think in that comment I said...

I'd rather provide sane defaults and let a user make silly decisions on their own.

I do think this falls under a sane default. Throw up a PR, I'll review/merge. I was hoping the original author of that PR would rebase/resubmit, but it didn't happen.

@wyardley
Copy link
Collaborator

wyardley commented Oct 7, 2016

I have included a way to suppress logs in #888
We do have some use cases that require setting different roots per location, but agree that the default behavior could be improved.
I'm going to close this one, just because it hasn't had any activity in ages, but, as they say, pull requests (with tests) will be reviewed and gratefully accepted if this is still an issue.

@wyardley wyardley closed this as completed Oct 7, 2016
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

No branches or pull requests

6 participants