addressable icon indicating copy to clipboard operation
addressable copied to clipboard

Check Encodings before calling force_encoding in Addressable::URI

Open ACBullen opened this issue 5 years ago • 5 comments

Reopening this pr with just the minimum changes to fix the issue blocking our use of the current version.

This is a quick fix to this issue, just adding in an extra check to avoid mutating strings when it isn't necessary

ACBullen avatar Apr 23 '19 01:04 ACBullen

Reopening this pr with just the minimum changes to fix the issue blocking our use of the current version.

I'm curious, what are the issue you are facing with the current version?

This is a quick fix to this issue, just adding in an extra check to avoid mutating strings when it isn't necessary

https://github.com/sporkmonger/addressable/issues/254 is about constants, but this PR only touches instance variables, so I don't think it is the same.

dentarg avatar Apr 23 '19 08:04 dentarg

The issue with constants is sort of an extension of the issue we have right now since we are normalizing the Addressable::URI object and then deep freezing. Even if the path or other part of the uri doesn't map to a constant, we are marking all of those strings as intended to be immutable. Since the force_encoding runs every time we try to reference parts of the parsed and normalized URI, this causes the same RuntimeError: can't modify frozen String to return.

Without this check, after the strings go through normalization, they are in their final state and do not mutate unless something tries to reassign one of the.

ACBullen avatar Apr 23 '19 18:04 ACBullen

Got it, thanks.

I think we need to add tests for this.

dentarg avatar Apr 23 '19 21:04 dentarg

@dentarg I have rebased and updated this PR. Let me know if theres anything else you want me to change.

baseballlover723 avatar Aug 31 '21 02:08 baseballlover723

@dentarg This is ready for review again

baseballlover723 avatar Sep 10 '21 21:09 baseballlover723