-
Notifications
You must be signed in to change notification settings - Fork 259
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
Iterator final touches #521
Conversation
ecoologic
commented
Nov 14, 2023
•
edited
Loading
edited
- Fix issue with Iterator API call frequency
- Expose the API call response for debugging and custom implementations
- Documentation
- Refactoring
Get a page only if no item in memory
32382c9
to
2c759f2
Compare
private function getPageIfNeeded() | ||
{ | ||
if (!$this->strategy->shouldGetPage($this->position)) { | ||
if (isset($this->page[$this->position]) || !$this->strategy->shouldGetPage($this->position)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before, loop cycle was making an HTTP call till all records were fetched, now it waits until it's done processing the whole page.
2c759f2
to
25187e2
Compare
return; | ||
} | ||
|
||
$getPageFn = function () { | ||
return $this->clientList->{$this->method}($this->strategy->params()); | ||
}; | ||
|
||
$this->page = array_merge($this->page, $this->strategy->page($getPageFn)); | ||
$this->items = array_merge($this->items, $this->strategy->page($getPageFn)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't store one single page, iterator_to_array
seems to require all items, see https://github.com/zendesk/zendesk_api_client_php/pull/520/files#diff-a0b5e09b74e9d3459034f5d68a5535a04fc8dc5e3a01947e0173780ac78cd60aR86
1e08103
to
339610d
Compare
|
||
$this->assertEquals($mockException, $previousException, $message); | ||
$this->assertEquals([], $subject->getErrorDetails()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
admittedly not a great test, but better than deleting the file.
d83f028
to
ca1f2c9
Compare
d32af7e
to
cae5e86
Compare
cae5e86
to
2ce8083
Compare
$this->latestResponse = $getPageFn(); | ||
if (!isset($this->latestResponse->meta->has_more)) { | ||
throw new PaginationError( | ||
"Response not conforming to the CBP format, if you think your request is correct, please open an issue at https://github.com/zendesk/zendesk_api_client_php/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you come across a case like this? can you try CBP requests on resources that do not implement them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found any. But the iterator can be used in a customized way by the final users, so it can either be a problem with an endpoint that we missed (php also defines chat, talk, HC, voice). This is a friendly error so that the final user won't have to debug our code.
@@ -1,5 +1,7 @@ | |||
# OBP to CBP Upgrade guide | |||
|
|||
TODO version requirement, how to bump. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this to do is meant to be there and will be filled out later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks, this is the upcoming work. To be addressed in a separate PR.