New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provides standard layout lookup behavior for method and proc cases #8458

Merged
merged 1 commit into from Mar 27, 2013

Conversation

Projects
None yet
6 participants
@chrisnicola
Contributor

chrisnicola commented Dec 8, 2012

When setting the layout either by referencing a method with a symbol or supplying a Proc there is no way to fall back to the default lookup behavior if desired instead returning nil will render no template.

This patch changes this behavior so that returning nil from either a method or a proc will result in the default template lookup.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 8, 2012

Member

Not completely sure it's a safe change, but anyway it'll need tests and a changelog to go on. Thanks.

Member

carlosantoniodasilva commented Dec 8, 2012

Not completely sure it's a safe change, but anyway it'll need tests and a changelog to go on. Thanks.

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Dec 8, 2012

Contributor

@carlosantoniodasilva thanks. Yeah the upside is it feels more consistent with the conventional behavior, the downside is that there is probably code out there that uses nil instead of false to skip rendering a layout.

Contributor

chrisnicola commented Dec 8, 2012

@carlosantoniodasilva thanks. Yeah the upside is it feels more consistent with the conventional behavior, the downside is that there is probably code out there that uses nil instead of false to skip rendering a layout.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Dec 12, 2012

Member

I'm not a big fan of returning false and returning a falsey value having different outcomes. Can we make the behavior more explicit? Something like returning a specific constant, or symbol to set off the desired behavior? Also, still no tests.

Member

schneems commented Dec 12, 2012

I'm not a big fan of returning false and returning a falsey value having different outcomes. Can we make the behavior more explicit? Something like returning a specific constant, or symbol to set off the desired behavior? Also, still no tests.

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Dec 12, 2012

Contributor

@schneems I don't disagree that being more explicit would be better but this is already the documented behaviour for "layout"

layout false    # renders no layout

layout nil      # keeps the default behaviour
Contributor

chrisnicola commented Dec 12, 2012

@schneems I don't disagree that being more explicit would be better but this is already the documented behaviour for "layout"

layout false    # renders no layout

layout nil      # keeps the default behaviour
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Dec 12, 2012

Member

gotcha, I didn't realize that was already documented behavior, thanks for the clarification (https://github.com/lucisferre/rails/blob/3489ba48d3be7a58207c8167a78c29e6828df1ba/actionpack/lib/abstract_controller/layouts.rb#L251).

Member

schneems commented Dec 12, 2012

gotcha, I didn't realize that was already documented behavior, thanks for the clarification (https://github.com/lucisferre/rails/blob/3489ba48d3be7a58207c8167a78c29e6828df1ba/actionpack/lib/abstract_controller/layouts.rb#L251).

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Dec 12, 2012

Contributor

On the subject of tests, I'm not actually sure where the documented default behavior is currently tested. Was planning on using that as a model for testing this behavior but there doesn't seem to be a test for it.

Contributor

chrisnicola commented Dec 12, 2012

On the subject of tests, I'm not actually sure where the documented default behavior is currently tested. Was planning on using that as a model for testing this behavior but there doesn't seem to be a test for it.

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Jan 6, 2013

Contributor

@schneems added the tests, sorry for the delay, holidays, yadda yadda 🎄

I feel I should mention that the test I added to abstract/layouts_test.rb passes regardless of the changes, but it felt like it was missing since there was an equivalent test already for the symbol and layout nil cases. There doesn't appear to be an obvious way to test that the default lookup behavior happens in those tests.

The controller/layout_test.rb tests are what actually verifies the changes and checks that the default layout lookup behavior is used when a symbol or proc returns nil.

Contributor

chrisnicola commented Jan 6, 2013

@schneems added the tests, sorry for the delay, holidays, yadda yadda 🎄

I feel I should mention that the test I added to abstract/layouts_test.rb passes regardless of the changes, but it felt like it was missing since there was an equivalent test already for the symbol and layout nil cases. There doesn't appear to be an obvious way to test that the default lookup behavior happens in those tests.

The controller/layout_test.rb tests are what actually verifies the changes and checks that the default layout lookup behavior is used when a symbol or proc returns nil.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 6, 2013

Member

The tests look straightforward, I made some style comments. Address those and add a changelog plz. Thanks for working on this.

Member

