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

Generalize https filter take 2 #131

Merged
merged 2 commits into from
Jul 7, 2014
Merged

Conversation

simeonwillbanks
Copy link
Contributor

Thanks for your contribution @rymohr!

@jch this is ready for review.

jch added a commit that referenced this pull request Jul 7, 2014
@jch jch merged commit 341e721 into master Jul 7, 2014
@jch
Copy link
Contributor

jch commented Jul 7, 2014

Thanks all!

@jch jch deleted the generalize-https-filter-take-2 branch July 7, 2014 16:57
@bkeepers
Copy link
Contributor

bkeepers commented Sep 5, 2014

I just tried to upgrade github to the latest release and this is causing issues. This change requires that we set base_url to "http://github.com", but that breaks some of our other URL generation which assumes base_url is the HTTPS url.

@jch
Copy link
Contributor

jch commented Sep 5, 2014

@bkeepers we could choose another variable name to avoid this conflict. What do you think of http_url? Where is the conflict coming from?

@bkeepers
Copy link
Contributor

bkeepers commented Sep 8, 2014

we could choose another variable name to avoid this conflict. What do you think of http_url?

I was thinking the same. We could fall back to base_url too: context[:http_url] || context[:base_url].

Where is the conflict coming from?

We use base_url in our MentionFilter and CommitMentionFilter for generating links to user profiles and commits.

@jch
Copy link
Contributor

jch commented Sep 8, 2014

@bkeepers 👍 to a fallback for compatibility. I think it'd be good to document context variables that are shared between variables to prevent conflicts also. I can start on that after we fix this issue.

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.

De-github https filter
4 participants