Skip to content
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

Classes aren't recognized when using a PHP extension as autoloader #7526

Closed
SMillerDev opened this issue Jun 24, 2022 · 16 comments · Fixed by phpstan/phpstan-src#1503
Closed

Comments

@SMillerDev
Copy link

Bug report

When using https://github.com/pprkut/autoload-psr PHPStan fails to load any non-project classes. Instead returning:

Could not read file: Lunr/Gravity/Database/DatabaseAccessObject.php

This is apparently because the php_stream_open_for_zend_ex calls in the extension do not trigger the code in:
https://github.com/phpstan/phpstan-src/blob/HEAD/src/Reflection/BetterReflection/SourceLocator/AutoloadSourceLocator.php

This does however still work on 1.6.9.

Code snippet that reproduces the problem

If you have the extension installed and run the code in https://phpstan.org/r/232ef63b-c3a0-45ba-b8f3-e58df73c9a44 with https://github.com/M2mobi/lunr in the Lunr-0.7 directory it should load the expected class, but doesn't.

Expected output

PHPStan can use the autoloader to find classes. Or, alternatively, the autoloader learns how to trigger the correct functions in PHPStan here.

Did PHPStan help you today? Did it make you happy in any way?

PHPStan has been a gamechanger to improve our code, it's simple, powerful and I love to use it.

@ondrejmirtes
Copy link
Member

Hello, can you please create a small reproducing repository with a series of steps that lead to this problem?

I tried to follow your instructions but failed - there's no Lunr-0.7 directory in https://github.com/M2mobi/lunr.

@SMillerDev
Copy link
Author

Here you go: https://github.com/SMillerDev/phpstan-repro the Makefile default target should do all the things you need. I also added a target to install the extension, but you might have your own preference there.

@ondrejmirtes
Copy link
Member

The problem is that the extension should load files by their absolute filenames but it doesn't do that, it loads Lunr/Core/Configuration.php for some reason.

I recommend you to use an alternative way to discover symbols, this phpstan.neon works just fine (instead of autoload.php):

parameters:
    scanDirectories:
        - Lunr.Config-0.7
        - Lunr.Locator-0.7

Since the extension doesn't do anything special, PSR-0 / PSR-4 autoloading has been part of PHP ecosystem for a decade through Composer, I recommend you to use a different way of setting up PSR autoloading. Thanks for understanding.

@pprkut
Copy link
Contributor

pprkut commented Jun 24, 2022

Sorry, I'm lost a little bit :(

The autoloader constructs a relative file path that's then resolved against the configured include path by PHP. I don't really see where the absolute path would need to come in. It would have to traverse all configured members of the include path manually to do that. PHP already does that so why would it do that by hand?

spl_autoload() essentially has the same behavior (I adapted the code from there)

@ondrejmirtes
Copy link
Member

PHPStan needs an absolute path to know which file to read to parse the sources. is_file('Lunr/Core/Configuration.php') is simply going to be false.

@pprkut
Copy link
Contributor

pprkut commented Jun 24, 2022

Alright, I get that. But stream_resolve_include_path() should help there. I'm happy to look into a change that would allow that. Is src/File/FileReader.php the right place to do that?

@ondrejmirtes
Copy link
Member

Doing stream_resolve_include_path('Lunr/Core/Configuration.php') doesn't work either, it returns false...

@pprkut
Copy link
Contributor

pprkut commented Jun 24, 2022

autoload_psr tries PSR-0 first before falling back to PSR-4. The Lunr\Core\Configuration.php would be what it tries to open for PSR-0, which fails because in the testcase autoloading is configured with PSR-4.
However, it will try to load it with PSR-4 afterwards (unless PSR-0 succeeded). So there should be another attempt to open the file with Lunr.Config-0.7/src/Lunr/Core/Configuration.php, and that should succeed. It's essentially a stacked autoloader scenario.

Since phpstan is trapping the filesystem calls, would it throw the exception after the first attempt and thus never get to the second one? Or is there perhaps something else to consider for a stacked autoloader scenario?

@ondrejmirtes
Copy link
Member

No, this is the only file tried. I feel like it's not worth to spending more time on this - PSR-0/PSR-4 autoloading is a solved problem that doesn't require a custom PHP extension to achieve. I even gave you the right PHPStan configuration to fix it :)

@pprkut
Copy link
Contributor

pprkut commented Jun 24, 2022

The problem is that doesn't work for us 😕
We don't know the absolute paths of the libraries on the system (that's up to the user), so we can't define a phpstan.neon file in the repo. We'd have to auto-generate it and that's not really worth it either. Using composer isn't an option either because it just doesn't do what we want.
And I totally believe this is a problem that's not unique to us. It's not like we're using a whole bunch of foreign concepts here. All of this should be reproducible with a different autoloader that relies on the include path. I still think this is solvable and worth solving in phpstan.

