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

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

Merged
merged 2 commits into from
Nov 8, 2023

Conversation

vroomfondle
Copy link
Contributor

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
Copy link
Owner

vgrem commented Nov 8, 2023

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
Copy link
Contributor Author

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 vgrem merged commit e41cb80 into vgrem:master Nov 8, 2023
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