basvandorst / StravaPHP

Strava API REST client with OAuth authentication
MIT License
184 stars 67 forks source link

Streams resolution parameter default value #17

Closed qligier closed 8 years ago

qligier commented 8 years ago

Hey, The default value of the resolution parameter in the streams function is set to all:

$client->getStreamsActivity($id, $types, $resolution = 'all', $series_type = 'distance');
$client->getStreamsEffort($id, $types, $resolution = 'all', $series_type = 'distance');
$client->getStreamsSegment($id, $types, $resolution = 'all', $series_type = 'distance');

The documentation isn't quite clear on this, but all isn't a valid value:

curl -G https://www.strava.com/api/v3/activities/683597852/streams/latlng -H "Authorization: Bearer abc" -d resolution=all

returns

{"message":"Bad Request","errors":[{"resource":"Application","field":"resolution","code":"invalid"}]}

As I understand it: If you don't specify the parameter, all points are returned. If you specify it (with low, medium or high), only (100/1000/10000) values are returned.

fbonzon commented 8 years ago

I confirm. To mean "all" the resolution should be left empty. I.e. replace everywhere 'all' by the empty string ''.

qligier commented 8 years ago

Setting the parameter to null also works, that's what I've done to patch this issue in my local repository.

fbonzon commented 8 years ago

Correct, null works too.

Could you change the 3 occurrences of 'all' to null in those 3 files too in your fork: src/Strava/API/Service/REST.php src/Strava/API/Service/ServiceInterface.php src/Strava/API/Service/Stub.php and send a PR here?

qligier commented 8 years ago

Done. The code works but it isn't passing the test because of the new version of dependencies. I'll fix that later.

basvandorst commented 8 years ago

Fixed in 1.0.2. Thanks to @qligier