fix respond_to without blocks always using all if one of the blocks is all #7823

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@grosser
Contributor

grosser commented Oct 2, 2012

would also be great to see this included in all minor releases of rails 3

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Oct 2, 2012

Contributor

FYI: hotfix

# https://github.com/rails/rails/pull/7823
if Rails::VERSION::MAJOR == 3
  ActionController::MimeResponds::Collector.class_eval do
    def custom_with_block_fix(mime_type, &block)
      custom_without_block_fix(mime_type, &(block || lambda{}))
    end
    alias_method_chain :custom, :block_fix
  end
else
  puts "You can remove #{__FILE__}:#{__LINE__} now!"
end
Contributor

grosser commented Oct 2, 2012

FYI: hotfix

# https://github.com/rails/rails/pull/7823
if Rails::VERSION::MAJOR == 3
  ActionController::MimeResponds::Collector.class_eval do
    def custom_with_block_fix(mime_type, &block)
      custom_without_block_fix(mime_type, &(block || lambda{}))
    end
    alias_method_chain :custom, :block_fix
  end
else
  puts "You can remove #{__FILE__}:#{__LINE__} now!"
end
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 3, 2012

Member

Are you trying to change the behavior of respond_to with a block argument or without the block?

Member

rafaelfranca commented Oct 3, 2012

Are you trying to change the behavior of respond_to with a block argument or without the block?

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Oct 3, 2012

Contributor

with block, the tests show what I'm up to :)

Contributor

grosser commented Oct 3, 2012

with block, the tests show what I'm up to :)

@frodsan

View changes

actionpack/CHANGELOG.md
@@ -1,5 +1,7 @@
## Rails 4.0.0 (unreleased) ##
+* Fix respond_to without format blocks always using all if one of the blocks is all. *Michael Grosser*

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor
`respond_to`
@frodsan

frodsan Oct 26, 2012

Contributor
`respond_to`
@frodsan

View changes

actionpack/test/controller/mime_responds_test.rb
+ def using_defaults_with_all
+ respond_to do |type|
+ type.html
+ type.all{ render :text => "ALL" }

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor

Please, use 1.9 hash syntax. Thanks.

@frodsan

frodsan Oct 26, 2012

Contributor

Please, use 1.9 hash syntax. Thanks.

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Oct 26, 2012

Contributor

Done and done

Contributor

grosser commented Oct 26, 2012

Done and done

@frodsan

View changes

actionpack/CHANGELOG.md
@@ -50,6 +50,8 @@
*Francesco Rodriguez*
+* Fix `respond_to` without format blocks always using all if one of the blocks is all. *Michael Grosser*

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Oct 26, 2012

Contributor

Entries must be on top.

@frodsan

frodsan Oct 26, 2012

Contributor

Entries must be on top.

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Oct 26, 2012

Contributor

I'll put it on top if someone wants to merge it, keeping it here will keep the merge-button green + I do not get a merge conflict on the next rebase.
If you can + want to merge it, let me know :)

@grosser

grosser Oct 26, 2012

Contributor

I'll put it on top if someone wants to merge it, keeping it here will keep the merge-button green + I do not get a merge conflict on the next rebase.
If you can + want to merge it, let me know :)

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Nov 28, 2012

Member

@rafaelfranca would you merge this if @grosser fixes the CHANGELOG entry?

Member

steveklabnik commented Nov 28, 2012

@rafaelfranca would you merge this if @grosser fixes the CHANGELOG entry?

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Jan 25, 2013

Contributor

@rafaelfranca merge ?

Contributor

grosser commented Jan 25, 2013

@rafaelfranca merge ?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 25, 2013

Member

For the CHANGELOG entry and issue description I could not understand the problem. The CHANGELOG says "without block", but the tests use a block.

Also it needs a rebase and the CHANGELOG should be on the top

Member

rafaelfranca commented Jan 25, 2013

For the CHANGELOG entry and issue description I could not understand the problem. The CHANGELOG says "without block", but the tests use a block.

Also it needs a rebase and the CHANGELOG should be on the top

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Jan 26, 2013

Contributor

@rafaelfranca rebased and put on top + hopefully clearer description :)

Contributor

grosser commented Jan 26, 2013

@rafaelfranca rebased and put on top + hopefully clearer description :)

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Jan 27, 2013

Contributor

Updated with simpler implementation

Contributor

grosser commented Jan 27, 2013

Updated with simpler implementation

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Jan 31, 2013

Contributor

@rafaelfranca looks good ? / can you merge it ?

Contributor

grosser commented Jan 31, 2013

@rafaelfranca looks good ? / can you merge it ?

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Feb 23, 2013

Contributor

@rafaelfranca
Rebased once more, please merge

Contributor

grosser commented Feb 23, 2013

@rafaelfranca
Rebased once more, please merge

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Feb 24, 2013

Contributor

@steveklabnik can you merge this ?

Contributor

grosser commented Feb 24, 2013

@steveklabnik can you merge this ?

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Feb 24, 2013

Member

I just took care of the merge conflict myself and merged. Sorry this took so long to address.

Member

steveklabnik commented Feb 24, 2013

I just took care of the merge conflict myself and merged. Sorry this took so long to address.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Feb 24, 2013

Member

Merged in 149e3cd

Member

steveklabnik commented Feb 24, 2013

Merged in 149e3cd

@grosser

This comment has been minimized.

Show comment Hide comment
@grosser

grosser Feb 24, 2013

Contributor

Thanks you so much! :D

Contributor

grosser commented Feb 24, 2013

Thanks you so much! :D

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