Fix implied layout detection for anonymous controllers #5884

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@mudge

When creating an anonymous subclass of a controller,
_write_layout_method wasn't able to find a layout path even if a
parent controller had one that could be used. Now, the method will call
super if the current controller has no name.

In a related issue, we switch to using anonymous? rather than checking
only for name in order to be compatible with Ruby < 1.9 where anonymous
class names are set to the empty string rather than nil.

@mudge

It is worth noting that RSpec-Rails suffers from this bug on Ruby 1.8.7 as it provides a DSL for creating anonymous controllers (in order to test ApplicationController cleanly). We have separately submitted a workaround for this but it would be good to address the core issue in Rails itself.

@steveklabnik steveklabnik and 1 other commented on an outdated diff Sep 14, 2012
actionpack/lib/abstract_controller/layouts.rb
@@ -275,7 +275,11 @@ def _write_layout_method
remove_possible_method(:_layout)
prefixes = _implied_layout_name =~ /\blayouts/ ? [] : ["layouts"]
- name_clause = if name
+ name_clause = if anonymous?
+ <<-RUBY
+ super
+ RUBY
@steveklabnik
steveklabnik Sep 14, 2012

Why do this rather than just "super"?

@mudge
mudge Nov 17, 2012

To be consistent with the style used in the line below but I agree that it's unnecessary in this simple case.

@steveklabnik
Ruby on Rails member

Hey @mudge !

It's been two months and I haven't heard from you. In addition, fixes should be filed against master, not 3-2-stable, and will be backported as necessary. If you still feel this is an issue, please re-file against master. Thanks.

@mudge

Apologies for the delay in responding (presumably lost in the quagmire of my GitHub notifications). Thanks for taking the time to look at this; unfortunately, master has diverged significantly from 3.2 in this area (with layouts being revamped in 4.0) which is why we filed this against 3-2-stable.

We'll see if it is still an issue in master and, if so, file a new request. That said, is there a separate process for fixes to < 4.0 functionality like this where back porting might not be straightforward?

@steveklabnik steveklabnik reopened this Nov 17, 2012
@steveklabnik
Ruby on Rails member

No worries! happens to the best of us.

You'd end up re-doing this, so I've re-opened it. That said, fixing it to just use "s would be nice.

And if you can check against master, that'd be really useful, too.

James Hunt & Paul Mucur Fix implied layout detection for anonymous controllers.
When creating an anonymous subclass of a controller,
_write_layout_method wasn't able to find a layout path even if a
parent controller had one that could be used. Now, the method will call
super if the current controller has no name.

In a related issue, we switch to using anonymous? rather than checking
only for name in order to be compatible with Ruby < 1.9 where anonymous
class names are set to the empty string rather than nil.
3f352ac
@mudge

I've amended super to use "s. Looking at master, it looks like this is already fixed:

# actionpack/lib/abstract_controller/layouts.rb:286-294
name_clause = if name
  <<-RUBY
    lookup_context.find_all("#{_implied_layout_name}", #{prefixes.inspect}).first || super
  RUBY
else
  <<-RUBY
    super
  RUBY
end

Note this might still be an issue on Ruby < 1.9 (due to name being "" and therefore truthy) which is why we used anonymous? (which is defined as name.blank? in activesupport/lib/active_support/core_ext/module/anonymous.rb in 3.2) instead.

@steveklabnik
Ruby on Rails member

Awesome. Let's see if someone can't review. Thanks.

@lukaszx0
Ruby on Rails member

Hey @steveklabnik do feel like merging it? Who should we ask for review?

@steveklabnik
Ruby on Rails member

I am not sure, to be honest.

@lukaszx0
Ruby on Rails member

@rafaelfranca @carlosantoniodasilva hey guys? Could you help here? Thanks.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Dec 3, 2013
actionpack/test/abstract/layouts_test.rb
@@ -168,6 +170,15 @@ def show
end
end
+ class WithImpliedLayout < Base
+ def index
+ render :template => ActionView::Template::Text.new("Hello!")
@carlosantoniodasilva
carlosantoniodasilva Dec 3, 2013

Can you use Ruby 1.9 style?

@carlosantoniodasilva
Ruby on Rails member

Actually, we are not accepting fixes on 3-2 anymore, would you like to resend it to master? If not, let me know and I'll check if I can port it. Thanks.

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