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

fastcgi: sync fastcgi_params with upstream fastcgi.conf #500

Closed

Conversation

vincentbernat
Copy link
Contributor

This is a risky change for SCRIPT_FILENAME but it handles more
situation than fastcgi_params. Upstream still ships the previous
fastcgi_params file.

Fixes #499

As explained in #499, the change is disruptive. I am doing the pull request for comments only. Only SCRIPT_FILENAME is changed (and HTTPS a bit).

This is a risky change for `SCRIPT_FILENAME` but it handles more
situation than fastcgi_params. Upstream still ships the previous
fastcgi_params file.

Fixes: voxpupuli#499
@jfryman
Copy link
Contributor

jfryman commented Dec 30, 2014

I do not use these parameters at all... so I would very much appreciate some insight as where the risk lies. It seems pretty straightforward to me in order to support the defaults that are being presented upstream... and if that really is the only change then I'm 👍

Thoughts appreciated.

@3flex
Copy link
Contributor

3flex commented Dec 30, 2014

Upstream provides two files, one is fastcgi.conf (newer) the other fastcgi_params (older). This module currently includes an out of date fastcgi_params.

The difference between upstream's fastcgi.conf and fastcgi_params is explained here: http://blog.martinfjordvald.com/2013/04/nginx-config-history-fastcgi_params-versus-fastcgi-conf/

So nobody's config is wrecked, I'd suggest:

  • Adding a new file resource in nginx::config which is responsible for creating fastcgi.conf. We can always create this file since it only has effect if it's included in the config somewhere, which it wouldn't be by default. The contents of that file would be upstream's fastcgi.conf
  • Updating fastcgi_params file in the module to match upstream (just requires updating HTTPS variable with if_not_empty parameter). Note this requires nginx 1.1.11. If that would be too disruptive then don't do this.
  • Move creation of fastcgi_params to nginx::config and always create it. Don't create that file in location.pp or vhost.pp

This is backwards compatible (module still uses fastcgi_params by default) and allows users to specify the newer fastcgi.conf if they wish.

@3flex 3flex added the enhancement New feature or request label Apr 16, 2015
@3flex 3flex mentioned this pull request May 8, 2015
@wyardley
Copy link
Collaborator

wyardley commented Oct 6, 2016

@3flex: what are your thoughts about this one?

@3flex
Copy link
Contributor

3flex commented Oct 9, 2016

I think it's a bigger question than whether to merge or not merge... I think we need consensus on #862 before proceeding.

@wyardley
Copy link
Collaborator

wyardley commented Dec 5, 2016

Decision on #862 has basically been made, is someone willing to rebase this PR? I know it's ancient now...

@wyardley
Copy link
Collaborator

wyardley commented Feb 2, 2017

I like this idea, but no response in the comments, so I'm going to close this for now; would welcome a new or updated PR that does the same thing.

@wyardley wyardley closed this Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with fastcgi_params
4 participants