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

[BUG] - Closing of stream after upload is not possible, resource is not valid. #2094

Closed
3 tasks done
jandominik opened this issue Mar 8, 2019 · 12 comments
Closed
3 tasks done

Comments

@jandominik
Copy link

Prerequisites

  • Able to reproduce the behaviour outside of your code, the problem is isolated to Laravel Excel.
  • Checked that your issue isn't already filed.
  • Checked if no PR was submitted that fixes this problem.

Versions

  • PHP version: 7.2.10
  • Laravel version: 5.7.28
  • Package version: 3.1.9

Description

Steps to Reproduce

Hello, problem is in closing of stream after upload to some storage like S3 or Google Cloud.

  1. I've got export job on which is ->queue() called.
  2. After dispatching this export job, the export goes well
  3. Exported file is uploaded to the storage
  4. But in the end of closing of local read stream, there is an error:
    fclose(): supplied resource is not a valid stream resource [2019-03-07 21:02:05] local.ERROR: fclose(): supplied resource is not a valid stream resource {"userId":10,"email":"mail@example.com","exception":"[object] (ErrorException(code: 0): fclose(): supplied resource is not a valid stream resource at /Applications/MAMP/htdocs/project/vendor/maatwebsite/excel/src/Files/Disk.php:82) [stacktrace] #0 /Applications/MAMP/htdocs/project/vendor/sentry/sentry/lib/Raven/Breadcrumbs/ErrorHandler.php(34): Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError(2, 'fclose(): suppl...', '/Applications/M...', 82, Array) #1 [internal function]: Raven_Breadcrumbs_ErrorHandler->handleError(2, 'fclose(): suppl...', '/Applications/M...', 82, Array) #2 /Applications/MAMP/htdocs/project/vendor/maatwebsite/excel/src/Files/Disk.php(82): fclose(Resource id #883) #3 /Applications/MAMP/htdocs/project/vendor/maatwebsite/excel/src/Jobs/StoreQueuedExport.php(54): Maatwebsite\\Excel\\Files\\Disk->copy(Object(Maatwebsite\\Excel\\Files\\LocalTemporaryFile), 'exports/clients...')

Expected behavior:

File should be uploaded as well but stream should be correctly closed just if it is possible.

Actual behavior:

Excel library is trying to close already closed stream or not valid stream.

Additional Information

There is a problem in Maatwebsite\Excel\Files\Disk::copy(). On line 82 is called fclose() on read stream but the stream is not in type="stream" but in type="Uknown". is_resource() function returns false so stream is not possible to close.

How to fix it? Just check the stream by is_stream() before closing.

According to FlySystem library - https://flysystem.thephpleague.com/docs/usage/filesystem-api/, Using streams for reads and writes paragraph - "Some SDK’s close streams after consuming them, therefore, before calling fclose on the resource, check if it’s still valid using is_resource.".

This is typical behavior for for example S3 adapter and another.

However thank you for this library, I hope this report has enough details.

@patrickbrouwers
Copy link
Member

Can you PR this change. Thanks

@jandominik
Copy link
Author

Of course, during this week I'm going to create PR. Thank you.

@lucacri
Copy link

lucacri commented Mar 13, 2019

This bug hit me hard :-(

Thank you for finding the culprit, I'll wait for the PR!

@patrickbrouwers
Copy link
Member

Haven't been able to reproduce it myself with S3, but have committed the change. Thanks for reporting the issue. Will be fixed in the next release.

@lucacri
Copy link

lucacri commented Mar 19, 2019

Awesome! Any chance you can create a release with this patch?

@patrickbrouwers
Copy link
Member

Will be released this week

@jandominik
Copy link
Author

Thank you Patrick for the commit. Sorry but I wasn't able to work on it last week. However thank you!

@lucacri
Copy link

lucacri commented Mar 26, 2019

@patrickbrouwers sorry to bother you (really), any chance you can tag the new release? I am holding off upgrading Laravel to 5.8 because of this bug!

Thank you so much for this package!

@rikless
Copy link

rikless commented Apr 2, 2019

@patrickbrouwers also concerned 🤞

@GlennM
Copy link
Contributor

GlennM commented Apr 2, 2019

@rikless
Copy link

rikless commented Apr 2, 2019

@patrickbrouwers yes, but not released

@patrickbrouwers
Copy link
Member

It's released now https://github.com/Maatwebsite/Laravel-Excel/releases/tag/3.1.11

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

No branches or pull requests

5 participants