Skip to content

[Bug] 77024 - Correct the behaviour of casting spl files to strings #3820

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

Closed
wants to merge 2 commits into from

Conversation

duncan3dc
Copy link
Member

@duncan3dc duncan3dc commented Feb 11, 2019

Bug #77024

This PR makes __toString() in a CSV context consistent with a plain text file.

This behaviour contradicts the manual (and common sense as far as I can tell) but this PR ensures backwards compatibility

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test currently fails on Windows. It's probably best not to use a temporary file. Either use SplTempFileObject for the second part as well, or create an actual file for this.

@duncan3dc
Copy link
Member Author

@nikic The second test is to ensure SplFileObject works too, if we use SplTempFileObject again then we have no regression test that protects us from somebody changing the behaviour of SplFileObject do we?
Looking at the AppVeyor error, presumably adding an unset($file) before unlink() may resolve it?

@nikic
Copy link
Member

nikic commented Feb 13, 2019

Looking at the AppVeyor error, presumably adding an unset($file) before unlink() may resolve it?

If that works, that would be fine as well.

@petk
Copy link
Member

petk commented Feb 13, 2019

Or add the unlink() call into the --CLEAN-- section instead. I think that helps on Windows in some cases also.

@duncan3dc
Copy link
Member Author

Windows build is fixed now 👍

@petk
Copy link
Member

petk commented Apr 27, 2019

Applied via 91c6fb8 to PHP-7.2+. Thank you @duncan3dc and the reviewers...

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

Successfully merging this pull request may close these issues.

None yet

4 participants