phpro / grumphp

A PHP code-quality tool
MIT License
4.11k stars 429 forks source link

fix: Resolve dist file path from import #1134

Closed mischabraam closed 4 weeks ago

mischabraam commented 1 month ago
Q A
Branch v2.x
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Documented? no
Fixed tickets #1133
mischabraam commented 1 month ago

@veewee I've found a solution without extending like you suggested. The ContainerBuilder creates a loader and injects only the path to the phpro/grumphp/resources/config directory in the project's vendor directory. The injected path is one item in an array of paths. These paths are used in \Symfony\Component\Config\FileLocator::locate to check for the file.

I've added the project root directory here which fixes the issue.

EDIT: Better phrased. When grumphp is triggered it uses a path to the configuration file grumphp.yml (for example) in your root. You can use the -c option to use another configuration file. My change adds the directory of this configuration file as extra path to check for files. (If this makes any sense ;-) )

veewee commented 1 month ago

Thanks for looking into it @mischabraam. The change seems sensible to me. Would it limit the dist files to be in the same directory as the grumphp configuration file only?

Can you also add an E2E test to cover this specific case?

mischabraam commented 1 month ago

Sure, I'd like to. Thought about adding a test to \GrumPHPTest\E2E\ConfigurationTest. But... no clue how this is executed. Can you provide some information on how I should trigger these tests?

EDIT: nvm found it

mischabraam commented 1 month ago

@veewee I've added an E2E test.

Would it limit the dist files to be in the same directory as the grumphp configuration file only?

No, adding this path enables you to use relative paths from the project root dir. So, the following works (verified this).

# ./grumphp.yml
imports:
  - { resource: 'vendor/some/base-module/grumphp.yml.dist' }
# ./grumphp.yml
imports:
  - { resource: 'grumphp.yml.dist' }
veewee commented 1 month ago

@mischabraam looks good thanks!

I do note that you mention 'project root dir' a few times. The location of the grumphp file (provided by the CLI config option) could be located outside of the project root dir location.

See:

Figuring out what the project root dir is a process that takes op to 2 iterations. A first naive guess is being done before the container is created. A second better guess is done after the container has been created.

Currently this PR only adds the path of the config file, and not the project root dir persé. (it could be the same path though). But now I'm wondering : is this correct and "enough" ?

Another approach might be to provide the GuessedPaths object to the ContainerBuilder and add some more paths as well... Dunno :)

mischabraam commented 1 month ago

Yeah, you're right. In my projects, the grumphp.yml file is in the root dir. But yeah, I mean the path from the config file. That's the reason why I called the variable configFileDir.

veewee commented 4 weeks ago

Its probably sufficient for now. Thanks for the PR!