-
Notifications
You must be signed in to change notification settings - Fork 534
FileReadTrapStreamWrapper: Detect non existent files #722
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
Conversation
I propose this change to: Restrict the detection to actually opened files while detection loaded classes from autoloader. This will mitigate errors with old "shot in the dark" auto-loaders that are not using any kind of include restriction like: ``` Could not read file: boolean.php ``` As mentioned in: phpstan/phpstan#5074 THIS PROPOSAL IS ONLY FOR YOUR CONVENIENCE ! Feel free to discard without comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this change might be right, but we need a failing test for that to be sure.
public function stream_open($path, $mode, $options, &$openedPath): bool | ||
{ | ||
self::$autoloadLocatedFile = $path; | ||
self::$autoloadLocatedFile = $openedPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the different between those two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As described in the PHP Documentation:
path - Specifies the URL that was passed to the original function.
opened_path - If the path is opened successfully, and STREAM_USE_PATH is set in options, opened_path should be set to the full path of the file/resource that was actually opened.
So if the intention of the FileReadTrapStreamWrapper is to detect successful autoloading - then this should be right.
EDIT: Sorry forgot the link https://www.php.net/manual/de/streamwrapper.stream-open.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we're dealing with an unsuccessful autoloading, so what's in the variable then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case the autoloader tries to blindly load "Test/boolean.php" by @include("Test/boolean.php");
$path will be "Test/boolean.php"
and
$opened_path will be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this you should be able to recreate the issue: https://github.com/fsmoak/phpstan-zf1-autoload-test
This should no longer be a problem, please test phpstan/phpstan dev-master: e30f446 |
I propose this change to: Restrict the detection to actually opened files while detection loaded classes from autoloader.
This will mitigate errors with old "shot in the dark" auto-loaders that are not using any kind of include restriction like:
As mentioned in: phpstan/phpstan#5074
THIS PROPOSAL IS ONLY FOR YOUR CONVENIENCE ! Feel free to discard without comment.