fix: Resolve dist file path from import

[mischabraam]

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

[veewee]

Thanks for looking into this issue!

This change requires an additional E2E unit test to make sure it works and will keep on working as expected.

[veewee]

I'm a bit confused on why this solves your issue:

The idea of this class is to be a decorator around another loader.
Every public method inside this decorator proxies to the wrapped loader.

So how does extending the FileLoader helps solving the problem?

[mischabraam]

In theory I'm not sure and you're right. In practice does this help, because the path to the .dist file is now resolved. Where the resolving didn't happen before.

I didn't take a deep dive into more code. I've added some var_dumps in all the load functions (from this DistFileLoader and the YamlFileLoader). Without the 'extend' the load function was presented with an unresolved path. With the 'extend' it was presented with a resolved path.

[mischabraam]

In addition... if you think this is not the correct way, no probs. It resolved the issue for me. But yeah, there might be a better way.

[veewee]

Probably one of the abstract FileLoader public methods is fixing the issue for you for some specific reason.

However I've got the feeling there should be a better fix indeed but haven't touched those classes in a long while so don't know without diving into it.

One thing I could think of, is that the import falls in this scenario which makes it fail:

grumphp/src/Configuration/Loader/DistFileLoader.php

Lines 35 to 37 in 16431b6

	if ($type !== null) {
	return $this->loader->supports($resource, $type);
	}

[mischabraam]

@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]

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]

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]

@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]

@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:

• https://github.com/phpro/grumphp/blob/v2.x/src/Configuration/GuessedPaths.php
• https://github.com/phpro/grumphp/blob/v2.x/src/Configuration/ContainerFactory.php

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]

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.