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

[FEATURE] Adds getAll() method to BaseClient #61

Conversation

web-dev-passion
Copy link

@web-dev-passion web-dev-passion commented Sep 16, 2022

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Other (please describe):

How to test this PR

This PR uses get() inside its method body. It is inspired by the getAll() implementation inside the storyblok-js-client.
See https://github.com/storyblok/storyblok-js-client

After reading through this info you should have an insight of what functionality the PR provides.
You can test the pagination logic by making a getAll()-method-call and compare what effects different api conditions (for example 24 entries, 25 or 26) have.
The getAll(() is expected to return an array of one - many stdClass storyblok responses, just like get() returns but multiple respones.

What is the new behavior?

First of all, the getAll() takes the same parameters like get().
If per_page is provided via the $queryString parameters it gets used, otherwise a newly created class constant DEFAULT_PER_PAGE which is set to 25 is used.
The first get request gives us the first response $firstResponse.
With this response we investigate the response headers sent to define our needed amount of requests with the goal in mind to receive all entries at the end.

In general the getAll() makes one to many get() requests based on totalRecords, that were received through the first response $firstResponse via the responses' header ['Total'].

  • iterating over all pages needed to return an array of all responses at the end

Other information

To my mind this PR is more than needed. Every time a user of this php-client want to get more entries than what can be controlled via per_page which is limited to 1000 by Storyblok's limitations, he/she needs to implement this logic everytime.
Moreover, it was included in the js-implementation, so why should not we also implement this in PHP?

@roberto-butti
Copy link
Collaborator

Hi @web-dev-passion, a great method that aims to improve the developer experience.
I will test with many Stories, focusing mainly on error management and API limit management. And tests the scenario if, under the hood, an error happens in just one HTTP call; check the result (expected empty array, IMHO) and the raised exception.

@roberto-butti
Copy link
Collaborator

Thank you @web-dev-passion i see you update the PR with the latest changes. 🥳
So @joaokamun , in my opinion, we can start to approve at least the workflow because: "1 workflow awaiting approval".

@roberto-butti
Copy link
Collaborator

@web-dev-passion I'm going to ask you for a little more effort. As you can see, the CICD failed.
I'm asking to run on your code the command composer run format that automatically will format the syntax according to PSR12 standards.
Thank you!

@roberto-butti
Copy link
Collaborator

@web-dev-passion i was creating integration tests for your pull request.
I have a question, because get() is returning a standard class. So instead of using getHeaders(), why not access to the headers with $firstResponse->httpResponseHeaders[
I would like to add a parameter to getAll for returning a list of response (like now) or a list of items (an array of stories for example)

The integration test I'm using is:

test('Integration: get All stories', function () {
    $client = new Client('Iw3XKcJb6MwkdZEwoQ9BCQtt');
    $options = $client->getApiParameters();
    $options['per_page'] = 3;
    $responses = $client->getAll('stories/', $options);
    $this->assertCount(3, $responses);
    $stories = [];
    foreach ($responses as $response) {
        array_push($stories, ...$response->httpResponseBody['stories']);
    }
    $this->assertCount(8, $stories);
})->setGroups(['integration']);

@roberto-butti roberto-butti merged commit 9d33c93 into storyblok:master Dec 16, 2022
@roberto-butti
Copy link
Collaborator

@web-dev-passion I'm going to merge your PR and i will fix the code style.
Thank you for the amazing PR!!!

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