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

Add reverse_www filter to fix www_redirect #486

Merged
merged 1 commit into from
Feb 18, 2016

Conversation

fullyint
Copy link
Contributor

@QWp6t pointed out that when SSL is enabled, wordpress-site.conf.j2 will fail to forward http://www.example.com => https://example.com. The current template produces the conf below.

server {
  listen 80;
  server_name example.com;
  return 301 https://$host$request_uri;
}

server {
  listen 443 ssl http2;
  server_name www.example.com;
  return 301 $scheme://example.com$request_uri;
}

This PR enables the first server block to also match www, taking the first step of forwarding http://www.example.com => https://www.example.com. Then the second server block will pick it up and strip the www.

I created and implemented a custom reverse_www filter to make the templating simpler.

match = re.match(r'www\.(.+)', host)
if match:
return match.group(1)
return 'www.' + host
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what idiomatic Python is but seems like this should be under the else condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you. I see this in PEP 08:

# No
def bar(x):
    if x < 0:
        return
    return math.sqrt(x)

# Yes
def foo(x):
    if x >= 0:
        return math.sqrt(x)
    else:
        return None

Another improvement is that although re.match() will...

Return None if the string does not match the pattern

I should probably change the evaluation to if match is not None: because PEP 8 also says this:

beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

@fullyint
Copy link
Contributor Author

Additional commit

  • code style better resembles python and Ansible conventions
  • filter now accommodates one or more host names
  • filter provides informative error message if input value is not string or list of strings

Pros of new filter

  • makes the wordpress-site.conf.j2 template much more readable, sparing most users from seeing elaborate jinja filter sequences
  • initiates the use of custom filters, bringing their power to Trellis

Cons of new filter

  • adds far more lines of code overall than if we stick to existing filters
  • probably wouldn't be used anywhere but in this instance, so it is not a benefit to DRY code

Below is an alternative to this PR's reverse_www filter. It may be the better solution for the www redirects issue at hand.

 {% if item.value.ssl is defined and item.value.ssl.enabled | default(False) %}
 server {
   listen 80;
-  server_name {{ item.value.site_hosts | join(' ') }};
+  server_name {{ item.value.site_hosts | join(' ') }}{% for host in item.value.site_hosts %} {{ (host[:3] == 'www.') | ternary(host[4:], 'www.' + host) }}{% endfor %};
   return 301 https://$host$request_uri;
 }
 {% endif %}

 {% for host in item.value.site_hosts if item.value.www_redirect | default(true) %}
 server {
   {% if item.value.ssl is defined and item.value.ssl.enabled | default(false) -%}
     listen 443 ssl http2;
   {% else -%}
     listen 80;
   {% endif -%}

-  server_name {{ host | match('^www\\.(.*)') | ternary(host | regex_replace('^www\\.(.*)', '\\1'), 'www.' + host ) }};
+  server_name {{ (host[:3] == 'www.') | ternary(host[4:], 'www.' + host) }};
   return 301 $scheme://{{ host }}$request_uri;
 }
 {% endfor %}

@fullyint fullyint force-pushed the www-filter branch 2 times, most recently from 2a0272a to f703caa Compare February 18, 2016 00:42
retlehs added a commit that referenced this pull request Feb 18, 2016
Add reverse_www filter to fix www_redirect
@retlehs retlehs merged commit 9b07707 into roots:master Feb 18, 2016
@fullyint fullyint deleted the www-filter branch February 18, 2016 19:53
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.

3 participants