Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Jul 25, 2019

Instead of rejecting directories / non-regular files early with
a generic error, we should just accept them and error later when a
read is attempted. This is more general and will generate a better
error message.

Instead of rejecting directories / non-regular files early with
a generic error, we should just accept them and error later when a
read is attempted. This is more general and will generate a better
error message.
@ramsey
Copy link
Member

ramsey commented May 27, 2022

I like this change. What's needed to merge it?

@Girgias Girgias requested a review from bukka January 25, 2023 19:35
@adoy adoy removed this from the PHP 8.2 milestone Feb 16, 2023
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I think it makes sense. I guess this was probably optimization before opcache where those fstat calls would happen on each request but with opcache it doesn't matter that much. So better errors make more sense.

Comment on lines 520 to 521
/* Still add it to open_files so the file_handle destruction logic works correctly. */
zend_llist_add_element(&CG(open_files), file_handle);
Copy link
Member

Choose a reason for hiding this comment

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

I see that this was already merged as part of other change ( 2f64844 ) so it should be removed from this PR.

@nikic nikic requested a review from iluuu1994 as a code owner October 7, 2023 13:51
@krakjoe
Copy link
Member

krakjoe commented Aug 31, 2025

It looks like we can't have this nice thing because windows.

If I'm wrong about windows, anyone is welcome to open a new PR that deals with the difference (I'm unsure how, or if it's possible).

@krakjoe krakjoe closed this Aug 31, 2025
@nikic
Copy link
Member Author

nikic commented Aug 31, 2025

@krakjoe I think we just need a separate test for Windows. For Windows the behavior remains unchanged, but we can still get a better error on Linux. Put up #19650 with that variant.

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

Successfully merging this pull request may close these issues.

5 participants