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

map & priority cause params to return lists #696

Closed
veesahni opened this Issue Oct 8, 2011 · 18 comments

Comments

Projects
None yet
7 participants

veesahni commented Oct 8, 2011

Given ruby 1.9.2, padrino 0.10.3

In app.rb:

get :test, map: "/test/:id", priority: :low do
  params[:id].inspect
end

Request:

`````` /test/something````

Expected Result:
"something"

Actual Result:
["test", "something"]

Without the priority, it works as expected...

Owner

nesquena commented Oct 8, 2011

If it's possible can you write a failing test to capture this issue?

Owner

nesquena commented Oct 8, 2011

So just removing the priority fixes the issue consistently?

veesahni commented Oct 8, 2011

Yes, removing priority resolves the issue consistently. I first encountered this in 0.10.2 and tested w/ a vanilla 0.10.3 to confirm prior to posting

veesahni commented Oct 8, 2011

not having much luck adding a test that actually fails.. it works fine on the mock infrastructure..

Owner

nesquena commented Oct 8, 2011

That's unfortunate, means its probably a reloader issue. Just curious put disable :reload in your app and see if priority routes work again?

veesahni commented Oct 8, 2011

Disabling reload doesn't help.. on further testing, it seems like map doesn't matter either.. the following simpler config is also broken:
get "/test/:id", priority: :low do

@ghost ghost assigned joshbuddy Oct 12, 2011

Contributor

joshbuddy commented Oct 26, 2011

@nesquena weren't you gonna bring in the forking reloader?

Owner

nesquena commented Oct 26, 2011

It actually doesn't make a difference in this case:

Disabling reload doesn't help..

Apparently disabling reload doesn't help :(

Contributor

sumskyi commented Jan 6, 2012

below coode fixing the issue, but the problem somewhere deeper.

i.e., catch-all routes with normal priority break env['router.params'] for routes with lower priority

def default_routes!
  configure :development do
    get '*__sinatra__/:image.png', :priority => :low do
      content_type :png
      filename = File.dirname(__FILE__) + "/images/#{params[:image]}.png"
      send_file filename
    end
  end
end

this is definitely the same issue as #738

Contributor

basex commented Jan 31, 2012

This is the same bug as in here:
joshbuddy/http_router#22

Contributor

sumskyi commented Jan 31, 2012

There is no :priority in the http_router, this is Padrino issue

I noticed this bug hasn't had any motion on it for a while. I just wanted to mention that I noticed that the problem still exits in head but it only happens in development. Production seems to work fine.

Member

dariocravero commented Jan 4, 2013

Same thing happens if you do get :test, :with => :id, priority: :low do

Member

dariocravero commented Jan 4, 2013

It seems as if it actually is an issue of http_router - I'm trying with 0.11 and that route works fine, although a few others are broken. I've already asked @joshbuddy what the breaking changes are from 0.10.2 since I already saw a good few. Perhaps we should also upgrade a few other dependencies (sinatra 1.3.3, tilt 1.3.3, downgrade thor to 0.15.4 (see #976) and activesupport to 3.2.10, and that's on core only). Should I close this issue and open a new one to upgrade all of those deps?

Owner

nesquena commented Jan 4, 2013

Makes sense to me, we probably should upgrade all of those in the next release. And yes I do think that is related to http_router so we'll have to see if we can upgrade to 0.11 cleanly.

Member

dariocravero commented Jan 4, 2013

Ok, closing this issue then and pushed a branch to work on upgrading to 0.11 first of all.

Owner

nesquena commented Jan 4, 2013

Thanks

Nathan Esquenazi

On Friday, January 4, 2013 at 3:57 PM, Darío Javier Cravero wrote:

Ok, closing this issue then and pushed a branch to work on upgrading to 0.11 first of all.


Reply to this email directly or view it on GitHub (#696 (comment)).

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