Fix encoding setting for non-binary IO-like objects in MockRequest#env_for#2227
Merged
Merged
Conversation
…v_for No core class implements both encoding and set_encoding. Classes implementing encoding: * Regexp * Symbol * String Classes implementing set_encoding: * File * ARGF.class * IO This code was added in 64610db. Previously, set_encoding was always called. 64610db actually broke encoding setting to binary for IO objects, but because there were no tests for the case, it wasn't caught. I don't see a point of warning of Regexp and Symbol use, and raising for objects that support encoding but not set_encoding doesn't make sense. I'm guessing the intention was to handle IO-like objects, and then internal_encoding could be checked, but it's simpler to just call set_encoding in that case, as that was the previous behavior. An alternative to this code would be reverting 64610db, but that is no longer backwards compatible with 3.1.0, which allows objects that do not respond to set_encoding. Found by coverage testing.
ioquatix
approved these changes
Jul 4, 2024
ioquatix
left a comment
Member
There was a problem hiding this comment.
LGTM - appreciate you fixing my mess.
Member
|
Is this a bug that needs to be back ported? |
Contributor
Author
|
Probably, as 3.0 used to set the encoding to binary and 3.1 doesn't. |
Member
|
Okay, so we should backport to 3.1 only? |
Contributor
Author
|
I think so. 3.0 didn't have this issue, it always called set_encoding. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No core class implements both encoding and set_encoding.
Classes implementing encoding:
Classes implementing set_encoding:
This code was added in 64610db. Previously, set_encoding was always called.
64610db actually broke encoding setting to binary for IO objects, but because there were no tests for the case, it wasn't caught.
I don't see a point of warning of Regexp and Symbol use, and raising for objects that support encoding but not set_encoding doesn't make sense. I'm guessing the intention was to handle IO-like objects, and then internal_encoding could be checked, but it's simpler to just call set_encoding in that case, as that was the previous behavior.
An alternative to this code would be reverting
64610db, but that is no longer backwards compatible with 3.1.0, which allows objects that do not respond to set_encoding.
Found by coverage testing.