Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Federico Brubacher May 8th, 2010 @ 09:35 PM

  I will also try to back-port the fix for it to work in 2-3.stable.

• You flagged this item as spam.
  

  rabble May 9th, 2010 @ 03:58 AM

  +1

• You flagged this item as spam.
  

  Diego Algorta May 10th, 2010 @ 06:02 AM

  • Tag set to “patch”

  Applied patch over ce5827ea4791e8b8143919ecceb0231e36e8932e (master from today) and actionpack's tests all pass.

  +1

  I'm attaching an optional patch to apply over Federico's. This patch just makes a tiny documentation update to better reflect behavior change based on Federico's patch, added two more tests using :except instead of :only, and renamed private instance variable from @parent_layout to @_parent_layout.

  As I said, my patch does not change behavior at all so it's optional.

  • layout_fallbacks_to_parent_more_tests.diff 5.9 KB

• You flagged this item as spam.
  

  Federico Brubacher May 11th, 2010 @ 03:26 PM

  • Tag changed from “patch” to “3.0, patch”

  Thanks Diego for your patch, I like your changes because it seems that the notation fits better with the rest part of the code.

• 

  Santiago Pastorino May 11th, 2010 @ 06:54 PM

  • Milestone cleared.
  • Tag changed from “3.0, patch” to “3.0, actionpack, layout, patch”
  • State changed from “new” to “open”
  • Assigned user changed from “Jeremy Kemper” to “Santiago Pastorino”

  Great guys ;) but merge all in one commit.
  Also this kind of documentation changes should be done on lifo/docrails.

• You flagged this item as spam.
  

  Federico Brubacher May 11th, 2010 @ 07:35 PM

  Santiago, I merged both patches into one ask requested.

  • layout_fallback_to_parent_layout.diff 5.3 KB
• 

  Santiago Pastorino May 12th, 2010 @ 11:35 PM

  • State changed from “open” to “verified”

  I helped a bit to Federico here is the patch http://github.com/spastorino/rails/commit/cc6dccec6683147de44ad9534...

• 

  Santiago Pastorino May 14th, 2010 @ 04:46 PM

  Things are a bit tricky to get this work. Anyways i'm trying to solve it ;). Any help is welcomed.

• You flagged this item as spam.
  

  Federico Brubacher May 15th, 2010 @ 03:59 AM

  • Tag changed from “3.0, actionpack, layout, patch” to “3.0, actionpack, bugmash, layout, patch”
  • Assigned user changed from “Santiago Pastorino” to “José Valim”

  There has been some going back and forth with Santiago and some lessons learned about this.

  First of all, my main selling point for this patch is that the bug this fix tries to solve affects us in almost every project we have done. Most of the time we have to repeat code to accommodate for the actions that fall out of an :only condition.

  Then, Santiago proposed a big re-factor, which makes sense except that the current implementation is good enough. To give a little perspective the main trap we are all falling when doing a refactor is that :only and :except conditions are evaluated on per instance basis while the _layout method is being defined in a class method. So there is no way of knowing a priori if you have to fallback to parent layout or not.

  I have added a new patch that takes into account the case of Proc layouts that Santiago pointed out. Jose I know that you have been involved in this, please advice.

  • layout_fallback_to_parent_layout.diff 6.2 KB
• 

  José Valim May 15th, 2010 @ 07:46 AM

  • Tag changed from “3.0, actionpack, bugmash, layout, patch” to “3.0, actionpack, layout, patch”
  • State changed from “verified” to “incomplete”

  "which makes sense except that the current implementation is good enough".

  This is not true. Your patch, for instance, does not take into account the following scenario:

  class ApplicationController < ActionController::Base
  end
  
  class UsersController < ApplicationController
    layout "foo", :only => :index
  end
  

  When rendering an action new, it should fallback to the application controller and consequently try to render the application layout. But it won't happen. Even worse, if I specify a string as layout in the application and then change to a symbol, it will stop working with no apparent reason.

  I'm ok with adding this behavior as long as a solution which cover all cases is found and does not mess up with the code. In any case, I would rather use a symbol if you have a complex scenario like the one described on this ticket:

  class HasTwoOwnLayoutWithConditionsController < LayoutTest
    layout 'layout_wadus', :only => 'wadus'
    layout 'layout_churro', :only => 'churro'
  end
  

  Instead I would do:

  class HasTwoOwnLayoutWithConditionsController < LayoutTest
    layout :specify_layout
  
  protected
  
    def specify_layout
      if action_name == "wadus"
        "layout_wadus"
      elsif action_name == "churro"
        "layout_churro"
      else
        "application"
      end
    end
  end
  

• You flagged this item as spam.
  

  Federico Brubacher May 15th, 2010 @ 03:04 PM

  You are right, it is easier to use the workaround that you provided.

• 

  José Valim May 15th, 2010 @ 08:21 PM

  • State changed from “incomplete” to “wontfix”
• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “Low”

  [bulk edit]