Rails 3.2.0 + 3.2.1 regression: :controller => "CampaignContent" in routes.rb under namespace converts badly #4720

Closed
NoICE opened this Issue Jan 27, 2012 · 8 comments

Comments

Projects
None yet
4 participants

NoICE commented Jan 27, 2012

CamelCased controller name in routes under namespace fails to convert properly. This works in Rails 3.1.x.

This is failing code:

http://.../admin/storage_files/1?width=400&height=300

  namespace :admin do
    resources :storage_files, :controller => "StorageFiles"
  end

uninitialized constant Admin::StoragefilesController

Note the small word "files", should say "Files".

    resources :storage_files, :controller => "StorageFiles"

works.

It is failing only under namespaced routes.
If I put :storage_files instead of "StorageFiles" here it works too (even under namespaced routes).

If the string syntax is deprecated, it should be clear :)

@tenderlove , I'm not sure if this is a bug in Rails or in Journey gem?
Looks like that string is downcased before converting to symbol or something?

Owner

tenderlove commented Jan 27, 2012

Not sure. I'll take a look!

@ghost ghost assigned tenderlove Jan 27, 2012

Contributor

kennyj commented Feb 3, 2012

@NoICE

/cc @tenderlove

I tackled this issue, and I found a problem in rails (and fixed).

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/route_set.rb#L56

controller_name is "admin/StorageFiles" in this case.
If it was camelized, a problem is happen.

> "admin/StorageFiles".camelize
 => "Admin::Storagefiles"

Please confirm the above commit.

I don't know that whether this issue should be solved in journey.

Contributor

kennyj commented Feb 5, 2012

I'm thinking that the cause of this problem is a strange behavior of camelize method.

ruby-1.9.3-p0 :002 > "StorageFiles".camelize
 => "StorageFiles"
ruby-1.9.3-p0 :003 > "admin/StorageFiles".camelize
 => "Admin::Storagefiles"

What do you think ?

This behavior is fixed by the above commit kennyj/rails@47e22e4.

NoICE commented Feb 5, 2012

Looks like that should fix it. I admit I was blaming camelize for this too. But it was working... when this "bug" was introduced? It is due to adapting action packs route_set to journey, or it is some independent change in camelize method alone?

Contributor

kennyj commented Feb 5, 2012

According to https://github.com/rails/rails/blame/master/activesupport/lib/active_support/inflector/methods.rb,
the lines that was fixed me were updated on 2011-06-09...
I suspect that the former is the case.

Owner

tenderlove commented Feb 6, 2012

@NoICE I think it's as you suspect. The problem is that I'm not sure what transformations rack-mount would apply to the routes. Journey does very little transformations on the data you feed it, and I'd like to keep it that way. Since that's the case, I think we should fix this on the rails side.

Owner

fxn commented Feb 9, 2012

The behavior of camelize is only documented for snake case strings, like file paths.

Camelize gives you a constant path out of a file path. The behavior in any other use case is undefined.

I don't know the details of the involved code, but I find suspicious that the fix involves touching camelize. If there's code using camelize on strings that are not snake case, that code should be revised instead.

kennyj added a commit to kennyj/rails that referenced this issue Feb 10, 2012

tenderlove added a commit that referenced this issue Feb 10, 2012

Merge pull request #4988 from kennyj/fix_4720-3
Fix GH #4720. Routing problem with nested namespace and already camelized controller option.

tenderlove added a commit that referenced this issue Feb 10, 2012

Merge pull request #4988 from kennyj/fix_4720-3
Fix GH #4720. Routing problem with nested namespace and already camelized controller option.
Contributor

kennyj commented Feb 11, 2012

I'm closing this issue, because the above PR is merged to 3-2-stable, master.
If you have any problem, please comment this issue :)

@kennyj kennyj closed this Feb 11, 2012

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