This project is archived and is in readonly mode.

#4649 ✓stale
Enrico Teotti

current_page? fails on namespaced controller with same base name

Reported by Enrico Teotti | May 19th, 2010 @ 07:42 AM

two controllers: Admin::EventsController and EventsController
seems like current_page? fails to spot the difference

.[29, 38] in /Users/enrico/code/rails/cos/app/helpers/admin/events_helper.rb
   29      block.call if current_user
   30    end
   31    
   32    def content_for_public_page(&block)
   33      debugger
=> 34      block.call if current_page?(:controller => 'events', :action => 'new')
   35    end
   36  end
/Users/enrico/code/rails/cos/app/helpers/admin/events_helper.rb:34
block.call if current_page?(:controller => 'events', :action => 'new')
(rdb:3) params[:controller]
"admin/events"
(rdb:3) current_page?(:controller => 'events', :action => 'new')
true
(rdb:3) current_page?(:controller => 'admin/events', :action => 'new')
true

Comments and changes to this ticket

  • Rizwan Reza

    Rizwan Reza May 21st, 2010 @ 12:46 AM

    • no changes were found...
  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer May 22nd, 2010 @ 10:49 AM

    Verified on master.

    EventsController#index only returns true for the first one:

    current_page?(:controller => 'events', :action => 'index')       # => true
    current_page?(:controller => 'admin/events', :action => 'index') # => false
    

    Admin::EventsController#index returns true for both:

    current_page?(:controller => 'events', :action => 'index')       # => true
    current_page?(:controller => 'admin/events', :action => 'index') # => true
    

    This definitely shouldn't be the case if you ask me.

  • Jonathan Greenberg

    Jonathan Greenberg May 23rd, 2010 @ 05:56 AM

    I was able to reproduce this as well.

    I started digging into the code and tests a bit but got stuck when I found that the current_page? tests stub out the controller#url_for method making it hard to write an accurate test.

    I am somehow guessing that this might be a tricky bug to solve in the end without rewriting the helper method because the helper currently is calling that controller method which probably is supposed to accommodate namespaced routes in this manner.

    This is my first attempt at cracking a rails bug so if anyone has any suggestions about how to approach solving this bug let me know.

  • Jonathan Greenberg

    Jonathan Greenberg May 24th, 2010 @ 05:26 AM

    So I have a possible simple fix but I am unsure of my implementation and the correct way to adapt the tests if my approach is acceptable. On master it could look like this:

      def current_page?(options)
        unless request
          raise "You cannot use helpers that need to determine the current " \
                "page unless your view context provides a Request object " \
                "in a #request method"
        end 
    
        return false if options[:controller] && controller.controller_path != options[:controller]
    
        url_string = CGI.unescapeHTML(url_for(options)) 
        ....
    

    This seems to return correct results using the circumstances described above when run with a server; however, the tests blow up. I get a little lost with how the current url_helper_test.rb mocks requests and what would be the best way to keep those test passing.

    I had hoped to submit this as a patch but did not feel comfortable without passing tests.

    I would appreciate the insight of anyone with more depth of experience with this part of rails and its tests.

  • Curtis Edmond

    Curtis Edmond May 27th, 2010 @ 07:01 PM

    The problem seems to stem from the "use_relative_controller!" method in the RouteSet class. It basically takes into account the url segments separated by "/" and appends them to the passed :controller if the passed :controller doesn't have them. Allowing us to say url_for(:controller => 'events', :action => 'index') to get "/admin/events" if the current controller we're in is "admin/events".

    I like Jonathan's proposed fix since it stays out of that use_relative_controller! method. I attached a patch for it which includes tests.

  • Lake

    Lake September 16th, 2010 @ 09:17 PM

    Hey Curtis,

    Your patch doesn't apply cleanly to master.

    I changed the patch to make it apply cleanly. I would post it here, but two tests are failing once the patch has been applied.

    I'm still looking into the failing tests.

    Thanks for your help.

  • Curtis Edmond

    Curtis Edmond September 16th, 2010 @ 09:32 PM

    Ok, thanks for the heads up. I'll try this against trunk and see what I can find.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:31 PM

    • State changed from “new” to “open”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:31 PM

    • State changed from “open” to “stale”
  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Pages