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

Subdomain elevator same as first domain elevator #339

Closed
typeoneerror opened this issue Jul 27, 2016 · 2 comments
Closed

Subdomain elevator same as first domain elevator #339

typeoneerror opened this issue Jul 27, 2016 · 2 comments

Comments

@typeoneerror
Copy link
Contributor

typeoneerror commented Jul 27, 2016

I noticed that the Subdomain elevator first parses the subdomains and then returns the first one in the list in https://github.com/influitive/apartment/blob/development/lib/apartment/elevators/subdomain.rb#L37.

Examples:

  • "bar.foo.com" would be ["bar"] and "bar" would be the subdomain.
  • "baz.bar.foo.com" would be ["baz", "bar"] and "baz" would be the subdomain.

Base on the FirstSubdomain elevator, this seems like exactly the same since the FirstSubdomain just splits on "." and returns the first item. Seeing that we always get first in Subdomain, should link 37 not be something line:

def subdomain(host)
  subdomains(host).join('.')
end

This way, following our example

  • "bar.foo.com" would be ["bar"] and "bar" would be the subdomain/schema name
  • "baz.bar.foo.com" would be ["baz", "bar"] and "baz.bar" would be the subdomain/schema name

FirstDomain should continue to work because in both cases, we'd still get "bar" and "baz".

Not sure if I'm mis-reading Subdomain implementation.

@bradrobertson
Copy link
Contributor

I didn't actually write the FirstSubdomain Elevator and to my knowledge it never did anything different than the subdomain one. While you're probably correct that the subdomain elevator should match on x number of subdomains depending on your configuration, it definitely doesn't currently do that and your change would very much be a breaking change for a lot of people, so pretty tough to pass off without a lot of grumbling.

These elevators are mainly for convenience. We've almost always found that people need to customize them to suit their own needs, hence the GenericElevator which let's you define your own parser block.

@typeoneerror
Copy link
Contributor Author

That makes sense. I'll just implement in my generic elevator for our own project.

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

No branches or pull requests

2 participants