mirakl / sdk-php-shop

Mirakl PHP SDK for sellers
30 stars 16 forks source link

Pagination example is unclear in GetOrdersRequest #33

Open shrink opened 2 years ago

shrink commented 2 years ago

For GetOrdersRequest::class the example mentions pagination:

/**
 * (OR11) Retrieve orders
 *
 * Sort by creation date, order identifier, shop name and then by index of the order line
 * This API uses pagination by default and will return 10 orders
 *
 * Example:
 *
 * <code>
 * use Mirakl\MMP\Shop\Client\ShopApiClient;
 * use Mirakl\MMP\Shop\Request\Order\Get\GetOrdersRequest;
 *
 * $api = new ShopApiClient('API_URL', 'API_KEY', 'SHOP_ID');
 * $request = new GetOrdersRequest();
 * // Optional parameters
 * $request->setOrderIds(['ORDER_ID_1', 'ORDER_ID_2'])
 *     ->setOffset(10)
 *     ->setMax(25)
 *     ->setPaginate(false)
 *     ->setStartDate('2015-06-09')
 *     ->setEndDate('2015-06-30');
 * $result = $api->getOrders($request);
 * // $result => @see \Mirakl\MMP\Shop\Domain\Collection\Order\ShopOrderCollection
 * </code>
 */

However, if we look in AbstractRequest::class we can see that setPaginate(false) means that max and offset are ignored:

        /** @see PageableTrait */
        if (is_bool($this->getPaginate())) {
            $params['paginate'] = $this->getPaginate() ? 'true' : 'false';
            if ($this->getPaginate()) {
                $params['max'] = $this->getMax();
                $params['offset'] = $this->getOffset();
            }
        }

I think the example is therefore misleading because setPaginate(false) is preventing the use of the values provided in setMax() and setOffset(). I am creating this as an issue rather than with a proposed change in a Pull Request as I am not confident that my understanding is correct, because the API does accept a paginate parameter which is described as...

paginate optional boolean Control the pagination usage Default to true

So it seems possible to completely disable pagination for this endpoint (and therefore get all results, independent of the max?) which would indicate that setPaginate(false) does have some intended influence on how the API responds. Unfortunately, we don't have enough orders in our Mirakl instance to test the behaviour of the API when there's more than 100 results.

jreinke commented 2 years ago

Hello,

You are right for the OR11 request example, the setPaginate(false) does not make sense if max and offset parameters are specified. Thanks for your feedback, we will modify the example. For some APIs like OR11, the paginate=false parameter does make sense and will return all the results in a single API call like you mentioned.

jsmcortina commented 1 year ago

This seems to be similar with getOffers() - I tried to disable pagination to get all offers (<400) but it doesn't work and I couldn't work out how to fetch the next page. With setPagination(true) I can setMax(10000) and then fetch all offers.

I've really struggled to use this SDK. The REST docs are easy to follow, but the lack of docs in the SDK has made it slow for me to implement. (I'd have been quicker by ignoring the SDK and just using the JSON.)