This project is archived and is in readonly mode.

#4556 ✓wontfix
Federico Brubacher

When using conditional layouts, actions fallback to parent layout

Reported by Federico Brubacher | May 8th, 2010 @ 09:34 PM | in 3.0.2

This patch tries to fix a subset of what is described here :
https://rails.lighthouseapp.com/projects/8994/tickets/3619-conditio...

The functionality added grabs the parent layout and uses that in case the conditional layout does not apply for that action.

As Fernando mentioned in the previous ticket a better way would be :

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

For now, the ability to fallback to a parent layout is good.

Comments and changes to this ticket

  • Federico Brubacher

    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.

  • rabble
  • Diego Algorta

    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.

  • Federico Brubacher

    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

    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.

  • Federico Brubacher

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

    Santiago, I merged both patches into one ask requested.

  • Santiago Pastorino

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

    • State changed from “open” to “verified”
  • Santiago Pastorino

    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.

  • Federico Brubacher

    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.

  • José Valim

    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
    
  • Federico Brubacher

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

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

  • José Valim

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

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

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

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

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>

Pages