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

Allow for includes to contain parts to be replaced #170

Closed
Idrinth opened this issue Jul 31, 2018 · 5 comments
Closed

Allow for includes to contain parts to be replaced #170

Idrinth opened this issue Jul 31, 2018 · 5 comments

Comments

@Idrinth
Copy link

@Idrinth Idrinth commented Jul 31, 2018

While using phpstan extensions(with phpstan using nette's di and loader(@ ^2.4.7||^3.0 )) I noticed that configuring the includes across different environments and relative paths was next to impossible without manually replacing the paths each time. A reference to a specific path not related to the config file's position would have helped out here by prefixing a relative path with a path to the tool used instead of the parent file's directory.

This would allow the loader to be less dependant on always having an identical file system structure, making it easier to use.

The resulting code in Nette\DI\Config\Loader::load() could look similar to the following:

	/**
	 * Reads configuration from file.
	 * @param  string   file name
	 * @param  string   optional section to load
     * @param  string[] replacements to be done in the paths to be included
	 * @return array
	 */
	public function load($file, $section = null, $replacements = [])
	{
		// removed unrelated checks and parsing

		// include child files
		$merged = [];
		if (isset($data[self::INCLUDES_KEY])) {
			Validators::assert($data[self::INCLUDES_KEY], 'list', "section 'includes' in file '$file'");
			foreach ($data[self::INCLUDES_KEY] as $include) {
				foreach ($replacements as $search => $replace) {
					$include = str_replace("%$search%", $replace, $include);
				}
				if (!preg_match('#([a-z]+:)?[/\\\\]#Ai', $include)) {
					$include = dirname($file) . '/' . $include;
				}
				$merged = Helpers::merge($this->load($include, null, $replacements), $merged);
			}
		}
		unset($data[self::INCLUDES_KEY], $this->loadedFiles[$file]);


		return Helpers::merge($data, $merged);
	}

With the initial call (optionally) passing along replacements, for example:

$config = (new Nette\DI\Config\Loader())->load('myconfig.neon', null, ['externals-dir' => '/a/directory/elsewhere']);

I'm willing to implement this feature properly if it's desired.

@milo
Copy link
Member

@milo milo commented Jul 31, 2018

Variability in include path would make cache invalidation very hard. This is IMHO task for application to deal with it. In one of mine modular application I do:

# simplified
$includes = glob('modules/*/config.neon');
file_put_contents('modules.neon', Neon::encode(['includes' => $includes], Neon::BLOCK);

and in main NEON config:

includes:
    - modules.neon

Btw. there is already variables concept in Nette DI (like %tmpPath%/foo) but these are not expanded in includes block.

@Idrinth
Copy link
Author

@Idrinth Idrinth commented Aug 1, 2018

I agree, that it's the application's task to define how to find the configuration files. Sadly with the current implementation the easy solution(like %tmpPath%) and other variables that could be replaced is not possible without copiing most of the loader class.

Even extracting the preparation of $include into a method of it's own would make it easier to allow adjustments to paths according to a specific application's rules if adding the replacements to nette/di itself is not useful enough.

@amorimjuliana
Copy link

@amorimjuliana amorimjuliana commented Jan 11, 2019

Same issue here.

@dg
Copy link
Member

@dg dg commented Feb 19, 2019

Is solution to expand %xyz% parameters in included file names?

@Taluu
Copy link

@Taluu Taluu commented Mar 21, 2019

Bit of a bump, but in PHPStan, I have the config file (which is a neon file) that needs some placeholders resolving. See phpstan/phpstan#2020 for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants