Set OutputStream.write_buffer's default buffer to binmode

[henkesn]

Fixes #438

[coveralls]

Coverage increased (+0.002%) to 95.769% when pulling 66324a7 on henkeinfo:binary-outstream-buffer into 8d91d00 on rubyzip:master.

[hainesr]

Thanks for this. Just one comment/suggestion from me.

[hainesr]

What if we pass an IO in here that hasn't been set to binmode? Do we still see the behaviour you're trying to avoid? This change only sets a newly created StringIO to binmode. Is this a problem?

How about:

Suggested change

	def write_buffer(io = ::StringIO.new('').binmode, encrypter = nil)
	def write_buffer(io = ::StringIO.new(''), encrypter = nil)
	io.binmode if io.respond_to?(:binmode)

Which would match behaviour in ::Zip::File.open_buffer?

[henkesn]

I found another location where the binmode would be set if supported. I think this is a cleaner solution:
https://github.com/henkeinfo/rubyzip/blob/5ce4e13ddd33443f01fa33c8208423b76d99fce3/lib/zip/file.rb#L151
Yes, you're right with your comment. I rewrote the write_buffer to behave like the source above and also fixed the same thing in yet another method.

[hainesr]

Yes I spotted that when I was looking through File and wondered why it was inconsistent. Thanks for the catch.

This looks good to me.

[hainesr]

This looks like a good patch to me.

@jdleesmiller I can't merge this myself, obvs, so does it look OK to you too?

[jdleesmiller]

Thanks for the patch! A few comments inline.

In my case this is UTF-8, which produces encoding errors in encoding sensitive use cases.

I'd be interested to see what the actual use case was that caused the error --- if we can get a test that failed (pre-fix) in the way that motivated you to open the issue, I think that will be more likely to catch future regressions.

There have been binmode problems in the past, notably #119 / #201, which resulted in this line:

rubyzip/lib/zip/file.rb

Lines 150 to 151 in 8d91d00

	# https://github.com/rubyzip/rubyzip/issues/119
	io.binmode if io.respond_to?(:binmode)

However, all the tests still pass even if that line is deleted, so I feel like we have already have some gaps in this area.

[henkesn]

Thank you for your feedback @jdleesmiller. My use case was an extension in my code where i filtered empty strings before storing to a database (String#presence). I already faced some issues with ActiveRecord in the past, when trying to save invalid UTF8 strings even for BLOB columns. But that's already some time ago and I neither know how to reproduce nor if that issues persist with newer versions.
Regarding the next line

rubyzip/lib/zip/file.rb

Lines 150 to 151 in 8d91d00

	# https://github.com/rubyzip/rubyzip/issues/119
	io.binmode if io.respond_to?(:binmode)

perhaps this line now is also superfluous because it internally calls OutputStream#write_buffer with my fix. But the yield call is done after this line and before the OutputStream#write_buffer, so removing this line potentially could break things.

[jdleesmiller]

OK, thanks for the updates. Looks good.

I think you're right that removing the existing binmode line would change what the user might get in the yield, so it's probably best to keep it.

[jdleesmiller]

This constant can now be removed, but I'll tidy it up post-merge.

[henkesn]

Thx!

[kitsunde]

This appears to have affected an upstream gem because of the encoding changing. weshatheleopard/rubyXL#377