samvera / questioning_authority

Question your authorities
Other
54 stars 30 forks source link

Replace URI.escape with ERB::Util.url_encode #210

Closed elrayle closed 5 years ago

elrayle commented 5 years ago

This is the closest replacement to what already exists. There are multiple discussions on what is the correct way to do this. We may want to revisit later.

See… https://stackoverflow.com/questions/2824126/whats-the-difference-between-uri-escape-and-cgi-escape

This addresses deprecation warnings associated with URI.escape

elrayle commented 5 years ago

This is related to the work done in PR #196.

elrayle commented 5 years ago

Confirmed each modified authority by comparing results before and after for the following URLs...

elrayle commented 5 years ago

This replaces PR #196 which made a fix for a single authority. This makes a consistent fix across all uses of URI.escape.

NOTE: The use of ERB::Util.url_encoding was selected as the replacement for two reasons... 1) It was already in use in other places in the code, so use in this case provided consistency. 2) The original suggestion of CGI.escape produces a different URL using + instead of %20 for spaces and other similar differences in substitution. These differences may or may not have an impact on the results.

elrayle commented 5 years ago

@bbarberBPL As this replaces PR #196, can you review and comment on the changes in this PR?

elrayle commented 5 years ago

I tried before and after for...

Both worked in the before and after tests.

elrayle commented 5 years ago

@hackmastera You appear to be the original committer for assign_fast. There is a question about the clean_query_string method for assign_fast. The call to URI.escape is being replaced by ERB::Util.url_encode. The original code includes a gsub that appears to remove some characters. With ERB::Util.url_encode, these characters appear to be encoded properly. I am wondering two things...

hackartisan commented 5 years ago

@elrayle It looks like assign_fast is still in use in https://github.com/sciencehistory/chf-sufia/blob/master/app/assets/javascripts/edit_metadata.js; maybe @jrochkind or @eddierubeiz could identify some tests?

My vague memory is that the characters I removed were an issue on the API side, rather than a matter of encoding.