Skip to content

Conversation

dennisverspuij
Copy link
Contributor

Hi, thanks for the library!

I am using addFileFromStream and was suprised to see the stream resources being closed afterwards. I feel the library should only do this for streams it opens itself. Since Stream is not fopen'ing itself, it shoudn't fclose either, it is responsibility of the user.

@dennisverspuij
Copy link
Contributor Author

Sorry, actually the File patch could instead simply read:

             $stream = new DeflateStream(fopen($path, 'rb'));
             $this->processStream($stream);
+            $stream->close();

@NicolasCARPi
Copy link
Collaborator

Hello,

Thank you for your contribution. You can push another commit on the same branch if needed.

@maennchen
Copy link
Owner

Thanks @dennisverspuij I fully agree, that this behavior should be changed.

@NicolasCARPi How would you proceed? This is a breaking change. Would you actually release a new major version or add some switch to opt in to the correct behavior?

@NicolasCARPi
Copy link
Collaborator

A new major version is the way to go if the new behavior is the "correct" one.

@dennisverspuij
Copy link
Contributor Author

A new release would be very welcome!

@maennchen
Copy link
Owner

@NicolasCARPi Agreed

@NicolasCARPi
Copy link
Collaborator

So should we merge this and release a new major version then?

@maennchen
Copy link
Owner

@NicolasCARPi Yes, we should also add a Migration Help Text (also in the Readme)

@NicolasCARPi NicolasCARPi merged commit 803b765 into maennchen:master Feb 19, 2020
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.

3 participants