Skip to content

Throw notice for plain wrapper fread/fwrite errors #4462

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 4 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Jul 23, 2019

Similar to what we do for socket ops, throw a notice for fread/fwrite errors on the plain file wrapper. The most common condition here is trying to write to a read-only stream or vice versa.

This continues the work from #4442. This will fix a number of bugs, including https://bugs.php.net/bug.php?id=69163 and https://bugs.php.net/bug.php?id=60671.

Not sure if this will work on Windows...

cc @bwoebi

@nikic nikic force-pushed the stream-fail-warn branch from 8d25d47 to 0c564dd Compare July 23, 2019 11:17
@nikic
Copy link
Member Author

nikic commented Jul 23, 2019

It looks like Windows mostly works. Apart from Windows-specific tests, we're missing finfo_open(): read of 8192 bytes failed with errno=21 Is a directory notices in bug61964.phpt.

@bwoebi
Copy link
Member

bwoebi commented Jul 23, 2019

TBH I wasn't aware we weren't doing that … but I never attempted reading from a directory fd either - so +1 from me; cannot imagine these being unhelpful.

@nikic nikic force-pushed the stream-fail-warn branch from 0c564dd to 33a4422 Compare July 24, 2019 12:55
@nikic nikic force-pushed the stream-fail-warn branch from aecd74e to 9bc1d5f Compare July 24, 2019 14:49
@cmb69
Copy link
Member

cmb69 commented Jul 24, 2019

@nikic, see https://gist.github.com/cmb69/0dce91aa6886ef422f21975ba5dd5987. Instead of fixing fread_variation4-win32.phpt, just kill it and make fgetss_variation1-win32.phpt portable.

@nikic nikic force-pushed the stream-fail-warn branch from 9bc1d5f to e2d301f Compare July 24, 2019 15:01
@nikic
Copy link
Member Author

nikic commented Jul 24, 2019

@cmb69 Thanks, I've applied your patch now.

@cmb69
Copy link
Member

cmb69 commented Jul 24, 2019

AppVeyor CI failure is unrelated.

@nikic
Copy link
Member Author

nikic commented Jul 25, 2019

Merged as 1cbcf0f.

@nikic nikic closed this Jul 25, 2019
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 29, 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.

4 participants