schneems commented Jan 6, 2013

The tests look straightforward, I made some style comments. Address those and add a changelog plz. Thanks for working on this.

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 15, 2013

Member

Hey @schneems and @carlosantoniodasilva, @LucisFerre has pushed commits accordingly to your suggestions and requests. Is it ready? Can we move this forward?

Thanks.

Member

lukaszx0 commented Mar 15, 2013

Hey @schneems and @carlosantoniodasilva, @LucisFerre has pushed commits accordingly to your suggestions and requests. Is it ready? Can we move this forward?

Thanks.

@schneems

View changes

Show outdated Hide outdated actionpack/test/controller/layout_test.rb
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Mar 16, 2013

Member

Switch over the hash to 1.9 syntax and then needs a merge and a rebase.

Member

schneems commented Mar 16, 2013

Switch over the hash to 1.9 syntax and then needs a merge and a rebase.

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Mar 25, 2013

Contributor

@schneems actually that file is littered with the 1.8 syntax which is why I used it, I just assumed based on what I saw in the file. I'm happy to change it over though if that is how things are being done now (I prefer it as well actually). I assume I should change the entire file though not just my lines? Is that fine?

Contributor

chrisnicola commented Mar 25, 2013

@schneems actually that file is littered with the 1.8 syntax which is why I used it, I just assumed based on what I saw in the file. I'm happy to change it over though if that is how things are being done now (I prefer it as well actually). I assume I should change the entire file though not just my lines? Is that fine?

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Mar 27, 2013

Contributor

@schneems sorry a merge and a rebase? You mean merge master into this or do you want me to rebase onto master?

Contributor

chrisnicola commented Mar 27, 2013

@schneems sorry a merge and a rebase? You mean merge master into this or do you want me to rebase onto master?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 27, 2013

Member

He means it needs 'squashed' and a rebase.

Member

steveklabnik commented Mar 27, 2013

He means it needs 'squashed' and a rebase.

Provides standard layout lookup behavior for method and proc cases
When setting the layout either by referencing a method or supplying a
Proc there is no way to fall back to the default lookup behavior if
desired. This patch allows fallback to the layout lookup behavior when
returning nil from the proc or method.
@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Mar 27, 2013

Contributor

@steveklabnik @schneems squashed and rebased now.

Contributor

chrisnicola commented Mar 27, 2013

@steveklabnik @schneems squashed and rebased now.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 27, 2013

Member

Says it can't be merged still :/

Member

steveklabnik commented Mar 27, 2013

Says it can't be merged still :/

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Mar 27, 2013

Member

@steveklabnik it is a changelog conflict, can you manually merge locally?

$ git pull https://github.com/lucisferre/rails.git improve-layout-override-fallback-behavior

This changelog conflict problem is an enormous waste of time :( wish we could have people commit to a .changelog file and on merge have a bot strip it out and prepend it to the CHANGELOG file. Or something like that... cc @rafaelfranca

Member

schneems commented Mar 27, 2013

@steveklabnik it is a changelog conflict, can you manually merge locally?

$ git pull https://github.com/lucisferre/rails.git improve-layout-override-fallback-behavior

This changelog conflict problem is an enormous waste of time :( wish we could have people commit to a .changelog file and on merge have a bot strip it out and prepend it to the CHANGELOG file. Or something like that... cc @rafaelfranca

rafaelfranca added a commit that referenced this pull request Mar 27, 2013

Merge pull request #8458 from lucisferre/improve-layout-override-fall…
…back-behavior

Provides standard layout lookup behavior for method and proc cases

Conflicts:
	actionpack/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit ef27bba into rails:master Mar 27, 2013

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Mar 27, 2013

Member

Yeah, this CHANGELOG problem is very annoying. Merged.

Member

rafaelfranca commented Mar 27, 2013

Yeah, this CHANGELOG problem is very annoying. Merged.

@chrisnicola chrisnicola deleted the chrisnicola:improve-layout-override-fallback-behavior branch Mar 27, 2013

carlosantoniodasilva added a commit that referenced this pull request Mar 30, 2013

Improve AP changelog entry about layout method with nil return
Add a note about getting the "no layout" behavior by returning "false"
to make it easier for people that might need to change their code.

Related to #8458. [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment