Skip to content

Remove include "sanity check" to get better error #4474

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove include "sanity check" to get better error #4474

wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Jul 25, 2019 •
edited
Loading

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.
@nikic
Remove include "sanity check" to get better error
f355717 
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 ramsey added this to the PHP 8.2 milestone May 27, 2022
@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
bukka
bukka reviewed Mar 17, 2023
View reviewed changes
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.
Zend/zend_language_scanner.l
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Streams Waiting on Review
4 participants
@nikic @ramsey @bukka @adoy