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

Redirect issue #2011

Closed
maxiloc opened this Issue Nov 23, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@maxiloc
Copy link

maxiloc commented Nov 23, 2016

Expected behavior and actual behavior

Redirect example from the doc is not working:

redirect "/my/old/path.html", to: "/my/new/path.html"

but

redirect "my/old/path.html", to: "/my/new/path.html"

is working fine.

I dig a bit into the issue.

It seems to come from here

@_lookup_by_destination_path[request_path]

request_path is my/old/path.html but the key in @ _lookup_by_destination_path is /my/old/path.html

Additional information

  • Ruby version: ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin16]
  • Middleman version: 4.1.10
@matiasgarciaisaia

This comment has been minimized.

Copy link
Contributor

matiasgarciaisaia commented Feb 11, 2017

Is there any legal use case for storing a non-normalized URL in both the @_lookup_by_destination_path and @_lookup_by_path hashes?

I don't see any user of that hash but the store itself. If we normalize the paths both when storing and accessing, then this bug should get fixed.

The only drawback is if there's any extension/customization that is [ab]using this feature to distinguish between both paths (/my/path and my/path), but I don't think we want to care about that.

If you're OK with the change, I'll do it.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 18, 2017

I can confirm that I'm still seeing this, on MM 4.2.1 on OSX 10.12 with Ruby 2.4.1.

@tdreyno

This comment has been minimized.

Copy link
Member

tdreyno commented Jul 21, 2017

@matiasgarciaisaia Seems like the @_ prefix is screaming at anyone looking that this is a "private" variable. Should be safe to normalize both. Mind putting a quick PR up and see if it passes the test suite?

@sgnl

This comment has been minimized.

Copy link

sgnl commented Dec 20, 2017

Just ran into this. Thanks for reporting the issue!

matiasgarciaisaia added a commit to matiasgarciaisaia/middleman that referenced this issue Jan 16, 2018

tdreyno added a commit that referenced this issue Jan 18, 2018

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