Backport to 4.2: Broadcast #silence on ActiveSupport::Logger #25397

Merged
merged 3 commits into from Jun 14, 2016

Projects

None yet

3 participants

@kmcphillips
Contributor

@rafaelfranca @sgrif @tenderlove

Backport of #25341 to 4-2-stable. Follow up to #25356. Includes the related bug @rafaelfranca fixed in 5e77a70 backported as well.

Problem

As explained in the original issue, the call to ActiveSupport::Logger#silence is not correctly delegated to broadcasted loggers. This was fixed by implementing the #silence method for logger broadcasting. This, along with rewriting the tests, was done on master.

Unfortunately, since on 4.2 in reporting.rb there are a number of Kernel patches, which includes aliasing the Kernel#capture method to silence:
https://github.com/rails/rails/blob/806e816bb972296f7419a7fbe67cb8a8c0eab6b5/activesupport/lib/active_support/core_ext/kernel/reporting.rb#L108

This means the respond_to? checks break because it duck-types to a logger which supports silence, but actually calls out to an unrelated method.

Solution

It's nasty, but I also validate that the silence method is not on Kernel. 😞

I'm open to further suggestions. There's not much prescient for this kind of check.

Up side is that this is only required in the backport as patch is deprecated and removed on master. If I'm going to refactor/reimplement logger broadcasting I'm not going to do it on the backport branch.

@kmcphillips
Contributor

Up side though is that with this fix and most recent sprockets-rails, the config.assets.quiet = true feature from #25351 can be used in 4.2 and the quiet_assets gem is no longer needed! 🎉

@kmcphillips
Contributor

Did a rails new and pointed the gemfile to:

gem 'rails', github: 'kmcphillips/rails', branch: '4-2-stable'
gem 'sprockets-rails', github: 'rails/sprockets-rails'

And confirmed this corrects the bug and supports the assets.quiet = true behaviour as expected! 🎉

$ cat config/environments/development.rb | grep quiet
  config.assets.quiet = false
$ bundle exec rails server
=> Booting WEBrick
=> Rails 4.2.6 application starting in development on http://localhost:3000
=> Run `rails server -h` for more startup options
=> Ctrl-C to shutdown server
[2016-06-14 11:10:02] INFO  WEBrick 1.3.1
[2016-06-14 11:10:02] INFO  ruby 2.3.1 (2016-04-26) [x86_64-darwin15]
[2016-06-14 11:10:02] INFO  WEBrick::HTTPServer#start: pid=18914 port=3000


Started GET "/" for ::1 at 2016-06-14 11:10:06 -0400
Processing by HelloController#index as HTML
  Rendered hello/index.html.erb within layouts/application (0.8ms)
Completed 200 OK in 197ms (Views: 196.9ms)


Started GET "/assets/jquery.self.js?body=1" for ::1 at 2016-06-14 11:10:06 -0400


Started GET "/assets/application.self.css?body=1" for ::1 at 2016-06-14 11:10:06 -0400


Started GET "/assets/jquery_ujs.self.js?body=1" for ::1 at 2016-06-14 11:10:06 -0400


Started GET "/assets/hello.self.css?body=1" for ::1 at 2016-06-14 11:10:06 -0400


Started GET "/assets/hello.self.js?body=1" for ::1 at 2016-06-14 11:10:06 -0400


Started GET "/assets/application.self.js?body=1" for ::1 at 2016-06-14 11:10:06 -0400
^C[2016-06-14 11:10:10] INFO  going to shutdown ...
[2016-06-14 11:10:10] INFO  WEBrick::HTTPServer#start done.
Exiting
$ cat config/environments/development.rb | grep quiet
  config.assets.quiet = true
$ bundle exec rails server
=> Booting WEBrick
=> Rails 4.2.6 application starting in development on http://localhost:3000
=> Run `rails server -h` for more startup options
=> Ctrl-C to shutdown server
[2016-06-14 11:10:24] INFO  WEBrick 1.3.1
[2016-06-14 11:10:24] INFO  ruby 2.3.1 (2016-04-26) [x86_64-darwin15]
[2016-06-14 11:10:24] INFO  WEBrick::HTTPServer#start: pid=18966 port=3000


Started GET "/" for ::1 at 2016-06-14 11:10:26 -0400
Processing by HelloController#index as HTML
  Rendered hello/index.html.erb within layouts/application (0.7ms)
Completed 200 OK in 202ms (Views: 201.8ms)
^C[2016-06-14 11:10:29] INFO  going to shutdown ...
[2016-06-14 11:10:29] INFO  WEBrick::HTTPServer#start done.
Exiting
@rafaelfranca rafaelfranca merged commit 3925282 into rails:4-2-stable Jun 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmcphillips kmcphillips added a commit to kmcphillips/rails that referenced this pull request Jun 23, 2016
@kmcphillips kmcphillips Update changelog for #25356 and #25397 e8cd808
@maclover7 maclover7 removed the backport label Jun 24, 2016
@MichaelSp MichaelSp added a commit to MichaelSp/rails that referenced this pull request Nov 2, 2016
@kmcphillips @MichaelSp kmcphillips + MichaelSp Update changelog for #25356 and #25397 753305b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment