Fix BufferedLogger constructor #1

Merged
merged 1 commit into from Apr 10, 2012

Conversation

Projects
None yet
2 participants
Contributor

needfeed commented Apr 10, 2012

For us, using rails 3.2.2, the patched BufferedLogger's constructor did not work.

It calls super, which perhaps is intended to be the constructor from rails' BufferedLogger, but which in fact is Object.new (as far as we could tell), which leads to @log not getting set.

When not explicitly constructing a logger, one might not notice this, as the default
rails logger would tend to get constructed before the patch is applied.

We weren't sure what the constructor in formatted_rails_logger is trying to do, but it didn't
seem to be referenced in the code or the README, and just removing it seemed to work for us.

Owner

jrochkind commented Apr 10, 2012

Odd, it works fine for me in Rails 3.2.2, wonder what our difference is.

I'll think about it and look at it; or I'll give up and just merge your change in if it seems to still work for me.

Can you give me (even psuedocode for) failing test for you, to better help me understand the problem? What doesn't work, it raises an exception, it does nothing, other?

Owner

jrochkind commented Apr 10, 2012

My idea was that a constructor with super will work either way, whether the superclass declares one or not, shouldn't possibly do any harm, and will be tolerant of superclass implementation changing to do something (or not). As BufferedLogger implementation seems to change with every patch release of rails sometimes.

Owner

jrochkind commented Apr 10, 2012

AH, I see the problem in my code (can't call super when you're monkey patching, duh, I over-rode the whole constructor).

The mystery is why this works anyway for me. Weird.

Your pull request removes the ability to set a .default_formatter on the class. That's probably not a problem, I guess.

jrochkind merged commit 7e600f0 into jrochkind:master Apr 10, 2012

Contributor

needfeed commented Apr 10, 2012

The reason this sometimes worked is "When not explicitly constructing a logger, one might not notice this, as the default
rails logger would tend to get constructed before the patch is applied." We were surprised by that too at first.

I could probably go back and figure out exactly which piece of our code broke but it was something like calling:

logger = ActiveSupport::TaggedLogging.new(
ActiveSupport::BufferedLogger.new("#{Rails.root}/log/facebook-#{Rails.env}.log")
)

after the rails initializers have run.

Owner

jrochkind commented Apr 11, 2012

Yeah, totally, thanks for finding this, sorry about that, I've merged
your patch in and released a new gem patch version. (Someone should
really submit a patch to Rails core to support a formatter, it's silly
that it doesn't, but I can't figure out what they're trying to do with
the rails logger and lack the energy to write and try to get a patch
accepted).

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