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

Stream must be a resource #61

Closed
jasonsarino opened this issue Jul 15, 2019 · 3 comments
Closed

Stream must be a resource #61

jasonsarino opened this issue Jul 15, 2019 · 3 comments
Labels

Comments

@jasonsarino
Copy link

Fatal error: Uncaught exception 'InvalidArgumentException' with message 'Stream must be a resource' in /home3/o3j3v5e1/public_html/onedrive/vendor/guzzlehttp/psr7/src/Stream.php:49 Stack trace: #0 /home3/o3j3v5e1/public_html/onedrive/vendor/microsoft/microsoft-graph/src/Http/GraphResponse.php(157): GuzzleHttp\Psr7\Stream->__construct(Array) #1 /home3/o3j3v5e1/public_html/onedrive/vendor/krizalys/onedrive-php-sdk/src/Proxy/DriveItemProxy.php(368): Microsoft\Graph\Http\GraphResponse->getResponseAsObject('GuzzleHttp\Psr7...') #2 /home3/o3j3v5e1/public_html/onedrive/redirect.php(45): Krizalys\Onedrive\Proxy\DriveItemProxy->download('hello123.txt') #3 {main} thrown in /home3/o3j3v5e1/public_html/onedrive/vendor/guzzlehttp/psr7/src/Stream.php on line 49

@krizalys
Copy link
Owner

Thanks for reporting this @jasonsarino.
I suspect a null value being passed around. Please can you share the code causing it?

BTW it seems that you are using the API incorrectly: Krizalys\Onedrive\Proxy\DriveItemProxy::download does not accept any argument: https://github.com/krizalys/onedrive-php-sdk/blob/master/src/Proxy/DriveItemProxy.php#L351

@krizalys krizalys added the bug label Jul 17, 2019
@feesler
Copy link

feesler commented Jul 19, 2019

@krizalys there is some story at msgraph-sdk-php behind this :)
The issue is reproducible with msgraph-sdk version 1.7.0-preview and later.

Original issue was reported on January and it was related to download DriveItem as stream. When it has been resolved some behavior changed.

GraphRequest class have it own member $returnType and it correctly set only by call of setReturnType() method. There is different behavior for return type 'GuzzleHttp\Psr7\Stream' and anything else.

Before 1.7.0-preview release at GraphRequest::execute() method 'stream' option was set directly to GuzzleHttp\Client::request() method. So for createRequest(...)->execute() case the 'stream' option for Guzzle client is explicitly set to FALSE. The body of response is immediately downloaded and returned with response object.

On release 1.7.0-preview set up 'stream' option GuzzleHttp\Client::request() method was removed and Guzzle client will always return response object with empty content but ready for stream read operations.
Next, because no return type is set the GraphResponse() constructor is invoked which generaly expects JSON content. It tries to json_decode() empty content and set empty array as body on fail.
Finaly, they removed check of return type for stream at GraphResponse::getResponseAsObject(). As result it currently tries to create stream object from empty array and throws an exception.

DriveItemProxy::download() method create via createRequest() and immediatelly execute() it. Next, calling $response->getStatus() we expect $response is GraphResponse. GraphRequest still does not support correct return of GraphResponse or Guzzle Response object on execute() for streaming download.

I think there is two ways how to fix this issue:
Quick fix is to set "microsoft/microsoft-graph": "1.6.0" at composer.json.
But there is issue with usage of stream in current behavior: whole file content downloaded into memory before to send response to caller, so it may and will cause memory issues.

Second way is to use returned stream at DriveItemProxy::download(). Unfortunatelly, we can't obtain status code from $response object, because it is Stream nor Response, but we can use try/catch block. Also, there is no need to convert to Stream by getResponseAsObject().
This will keep current behavior:

try {
    $response = $this
        ->graph
        ->createRequest('GET', $endpoint)
        ->setReturnType('GuzzleHttp\Psr7\Stream')
        ->execute();

    return $response->getContents();
} catch(ClientException $e) {
// we may use here information from guzzle client exception, ex. $e->getCode()
    throw $e;
} catch(Exception $e) {
    throw $e;
}

Also, it is a good idea to add DriveItemProxy::getDownloadStream() method which will simply return $response object to let user implement buffered stream download and avoid memory issues.

krizalys added a commit that referenced this issue Jul 21, 2019
@krizalys
Copy link
Owner

krizalys commented Jul 21, 2019

@jasonsarino @feesler Thanks for the details, this is fixed in 2.1.2.

Note: Krizalys\Onedrive\Proxy\DriveItemProxy::download() already returned a GuzzleHttp\Psr7\Stream: your client application is responsible for calling getContents() on it if needed, wary of possible memory issues that may arise when downloading big files. Your alternative is to read the stream in a streamed fashion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants