samvera / questioning_authority

Question your authorities
Other
54 stars 30 forks source link

Fix deprecation #196

Closed bbarberBPL closed 5 years ago

bbarberBPL commented 5 years ago

I'm currently refactoring one of our gems and qa v2.2. However when running the tests I kept getting

/home/bbarber/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/qa-3.0.0/lib/qa/authorities/loc/generic_authority.rb:27:in `build_query_u rl': warning: URI.escape is obsolete /home/bbarber/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/qa-3.0.0/lib/qa/authorities/loc/generic_authority.rb:28:in `build_query_url' : warning: URI.escape is obsolete /home/bbarber/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/qa-3.0.0/lib/qa/authorities/loc/generic_authority.rb:27:in `build_query_url' : warning: URI.escape is obsolete /home/bbarber/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/qa-3.0.0/lib/qa/authorities/loc/generic_authority.rb:28:in `build_query_url' : warning: URI.escape is obsolete

I bumped the version up in my gemspec to the latest release and found i was getting the same warnings in my tests

Doing further research online I found on the ruby docs here That as of 2.4.0 this method has been deprecated and is now obsolete. Since I am running on 2.4.5 I first was able to make a monkey patch in the gem. But my colleague advised me to inform the Samvera community about this. I also removed a duplicate declaration of simplecov that was listed in both the gemspec and the Gemfile

I was able to set up engine cart and run the specs and all of them passed. I created a method in the web_service_base module that will maintain backwards compatibility for earlier ruby versions but for anyone using 2.4.0 and above it will use CGI.escape instead of URI.escape.

elrayle commented 5 years ago

@bbarberBPL Thanks for submitting this. I am in the process of working on a major refactor for QA which will likely delay the release of this. How critical is it for you to get this in place? Can it wait a while for the full refactor to be completed?

bbarberBPL commented 5 years ago

@elrayle Yeah that's no problem for us I can use the monkey patch I have in place for the gem thats dependent on this for now.

elrayle commented 5 years ago

@bbarberBPL Got a question for you before this PR is merged. I did some exploring to see how others are resolving this deprecation since it appears in other places as well. This is the most concise explanation I found for the various encoding options. https://medium.com/@rainerborene/7-ways-to-encode-urls-in-ruby-c15078a7d6bc

The thing of note is that URI::escape and CGI::escape do not produce the same effect. I've used ERB::encode in other places.

URI::escape('hello world')          # 'hello%20world'
CGI::escape('hello world')          # 'hello+world'
ERB::Util.url_encode('hello world') # 'hello%20world'

In this example, URI::escape and ERB::Util.url_encode produce the same result, but I'm not sure that is always the case. It looks like CGI::escape is commonly used to encode query strings with the preference for + over %20 for blank spaces. That seems consistent with the changes you made. Are you aware of the difference in behavior between CGI::escape and URI::escape?

elrayle commented 5 years ago

I spoke with several folks in Slack. There was general agreement that ERB::Util.url_encode produces the same behavior as the deprecated URI::escape. For that reason, the deprecation will be resolved using ERB as the replacement. There are a number of places beyond the one addressed in this PR.

elrayle commented 5 years ago

Interestingly, the URI.escape method itself has this recommendation...

"This method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case."

elrayle commented 5 years ago

More info...

>> require 'addressable/uri'
=> false
>> Addressable::URI.escape 'http://google.com/foo?bar=at#anchor&title=My Blog & Your Blog'
=> "http://google.com/foo?bar=at#anchor&title=My%20Blog%20&%20Your%20Blog"
>>
>> URI.escape 'http://google.com/foo?bar=at#anchor&title=My Blog & Your Blog'
=> "http://google.com/foo?bar=at%23anchor&title=My%20Blog%20&%20Your%20Blog"
>>
>> CGI.escape 'http://google.com/foo?bar=at#anchor&title=My Blog & Your Blog'
=> "http%3A%2F%2Fgoogle.com%2Ffoo%3Fbar%3Dat%23anchor%26title%3DMy+Blog+%26+Your+Blog"
>>
>> ERB::Util.url_encode 'http://google.com/foo?bar=at#anchor&title=My Blog & Your Blog'
=> "http%3A%2F%2Fgoogle.com%2Ffoo%3Fbar%3Dat%23anchor%26title%3DMy%20Blog%20%26%20Your%20Blog"

From this, if you want to encode a URL to be passed to CURL as the URL, use Addressable. If you want to encode a URL to pass as a parameter to another URL, then use either CGI or ERB.

elrayle commented 5 years ago

This was addressed by PR #210.