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

Bug :: NGINX Compat. :: `$_GET['q']` #561

Closed
jaswrks opened this issue Aug 30, 2015 · 5 comments
Closed

Bug :: NGINX Compat. :: `$_GET['q']` #561

jaswrks opened this issue Aug 30, 2015 · 5 comments
Labels
bug
Milestone

Comments

@jaswrks
Copy link

@jaswrks jaswrks commented Aug 30, 2015

Overview

Referencing: http://wiki.nginx.org/Pitfalls#Front_Controller_Pattern_based_packages

try_files $uri $uri/ /index.php?q=$uri&$args;

This is a popular configuration for Nginx servers.

The $_GET['q'] variable being passed to WordPress & ZenCache though; it can prevent an entire site from being cached. This is because ZenCache, by default, will not cache URLs that contain a query string. With this Nginx config, all requests include the ?q= variable.

Steps to Reproduce

  • Fire up Nginx

  • Use a configuration like this.

    try_files $uri $uri/ /index.php?q=$uri&$args;
    
  • Enable pretty permalinks in WordPress.

  • Visit any page on the site.

Expected Behavior

That fancy permalinks would be cached.

Observed Behavior

<!-- ZenCache is NOT caching this page, because `$_GET` contains query string data. The current configuration says NOT to cache GET requests with a query string. -->

Possible Solution

ZenCache already has a check for special query vars. We could add q to the list of special query vars whenever we can detect that a site is running Nginx. That way the $_GET['q'] and $_SERVER['QUERY_STRING'] environment vars will not prevent caching.

@jaswrks jaswrks added the bug label Aug 30, 2015
@jaswrks
Copy link
Author

@jaswrks jaswrks commented Aug 30, 2015

Short-Term Fix

Change:

try_files $uri $uri/ /index.php?q=$uri&$args;

To:

try_files $uri $uri/ /index.php?$args;
jaswrks pushed a commit to wpsharks/comet-cache-pro that referenced this issue Aug 31, 2015
@raamdev raamdev added this to the Next Release (Pro) milestone Aug 31, 2015
@jaswrks
Copy link
Author

@jaswrks jaswrks commented Aug 31, 2015

I should note that the ?q=$uri is not there for WordPress. WordPress doesn't need this. It's there (i.e., suggested by Nginx) for cross-compatibility with Drupal and Joomla also. I'm not sure which of those relies on this, but WordPress itself does not. Thus, the short-term fix I posted could also be a long-term fix if you're only running WordPress.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Sep 3, 2015

@raamdev raamdev closed this Sep 3, 2015
@raamdev
Copy link
Contributor

@raamdev raamdev commented Sep 3, 2015

Next Pro Release Changelog:

  • Bug Fix: Fixed an issue related to a popular NGINX server configuration (try_files $uri $uri/ /index.php?q=$uri&$args;) that was preventing the entire site from being cached. ZenCache disables caching by default for all requests that include a query string (see Dashboard → ZenCache → Plugin Options → GET Requests) and this particular NGINX configuration passes all requests to WordPress with a ?q= query variable, which was resulting in ZenCache disabling caching on all pages. This release implements better detection for NGINX and works around this scenario. Props @jaswsinc. See Issue #561.
@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 2, 2015

ZenCache Pro v151002 has been released and includes changes from this GitHub Issue.

See the ZenCache Pro v151002 release announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#561).

@wpsharks wpsharks locked and limited conversation to collaborators Oct 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants