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

Placeholders in config file doesn't seem to be supported out of parameters #2020

Closed
Taluu opened this issue Mar 21, 2019 · 14 comments
Closed

Placeholders in config file doesn't seem to be supported out of parameters #2020

Taluu opened this issue Mar 21, 2019 · 14 comments

Comments

@Taluu
Copy link

@Taluu Taluu commented Mar 21, 2019

Summary of a problem or a feature request

In the config file, we should have at least these two placeholders : %rootDir% (directory where phpstan is installed) and %currentWorkingDirectory% (directory from where phpstan is launched).

But it seems they are only supported in parameters section, not the includes section... This could be nice to have at least %rootDir% supported, if you install global dependency / plugins (such as the symfony one), but don't want to clutter the config file (and commit it...) with a personnal value in my case, /home/talus/.composer/vendor/...`).

Code snippet that reproduces the problem

includes:
  - %currentWorkingDirectory%/foo
  - %rootDir%/../phpstan-symfony/extension.neon
  - %rootDir%/../phpstan-doctrine/extension.neon
  - %rootDir%/../../jangregor/phpstan-prophecy/src/extension.neon

Neither %currentworkingDirectory nor %rootDir% are resolved, as phpstan tries to load (let's say I'm triggering phpstan from /home/talus/dev/project) : File '/home/talus/dev/project/%currentWorkingDirectory%/foo' is missing or is not readable. Same thing for the ones with %rootDir%.

Expected output

The files are correctly loaded / included. :}

@Taluu
Copy link
Author

@Taluu Taluu commented Mar 21, 2019

According to nette/di#170 (comment), it seems it should be up to the application to support it ?

@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Mar 21, 2019

I think it must be supported by Nette itself. Anyway, I never needed this, the paths of other included configs is predictable and always the same, so relative paths from the current config file can be used.

@Taluu
Copy link
Author

@Taluu Taluu commented Mar 21, 2019

Anyway, I never needed this, the paths of other included configs is predictable and always the same

Agree to disagree ? I have some inclusions (I am ignore the phpstan.neon file in my git repository, so it may be related to that...) that are not within the vendir repository so accessing through a relative path is a pain (... and I can't share the content of my file as the directory is far too personnal).

This is what I currently have :

includes:
  - /home/talus/.config/composer/vendor/phpstan/phpstan-symfony/extension.neon
  - /home/talus/.config/composer/vendor/phpstan/phpstan-doctrine/extension.neon
  - /home/talus/.config/composer/vendor/jangregor/phpstan-prophecy/src/extension.neon

So yes I can include those with the full qualified path name, but honestly, this is a pain if you ever need to share the file on a github repository (even with vendor and all, as it can vary on the setup it is used).

@mavimo
Copy link
Contributor

@mavimo mavimo commented Mar 21, 2019

@Taluu you can use a relative path from current directory, something like:

includes:
    - vendor/jangregor/phpstan-prophecy/src/extension.neon
    - vendor/phpstan/phpstan-doctrine/extension.neon
    - vendor/phpstan/phpstan-phpunit/extension.neon
    - vendor/phpstan/phpstan-phpunit/rules.neon
    - vendor/phpstan/phpstan-symfony/extension.neon
    - vendor/phpstan/phpstan-webmozart-assert/extension.neon

@Taluu
Copy link
Author

@Taluu Taluu commented Mar 21, 2019

The thing is, as I said, it is not in the current work dir's package vendor, but in the global one (for now). So I really need to use the full qualified path.

@dg
Copy link
Contributor

@dg dg commented Mar 21, 2019

I can add something like Nette\DI\Config\Loader::setParameter() for use in PHPStan\DependencyInjection\LoaderFactory if it helps.

@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Mar 21, 2019

That would be nice, but I worry that users expect to be able to expand the same parameters in include as they are in parameters...

@dg
Copy link
Contributor

@dg dg commented Mar 21, 2019

There is chicken and egg problem.

One option is to allow to use only parameters from outside (via addParameters), others will throw exception, second option is to mix parameters from outside with parameters section from current file.

@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Mar 21, 2019

Yeah I know. Second option might lead to unexpected results - %foo% from parameters section might get expanded to something else than in the include section in the same file. So we would need something deterministic :)

@dg
Copy link
Contributor

@dg dg commented Mar 21, 2019

Yes, so it would be better to allow only the first option.

@Taluu
Copy link
Author

@Taluu Taluu commented Mar 21, 2019

Restricting in the includes for at least the currentWorkDir and rootDir would be fine IMO, rather than leaving the possibility of using foos variables, which can be quickly out of control.

@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Mar 21, 2019

Yeah, agreed.

dg added a commit to nette/di that referenced this issue Mar 21, 2019
dg added a commit to nette/bootstrap that referenced this issue Mar 21, 2019
@ondrejmirtes ondrejmirtes reopened this Apr 7, 2019
@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented Apr 7, 2019

I'll add the path expanding.

@ondrejmirtes
Copy link
Member

@ondrejmirtes ondrejmirtes commented May 18, 2019

Done: 4edbe7c

@lock lock bot locked as resolved and limited conversation to collaborators Dec 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants