Rails 3.2.5 breaks Redmine 2.0.1 #6585

Closed
darix opened this Issue Jun 1, 2012 · 15 comments

Comments

Projects
None yet
5 participants

darix commented Jun 1, 2012

Started GET "/issues/43" for 195.135.221.2 at Fri Jun 01 15:12:44 +0200 2012
Processing by IssuesController#show as HTML
  Parameters: {"id"=>"43"}
  Rendered issues/_action_menu.html.erb (6.2ms)
  Rendered issues/show.html.erb within layouts/base (12.5ms)
Completed 500 Internal Server Error in 325ms

ActionController::RoutingError (No route matches {:controller=>"issues", :id=>nil, :action=>"show"}):
  app/views/issues/show.html.erb:17:in `_app_views_issues_show_html_erb__902631651_70179852102700'
  app/controllers/issues_controller.rb:118:in `show'
  app/controllers/issues_controller.rb:115:in `show'

Downgrading to 3.2.3 fixes the issue again.

Owner

pixeltrix commented Jun 1, 2012

Did your version of Journey get upgraded to 1.0.2 during the upgrade process? It looks similar to this issue: rails/journey#20

darix commented Jun 1, 2012

My Gemfile.lock for 3.2.3 also has journey 1.0.3 listed. Same as my Gemfile.lock with 3.2.5.

Just did a diff between the 2 Gemfile.locks and they only differ in rails components. the rest is the same.

Owner

pixeltrix commented Jun 1, 2012

It turns out it's a combination of the above issue, #6196 and the commit 66e338a which fixes #6196. If you look at this segment from show.html:

  <% if @prev_issue_id || @next_issue_id %>
    <div class="next-prev-links contextual">
      <%= link_to_if @prev_issue_id,
                     "\xc2\xab #{l(:label_previous)}",
                     issue_path(@prev_issue_id),
                     :title => "##{@prev_issue_id}" %> |
      <% if @issue_position && @issue_count %>
        <span class="position"><%= l(:label_item_position, :position => @issue_position, :count => @issue_count) %></span> |
      <% end %>
      <%= link_to_if @next_issue_id,
                     "#{l(:label_next)} \xc2\xbb",
                     issue_path(@next_issue_id),
                     :title => "##{@next_issue_id}" %>
    </div>
  <% end %>

On the first and last tickets one of @prev_issue_id and @next_issue_id is nil but issue_path is called with the nil value triggering the Journey issue. The reason it didn't fail in 3.2.3 is because of #6196 - it discarded the nil positional arg and pulled the :id from the recall path parameters. I'm sorry, you'll have to fix the template - this works for me:

    <% if @prev_issue_id %>
      <%= link_to "\xc2\xab #{l(:label_previous)}",
                  issue_path(@prev_issue_id),
                  :title => "##{@prev_issue_id}" %> |
    <% end %>
    <% if @issue_position && @issue_count %>
      <span class="position"><%= l(:label_item_position, :position => @issue_position, :count => @issue_count) %></span> |
    <% end %>
    <% if @next_issue_id %>
      <%= link_to "#{l(:label_next)} \xc2\xbb",
                  issue_path(@next_issue_id),
                  :title => "##{@next_issue_id}" %>
    <% end %>

Hope this helps - you may want to create a ticket wherever Redmine tracks issues (I'm guessing they self-host)

@pixeltrix pixeltrix closed this Jun 1, 2012

Contributor

dgm commented Jun 5, 2012

Ouch, this breaks any occurance of link_to_if .... something_path(possibly_nil). It used to be a useful idiom to have a link to a resource if the resource existed or a "disabled" link if not. Broke one of my own apps too.

Owner

pixeltrix commented Jun 5, 2012

@dgm unfortunately there's not a lot you can do - the url helper is called in the params before the condition inside link_to_if is checked. Rather than using the url helper you can use a hash or just pass the model if the polymorphic route is the right route, e.g:

<%= link_to_if @next_issue_id, "Next", { :controller => 'issues', :action => 'show', :id => @next_issue_id }, :title => "##{@next_issue_id}" %>
<%= link_to_if @next_issue, "Next", @next_issue, :title => "##{@next_issue.to_param}" %>

The latter is nicer, but you need to fetch the model and not just the id. Personally I'd wrap the logic up inside a helper or decorator to simplify the view - there's too much going on inside that Redmine template.

Contributor

dgm commented Jun 5, 2012

Yeah, I can see that. An unfortunate side effect that worked before. But I agree, that path function should not be executed if there is no id, so I'll have to change a few places. Just saying, redmine probably isn't the only place this happened. :)

Owner

pixeltrix commented Jun 5, 2012

@dgm it worked before but the urls generated were wrong - they either pointed to the wrong id or to the index action, so we couldn't not fix the bug.

Member

NZKoz commented Jun 5, 2012

@pixeltrix that change in #6196 was a regression, and intentional / supported functionality. I think we should restore the previous behaviour for the next 3.2.x release and perhaps leave it off in master.

Owner

rafaelfranca commented Jun 5, 2012

Reopening this.

@rafaelfranca rafaelfranca reopened this Jun 5, 2012

@ghost ghost assigned pixeltrix Jun 5, 2012

Owner

pixeltrix commented Jun 6, 2012

@NZKoz recall parameters still work as intended - all #6169 fixes is an edge case with a single nil parameter or new record passed to a url helper. For example given these routes:

resources :brands do
  resources :products
end

this is what happens before the #6169 fix:

# No request
>> app.brand_path(nil)
ActionController::RoutingError

# GET /brands
>> app.brand_path(nil)
ActionController::RoutingError

# GET /brands/1
>> app.brand_path(nil)
=> "/brands/1"

# GET /brands/1
>> app.brand_path(:id => nil)
ActionController::RoutingError

# GET /brands/1/products/2
>> app.brand_path(nil)
=> "/brands/2"

# GET /brands/1/products
>> app.brand_product_path(1, nil)
ActionController::RoutingError

# GET /brands/1/products/2
>> app.brand_product_path(1, nil)
ActionController::RoutingError

As you can see most of the time it generates a routing error - the only instances where it generates a route is when there's a single nil argument and then it generates an incorrect url unless it's to the current brand. It doesn't pull :id from the recall when you have at least one truthy argument - the positional argument wins when all the hashes are merged in url_for. However leaving out the positional arg still works:

# GET /brands/1/products/2
>> app.brand_product_path(1)
=> "/brands/1/products/2"

The Redmine template example is just wrong - in 3.2.3 the previous/next links would be to the current issue and not to the previous/next issues, it's just that the link_to_if then discards the link. If the template had a nested url helper and the last argument was nil then it would get the same result in 3.2.3 and 3.2.5.

TL;DR all the fix in #6196 does is make the treatment of nil positional arguments consistent - if you think that nil positional arguments should pull from the recall parameters then that's a different argument - no pun intended. :-)

These errors have been coming to light after @tenderlove changed the behavior of nil/false arguments in Journey 1.0.2 - previously the nil argument would've been converted to empty string, whereas now it raises an error. If you revert #6169 then you'll probably want to revert the Journey fix as well, however I don't think I've see an example yet where a app no longer generates a correct url - generally we're just raising an error now instead of generating an incorrect url.

Owner

pixeltrix commented Jun 6, 2012

@NZKoz just be clear, replacing nil positional arguments from the recall parameters should only go in master as it's a change from existing behavior.

Also if you pass a new record then it will get pass the args.any? check but then raise an error since to_param on new record is nil. So in 3.2.3, brand_path(Brand.new) generates an error but brand_path(Brand.new.id) would pull the id from the recall parameters.

Member

NZKoz commented Jun 7, 2012

@pixeltrix I see, I missed the part where link_to_if was obviously going to never append the link if the value was nil / set to something incorrect so the nonsense-urls generated in this case were never seen.

So the journey fix was for #4587, which does indeed seem somewhat crazy. Did this functionality work in 3.0 or earlier?

@darix seems like in this case it's a regression, but we just changed what was 'undefined' behaviour to raise an error, I'm a little less strident on reverting this change unless there's another usecase that breaks?

Contributor

dgm commented Jun 7, 2012

Even though it slightly irks me that it feels like a regression, in this case the behavior was a throw-away. But if the original bug was to somehow happen in a place that isn't a throw-away, a more serious error could happen. I think it is better to leave it fixed, rather then allow undefined behavior.

This also serves as a warning to the dangers of "link_to_if"... it's not truly an if statement that avoids execution. I think it's a subtle trap to expect it to work the same as link_to(...) if condition?

Owner

pixeltrix commented Jun 7, 2012

So the journey fix was for #4587, which does indeed seem somewhat crazy. Did this functionality work in 3.0 or earlier?

No, pond_duck_path(Duck.new) fails with a routing error in 3.0 as well.

@pixeltrix pixeltrix closed this Jun 7, 2012

Owner

pixeltrix commented Jun 7, 2012

Okay, closing this again as consensus seems to be leave it as is.

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