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

Fix 404s by moving skip_cache conditions to server block #692

Merged
merged 1 commit into from
Dec 1, 2016
Merged

Conversation

fullyint
Copy link
Contributor

@fullyint fullyint commented Dec 1, 2016

Problem
Fixes a version of #436 where a system default 404 page is returned instead of the WordPress 404 (via index.html) . To reproduce, provision with cache enabled: true and do any of the following:

  • visit example.com/missing.php?p=1 (triggers the $query_string condition in code block below)
  • visit example.com/xmlrpc.php (triggers nginx_skip_cache_uri condition in code block below)
  • log in to wp-admin and then visit example.com/missing.php (triggers nginx_skip_cache_cookie in code block below)

Solution
The PR diff display overcomplicates the changes. This PR simply moves the following portion of the caching section up out of the location block context and into the server block context.

set $skip_cache 0;

if ($query_string != "") {
  set $skip_cache 1;
}

# Don't cache uris containing the following segments
if ($request_uri ~* "{{ item.value.cache.skip_cache_uri | default(nginx_skip_cache_uri) }}") {
  set $skip_cache 1;
}

# Don't use the cache if cookies includes the following
if ($http_cookie ~* "{{ item.value.cache.skip_cache_cookie | default(nginx_skip_cache_cookie) }}") {
  set $skip_cache 1;
}

Explanation
Why does it fix the problem to move these if conditions out of the location ~ \.php$ block? Because If Is Evil, particularly when inside a location block. Here is a summary of the affected Trellis location block:

location ~ \.php$ {
  try_files $uri =404;
  error_page 404 /index.php;

  ... if conditions in code block above ...
}

The excerpt above appears to exhibit the problem demonstrated in the example titled # try_files wont work due to if. If one of the if conditions is met, the try_files directive won't even be run. In our case, this means the WP index.php does not load and the system 404 is used instead. I am guessing this is related to the various descriptions here about how 'Nginx traps into the "if" inner block because its condition ... was met.'

Justification
Here are two examples recommending placing the if condition cache exceptions in the server block instead of the location block: easyengine.io and digitalocean.com, the latter even suggesting that these exceptions "must be used in the server{ } context."

@swalkinshaw
Copy link
Member

Good find :shipit:

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 this pull request may close these issues.

2 participants