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

#5 Allow setting a proxy #27

Merged
merged 4 commits into from
Aug 10, 2017
Merged

#5 Allow setting a proxy #27

merged 4 commits into from
Aug 10, 2017

Conversation

qurben
Copy link
Contributor

@qurben qurben commented Aug 7, 2017

Save the proxy in the ApiContext. Allow an api context to not contain a proxy.

Fixes #5

@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@qurben Have you tested it somehow? :)

@qurben
Copy link
Contributor Author

qurben commented Aug 7, 2017

@dnl-blkv I tested it with a socks proxy through ssh to a remote server.

Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments :)

README.md Outdated
##### Proxy

You can use a proxy with the bunq PHP SDK. This option must be a string. This proxy will be used for all requests done with
the SDK. You will be prompted to provide a proxy URL when using the interactive installation script.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be smth like This proxy will be used for all the requests done with the context for which it was specified ?

README.md Outdated
@@ -80,6 +80,20 @@ $apiContext = ApiContext::restore($fileName);
**Tip:** both saving and restoring the context can be done without any arguments. In this case the context will be saved
to/restored from the `bunq.conf` file in the same folder with your script.

##### Proxy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this newline? :P

README.md Outdated
the SDK. You will be prompted to provide a proxy URL when using the interactive installation script.

```php
$proxyUrl = 'socks5://localhost:1080'; // The proxy for all requests, leave empty to disable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave empty? So, empty string disables it?

@@ -331,6 +346,14 @@ public function getEnvironmentType()
}

/**
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|null

@@ -331,6 +346,14 @@ public function getEnvironmentType()
}

/**
* @return string
*/
public function getProxyUrlOrNull()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically do not do OrNull in the getters in the SDK code. The reason is that the option of nullability is already stated in the doc header, and adding it into method name is not a normal practice among the PHP community.

@OGKevin OGKevin assigned qurben and unassigned qurben and dnl-blkv Aug 7, 2017
@qurben qurben assigned dnl-blkv and unassigned qurben Aug 7, 2017
@OGKevin OGKevin requested a review from dnl-blkv August 7, 2017 11:16
@OGKevin OGKevin added this to the Patch 0.9.2 milestone Aug 7, 2017
@OGKevin OGKevin requested review from dnl-blkv and removed request for dnl-blkv August 10, 2017 06:49
Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dnl-blkv dnl-blkv merged commit c8a2dc5 into develop Aug 10, 2017
@dnl-blkv dnl-blkv deleted the 5-allow-proxy branch August 10, 2017 08:57
@OGKevin OGKevin modified the milestones: Patch 0.9.2, v0.10.0 Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants