Skip to content

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

Closed
nikic wants to merge 5 commits into
php:masterfrom
nikic:include-fail
Closed

nikic wants to merge 5 commits into
php:masterfrom
nikic:include-fail

Conversation

@nikic

@nikic nikic commented Jul 25, 2019 •

Copy link
Copy Markdown
Member

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

ramsey commented May 27, 2022

Copy link
Copy Markdown
Member

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 left a comment

Copy link
Copy Markdown
Member

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 thread Zend/zend_language_scanner.l Outdated
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
Copy Markdown
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

krakjoe commented Aug 31, 2025

Copy link
Copy Markdown
Member

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

nikic commented Aug 31, 2025

Copy link
Copy Markdown
Member Author

@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