search

bugsnag / bugsnag-api-ruby

BugSnag API toolkit for Ruby
Other
21 stars 15 forks source link

Remove call to deprecated URI.escape. #28

Closed askreet closed 3 years ago

askreet commented 3 years ago

Goal

In Ruby 3, URI.escape is removed. There is no simple replacement provided by Ruby.

Design

As far as I can tell, this isn't necessary. When passing query parameters to GET requests, or form data to POST requests, we use the underlying Faraday bits with a Hash, which are encoded correctly. The path argument should already be a well-formed URI, or it will produce an HTTP error from the API as normal.

Changeset

Simply removed the call to URI.escape.

Testing

The RSpec test suite has 5 failing tests on master for me locally with Ruby 2.6.5. With Ruby 3.0.0, most of the suite fails with calls to URI.escape. After removing this call, I was back to the same 5 tests failing.

yousif-bugsnag commented 3 years ago

Hi @askreet - Thanks for the PR!

Since you can call get/post/etc with any url, we'll need to look a bit further into whether or not we can simply remove URI.escape or if we need to replace it with CGI.escape. We'll take a look when priorities allow.

askreet commented 3 years ago

@yousif-bugsnag CGI.escape will convert path separators (/) to %2F. While this might be OK, it will look odd in logs and such.