Skip to content

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

Closed
duncan3dc wants to merge 2 commits intophp:PHP-7.2from
duncan3dc:bug-77024
Closed

[Bug] 77024 - Correct the behaviour of casting spl files to strings #3820
duncan3dc wants to merge 2 commits intophp:PHP-7.2from
duncan3dc:bug-77024

Conversation

@duncan3dc
Copy link
Member

@duncan3dc duncan3dc commented Feb 11, 2019 •
edited
Loading

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

nikic
nikic reviewed Feb 12, 2019
View reviewed changes
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

duncan3dc commented Feb 13, 2019

@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

duncan3dc commented Feb 15, 2019

Windows build is fixed now 👍

carusogabriel
carusogabriel reviewed Feb 16, 2019
View reviewed changes
@petk
Copy link
Member

petk commented Apr 27, 2019

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

@petk petk closed this Apr 27, 2019
@cmb69 cmb69 mentioned this pull request Nov 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

+1 more reviewer

@carusogabriel carusogabriel carusogabriel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@duncan3dc @nikic @petk @carusogabriel