szepeviktor / phpstan-wordpress

WordPress extensions for PHPStan ⛏️
https://packagist.org/packages/szepeviktor/phpstan-wordpress
MIT License
267 stars 26 forks source link

Use relative path in config file #103

Closed bart-jaskulski closed 2 years ago

bart-jaskulski commented 2 years ago
By dropping `%rootDir%` variable, including extension isn't strictly tied to path of PHPStan executable.

I would like to request a change in config file for this extension.

In my case, I would like to use PHPStan through phar file located in tools directory (default for installation from phive). Actually, it's possible, if I don't use phpstan/extensions-installer and rely on explicit include in my config file. (This also applies to globally installed phpstan executable).

Relying on relative path searches for wordpress-stubs package from current working directory instead of executable location, which is desired in aforementioned cases.

szepeviktor commented 2 years ago

Hello Bartek! 👋 Thank you for your report and suggestion.

Relying on relative path searches for wordpress-stubs package from current working directory instead of executable location, which is desired in aforementioned cases.

AFAIK PHPStan looks for relative paths from the config file itself extension.neon - that is why %rootDir% was removed. Please see https://phpstan.org/config-reference#expanding-paths

Do you have modern PHPStan?

bart-jaskulski commented 2 years ago

Hi Viktor!

Sorry, I may have written previous message in a bit bizzare manner as it was quite late for me :confused: :laughing:

I'm using the newest PHPStan version: 1.6.7

I'm aware that PHPStan resolves relative paths from config file and this was what I meant about relying on relative path search. This is precisely why my PR suggests removing %rootDir%, which is still present in extension.neon configuration in this repository.

I hope now I've described this more precisely :D

szepeviktor commented 2 years ago

You know what! It was removed long time ago and re-added in #58. 🤯

szepeviktor commented 2 years ago

Thank you!!

szepeviktor commented 2 years ago

Now PHPUnit tests are failing. 💣 Investigating...

herndlm commented 2 years ago

the path can be resolved relatively if this extension is installed via composer I presume, but not for the lib here itself, since ../ would already move out of the root dir of this repo basically (don't require other files from my system please xD)

anyways, I also was thinking about a solution but the only thing I came up with so far, is a bootstrap.php file here where you can add some logic to dynamically include the right file or basically check where it is. not my preferred solution but should work I guess

szepeviktor commented 2 years ago

This is the source https://github.com/szepeviktor/phpstan-wordpress/blob/master/tests/DynamicReturnTypeExtensionTest.php#L40-L44

All I can think of

herndlm commented 2 years ago

the development copy approach would work, but well, it's a copy hmm.. I looked at phpstan itself and some of the extensions and did not find a case yet where a bootstrap file is required from vendor basically as it is the case here. Let me open the PR that dynamically requires it, feel free to close it, I'm not a 100% happy with it, but still prefer if over a potentially hard-to-maintain copy I think

bart-jaskulski commented 2 years ago

This case is more complicated than it seemed at first.

After a bit of digging I would advise to create separate config file for test purposes and another one for inclusion by clients consuming this library. Let me present what have I found.

Using %rootDir% in config easily causes tests to pass, because we are referencing stubs package directly through our package, then composer's vendor directory is the same for PHPStan, which lies in vendor directory. Thus it works both for test cases and client's applications.

Using %currentWorkingDirectory% is generally a bad idea, because in tests path is inferred from phar package, providing some weird reference to non existing file. Not much to say.

Using relative path, which is interpreted by PHPStan is good solution for client's usage, but tests, which are using a bit different environment for execution are failing, because through ../../php-stubs/... we are referencing files from outside project scope (eg. if path is /home/user/repo/phpstan-wordpress, PHPStan during tests tries to find files in /home/user/php-stubs).

At first, I thought it could be resolved by changing the tests, but this isn't possible as well - all of the logic is inherited from PHPStan testing utilities, which underneath compiles a DI container with resolved path. The sole abuser of our config file lies exactly on following line:

$val = $fileHelper->normalizePath($fileHelper->absolutizePath($val));

https://github.com/phpstan/phpstan-src/blob/c8b3926f005d008178d6d8c62aaca0200a6359a2/src/DependencyInjection/NeonAdapter.php#L125

PHPStan, when loading config file, already resolves and normalizes paths and this part is impossible to change - it works as expected, despite our test cases.

Thus, the best solution to keep tests passing and allow using this extension without composer based installation of PHPStan would be to create separate config file for testing purposes.

Furthermore, this solution would enable the whole package not to strictly demand PHPStan installation as dependency, but only placing it as suggested package - letting users decide whether they want to place PHPStan itself in their dependencies.

szepeviktor commented 2 years ago

@bart-jaskulski Thank you!

szepeviktor commented 2 years ago

@bart-jaskulski Please see #105 (merged).