Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ActionDispatch: remove trailing slash when root_url is called #7729

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

bogdan commented Sep 22, 2012

root_url is always returning a trailing slash.
That is actually no way to remove it.

So patched #url_for to remove trailing slash if present.

Member

steveklabnik commented Sep 22, 2012

Why is this needed?

Contributor

bogdan commented Sep 22, 2012

Kinda cosmetic change. Some people in our team think that without trailing slash it looks better. Especially when URL is not hidden under a[href] but shown.

Owner

rafaelfranca commented Sep 22, 2012

I don't know if this is a good change. I always expect the / in the root_url. This may break the people test suite.

cc @spastorino @jeremy

Contributor

homakov commented Sep 22, 2012

according to URL canonicalization , trailing slash is not necessary. as a cosmetic change +1

Contributor

acapilleri commented Sep 22, 2012

it's a cosmetic changes but maybe it could have impact with some static links that assume "/" at the end of urls

Member

steveklabnik commented Sep 22, 2012

Since it's an option, it shouldn't break anything.

I'm just not sure it's worth adding code for something so trivial.

Contributor

bogdan commented Sep 23, 2012

The main reason why I propose this change is because this behavior is currently impossible to reach now.
I have to strip out trailing slash manually: root_url.sub(/\/$/, '')

Member

steveklabnik commented Sep 23, 2012

You could trivially write your own helper:

  def root_url
    super.sub(/\$/, '')
  end

no?

Owner

jeremy commented Oct 26, 2012

-1 on special-casing the root url like this. Might want to experiment with changing root_path to an empty string. This needs deeper consideration in any case. Check out the interaction between SCRIPT_NAME and PATH_INFO as well.

@jeremy jeremy closed this Oct 26, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment