Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally pass in proxy parameter to Savon #548

Conversation

dbecker-stripe
Copy link
Contributor

@dbecker-stripe dbecker-stripe commented Jun 14, 2022

Savon will call to_s on any proxy value passed in so it will treat nil like an empty string instead of not using it. This causes requests to fail with an invalid proxy. This change will not pass in a value for proxy if the attribute is nil.

This change also updates history with addition of the proxy attribute.

A fix was added to version 2.12.0 of savon but this changes allows netsuite to be compatible with earlier versions of savon.

Savon will call `to_s` on any `proxy` value passed in so it will treat `nil` like an empty string instead of not using it. This causes requests to fail with an invalid proxy. This change will not pass in a value for `proxy` if the attribute is `nil`.

This change also updates history with addition of the proxy attribute.
lib/netsuite/configuration.rb Outdated Show resolved Hide resolved
@iloveitaly
Copy link
Member

@dbecker-stripe what version of savon fixed the weird issue you were running into? I think it's worth adding that as a gemspec dependency.

@iloveitaly iloveitaly merged commit 7490788 into NetSweet:master Mar 27, 2023
andrewdicken-stripe added a commit to andrewdicken-stripe/netsuite that referenced this pull request Apr 19, 2023
iloveitaly pushed a commit that referenced this pull request Aug 14, 2023
* Revert "Optionally pass in `proxy` parameter to Savon (#548)"

This reverts commit 7490788.

* Restore history to state of master

* Updates history with current change

* Revert test file back to state of master

* Random comment change to get another build

* Reverting comment test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants