ably / ably-php

PHP client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
49 stars 10 forks source link

Fixes relative path creation issue in PaginatedResult@parsePaginationHeaders #89

Closed srhyne closed 3 years ago

srhyne commented 3 years ago

Consuming the following pagination request I received API path errros..

$pushAdmin = $client->push->admin;
$page = $pushAdmin->deviceRegistrations->list_([ 'limit' => 10 ]);
if( $page->hasNext() ){
    $page->next();
}

Inspecting the $path passed to requestInternal showed me that /push was becoming /push/push

QuintinWillison commented 3 years ago

Hello @srhyne, and thanks for this. 😁

Sorry for the lack of response on this pull request. We're looking to do some more work on this library very soon and will look at the issue you've seen and the resolution you propose in this PR.

jdavid commented 3 years ago

Hello @srhyne

This PR is related to PR #81 , where I added a unit test that shows the problem, see https://github.com/ably/ably-php/runs/1816457788?check_suite_focus=true#step:6:271

And indeed your commit fixes that unit test. But it breaks many others, as can be seen in Travis, e.g. https://travis-ci.org/github/ably/ably-php/jobs/753470119#L659

We have moved today from Travis to GitHub actions. And I've cherry-picked your commit in PR #81, so we can see the same errors in GitHub actions, e.g. https://github.com/ably/ably-php/runs/1816826803?check_suite_focus=true#step:6:325

IMHO the response sent by the server doesn't look good:

@QuintinWillison We can find a better workaround that fixes the new unit test without breaking anything else. Or maybe you can look at the server side?

By the way there's the same problem in ably-python (which uses urllib.parse.urljoin) but there's not a unit test that shows it.

jdavid commented 3 years ago

I'm closing this PR in favor of PR #81 where I've pushed a different workaround.