However, I also understand that you don't want to spend time on this. I'm not asking you to. But I hope you don't mind if I keep looking and try to get it working :)

@ondrejmirtes
Copy link
Member

It's interesting that your extension is in a weird spot that's not currently covered by PHPStan locator mechanisms.

If you used spl_autoload_register in your bootstrapFiles (or -a autoload file), this would most certainly work - there are integration tests for this scenario.

If you'd load absolute paths in your PHP extension autoloader, it'd also work.

If you used PSR-4 autoloading through Composer, it'd work because that's how the majority of modern projects works.

@pprkut
Copy link
Contributor

pprkut commented Jun 24, 2022

Perhaps even more interesting is that it actually is doing that, just not in the bootstrap file. It's calling spl_autoload_register() in the RINIT step.

@pprkut
Copy link
Contributor

pprkut commented Jun 25, 2022

@ondrejmirtes I found it :)

However, not sure what the best solution is, so your input would be appreciated!

Part 1 of the problem is that autoload_psr doesn't have a "file exists" check. We rely on the return value of php_stream_open_for_zend_ex() to know whether the file could be opened or not. Best I can tell, spl_autoload() behaves the same. FileReadTrapStreamWrapper::stream_open, however, returns always true. So phpstan hijacks the filesystem call and reports back to the autoloader that a non-existing file exists, and that's why it stops trying after the PSR-0 checks and doesn't continue with the PSR-4 check. I now modified it to this, which works, but is obviously POC code:

	public function stream_open($path, $mode, $options, &$openedPath): bool
	{
		echo "stream_open: $path\n";

		$exists = stream_resolve_include_path($path);

		if ($exists)
		{
			self::$autoloadLocatedFiles[] = $path;
		}
		$this->readFromFile = false;
		$this->seekPosition = 0;

		return $exists ? TRUE : FALSE;
	}

Part 2 is what we already suspected before, FileReader::read returns false for is_file and would need to additionally check against the include path. Again, POC code:

	public static function read(string $fileName): string
	{
		$path = $fileName;

		if (!is_file($path)) {
			$path = stream_resolve_include_path($fileName);

			if (!$path) {
				throw new CouldNotReadFileException($fileName);
			}
		}
		$contents = @file_get_contents($path);
		if ($contents === false) {
			throw new CouldNotReadFileException($fileName);
		}

		return $contents;
	}

With these two modifications in place, phpstan works for me :)

@pprkut
Copy link
Contributor

pprkut commented Jun 25, 2022

pprkut/phpstan-src@3b3f517 is the cleaned up version

@pprkut
Copy link
Contributor

pprkut commented Jul 10, 2022

@ondrejmirtes I tried to add an e2e test for phpstan for this based on spl_autoload(), but noticed it behaved differently and started investigating. Turns out FileReadTrapStreamWrapper isn't triggered for spl_autoload(), at all.

In CommandHelper::begin() the autoload file is loaded. It compares the autoloaders that were loaded before the autoload file was run, and the autoloaders loaded after. The new ones, supposedly added by the autoload file, get added to $GLOBALS['__phpstanAutoloadFunctions']. AutoloadFunctionsSourceLocator::locateIdentifier then executes every autoloader in that list to load the classes, after which class_exists() returns true. All autoloaders already loaded before the autoload file is run, need to go through the FileReadTrapStreamWrapper process instead.

While spl_autoload() is present in PHP from the beginning, it needs to be specifically enabled with a call to spl_autoload_register(), which happens in the autoload file and that's why even the default implementation of spl_autoload() ends up in $GLOBALS['__phpstanAutoloadFunctions']. autoload_psr(), on the other hand, is unconditionally loaded on PHP startup and thus already loaded before the autoload file runs, it's just configured by the autoload file. Since it's already present before the autoload file is run, it ends up on the stack that needs to go through FileReadTrapStreamWrapper. If I add it manually to $GLOBALS['__phpstanAutoloadFunctions'] it works just fine, without the code modifications I mentioned in #7526 (comment)

I'm not changing autoload_psr to need manual registration, since our whole point of wanting the extension was so we would have the autoloader available at all times without manual extra steps. So that leaves me with either whitelisting autoload_psr for $GLOBALS['__phpstanAutoloadFunctions'], or apply the fixes I mentioned before. The latter sounds more generic to me, so I'll open a pull request for that. But adding a test for it would require autoload_psr being installed, which I was trying to avoid 😕

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants