Fix: Don't try to retrieve next set of items when using an iterator, if no next items are expected to exist

vroomfondle commented 10 months ago

We use this library to retrieve Sharepoint lists and came across a crash when using the getAll() method of ClientObjectCollection.

Symptoms

When using foreach on a ListItemCollection, the following error occurs:

In Requests.php line 34:

  [Office365\Runtime\Http\RequestException (3)]  

Exception trace:
  at /var/www/api/vendor/vgrem/php-spo/src/Runtime/Http/Requests.php:34
 Office365\Runtime\Http\Requests::execute() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/ClientRequest.php:101
 Office365\Runtime\ClientRequest->executeQueryDirect() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/OData/ODataRequest.php:128
 Office365\Runtime\OData\ODataRequest->executeQueryDirect() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/ClientRequest.php:83
 Office365\Runtime\ClientRequest->executeQuery() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/ClientRuntimeContext.php:81
 Office365\Runtime\ClientRuntimeContext->executeQuery() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/ClientObject.php:99
 Office365\Runtime\ClientObject->executeQuery() at /var/www/api/vendor/vgrem/php-spo/src/Runtime/ClientObjectCollection.php:372
 Office365\Runtime\ClientObjectCollection->getIterator() at /var/www/api/app/Integration/Services/Sharepoint/ListFileIntegration.php:67
...

Cause

I believe this is because we're using a foreach which is causing getIterator to be called and that is trying to retrieve the next set of items before it checks whether there are any more items to retrieve.

Fix

Simple - I just moved the line which retrieves the items inside the conditional which checks whether items are expected to exist.

Tests

I'm happy to add a test somewhere if someone can give me a pointer to the relevant test case.

vgrem commented 10 months ago

Greetings, thank you for catching it and providing the elegant solution! :)

In terms of:

I'm happy to add a test somewhere if someone can give me a pointer to the relevant test case.

ListItemTest.php fits for the new test to be introduced and this example demonstrates the use cases for that method

vroomfondle commented 10 months ago

I can't execute the tests locally due to a lack of credentials (and we don't have a suitable sharepoint instance for me to run them against), but I've added a test anyway - hopefully it's good enough.

vgrem / phpSPO