Check Before Re-encoding #1
Conversation
|
I think this looks good to me. The 2nd commit might be problematic, but I don't know what the implications of both choices are, and I think I agree with your assessment. I'd be curious to hear what Sporkmonger has to say about it if you PR these upstream. |
|
Worth noting is that some URLs will still break for us (anything that is not UTF-8 will get force encoded), but I don't think that will happen in normal operation, and it might be okay for those URLs to fail anyway. |
calling force_encoding on them This prevents unnecessary mutation of these variables when already set and appropriately encoded. Necessary for our use case as we freeze our Addressable::URI objects to ensure they don't get changed when passed around
Calling force_encoding doesn't actually change anything about the string, it just changes what the string considers its encoding to be, allowing you to end up with strings with invalid encodings. There are some invocations in Addressable::URI where this seems intentional (followed by some g_sub'ing) but at least at the output stage it seems like the strings should be converted and the non-utf-8 bytes/characters removed
ACBullen
force-pushed
the
alexb/check_before_force_encoding
branch
from
ba9af52
to
bfc9a44
Compare
Since we call |
|
Ooh, I hadn't considered that, but that sounds excellent :) |
First commit is straightforward and harmless (to address: sporkmonger#254), second is potentially problematic, but I think represents what's actually wanted with all the
force_encodingcalls.