Issue 9048063: infrastructure: Redirects for the new website (Closed)
|
Created:
Dec. 19, 2012, 10:27 a.m. by Felix Dahlke Modified:
Nov. 8, 2013, 8:04 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThe new websites will use different URLs for the browser-specific sites. I've changed that for our development environment, I'll make exactly the same changes on the production environment when we put the new site up.
Note that I have also fixed the /installation rewrite, it wouldn't have matched /zh_TW/installation like this.
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #MessagesTotal messages: 9
http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... File modules/adblockplusorg/files/adblockplus.org (right): http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:25: if ($http_user_agent !~ \bChrome/\d+) { Note that this causes Opera users to see the Firefox site. It was like that before, I decided not to fix it while I'm at it. http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:35: rewrite ^/(\w\w(_\w\w)?)/installation$ /$1/ permanent; This one I did fix while I was at it.
http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... File modules/adblockplusorg/files/adblockplus.org (right): http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:25: if ($http_user_agent !~ \bChrome/\d+) { Actually, it wasn't broken before - a rewrite statement has the effect that further rules for the original location are no longer processed. Also, the current version of the server config no longer has the pointless "not Chrome" check, it simply defaults to Firefox if no other rules match. So please fix this. Put |set $browser "firefox";| before the rules to have a default value, override it with "chrome" or "opera" as necessary. Also, please add the detection rule for Android here - this one was also added to the server config after you created your copy. http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:34: rewrite ^/(\w\w(_\w\w)?)/(firefox|chrome|opera) /$1/#$3 permanent; Also android. http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:35: rewrite ^/(\w\w(_\w\w)?)/installation$ /$1/ permanent; Actually, I think that zh_CN was only added after the dedicated installation page was gone - so there are no old links of this form to rewrite. Still, we can have it like this for consistency.
Uploaded a new patch set addressing the comments. I know we're probably going to try a different approach here, but I'd still like to have this one as a patch set. http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... File modules/adblockplusorg/files/adblockplus.org (right): http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:25: if ($http_user_agent !~ \bChrome/\d+) { On 2012/12/19 11:16:39, Wladimir Palant wrote: > Actually, it wasn't broken before - a rewrite statement has the effect that > further rules for the original location are no longer processed. Right, didn't think there. I presumed this was just broken with the dev environment so I didn't dig much further, just looked wrong. > Also, please add the detection rule for Android here - this one was also added > to the server config after you created your copy. Done. http://codereview.adblockplus.org/9048063/diff/1/modules/adblockplusorg/files... modules/adblockplusorg/files/adblockplus.org:34: rewrite ^/(\w\w(_\w\w)?)/(firefox|chrome|opera) /$1/#$3 permanent; On 2012/12/19 11:16:39, Wladimir Palant wrote: > Also android. Done.
LGTM even though we likely won't go with that approach.
Here's a completely different approach :) As discussed, I'm doing it like this now: 1. Redirect / to /firefox, /chrome etc., based on the user agent 2. Set the browser class based on the URL 3. Rewrite to /, so that the final URL for the user is /firefox etc.
http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... File modules/adblockplusorg/files/adblockplus.org (right): http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:59: rewrite (.*) /static/$1 last; Do we need the special case and code duplication for the index page? Why not redirect to $static_uri with the browser parameter every time? http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:77: sub_filter ' id="content"' ' id="content" class="$arg_browser"'; Note that this will affect all pages, not just the homepage - they will get an empty class attribute. http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:109: sub_filter ' id="content"' ' id="content" class="$arg_browser"'; Nit: you probably should leave sub_filter in the same place as before.
Uploaded a new patch set addressing the remaining issues. http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... File modules/adblockplusorg/files/adblockplus.org (right): http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:59: rewrite (.*) /static/$1 last; On 2012/12/19 16:41:56, Wladimir Palant wrote: > Do we need the special case and code duplication for the index page? Why not > redirect to $static_uri with the browser parameter every time? You're right, changed that part. I was a bit worried that this would make some query strings invalid, but I suppose we're not going to have any query parameters at this point. http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:77: sub_filter ' id="content"' ' id="content" class="$arg_browser"'; On 2012/12/19 16:41:56, Wladimir Palant wrote: > Note that this will affect all pages, not just the homepage - they will get an > empty class attribute. I've tried to put that sub_filter inside an if, but that doesn't seem to be allowed :( But that empty class is still not very nice, I've made some changes to avoid that. http://codereview.adblockplus.org/9048063/diff/7001/modules/adblockplusorg/fi... modules/adblockplusorg/files/adblockplus.org:109: sub_filter ' id="content"' ' id="content" class="$arg_browser"'; On 2012/12/19 16:41:56, Wladimir Palant wrote: > Nit: you probably should leave sub_filter in the same place as before. Done. Thought it made more sense down there but you're right, better to have a useful diff.
LGTM 
|
