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

the decision whether or not to overwrite an existing schema is up to the user #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dimtrovich
Copy link

No description provided.

@colinodell
Copy link
Member

colinodell commented Jun 1, 2022

The code itself looks fine, but I'm not sure I understand the use case here. Would you mind describing why you'd want this to ignore an added schema? And with that use case, would it be better to have an expectation exception thrown instead of silently ignoring the operation?

@dimtrovich
Copy link
Author

by default, the configuration is initialized with a schema array.

it can happen that we add by mistake a new schema having the key of an already existing schema,
the problem will be that when retrieving a configuration, we will then obtain an unexpected value (due to the accidental modification of the schema)

by offering the possibility to the developer not to overwrite a schema, we are sure that the values returned will actually correspond to the first schema defined

@dimtrovich
Copy link
Author

dimtrovich commented Jun 1, 2022

I work on an internal micro framework,

the configurations are in files and we add the schemas only when we try to recover a configuration (this allows not to load the unused schema and saves time)

the fact is that there are certain predefined patterns that should not be modified, hence this idea

protected function __construct() 
{
	$this->configurator = new Configuration([
		'app' => Expect::structure([
		'base_url' => Expect::string()->default('auto'),
		'charset' => Expect::string()->default('UTF-8'),
		'environment' => Expect::string()->default('auto'),
		'language' => Expect::string()->default('en')
		])->otherItems()
	]);
}
`

`
public static function get(string $key)
{
	$configurator = self::instance()->configurator;

	if ($configurator->exists($key)) {
		return $configurator->get($key);
	}

	$config = explode('.', $key);

	// load config when we try to get it
	// first part is the name of config file
	self::load($config[0]);

	return self::instance()->configurator->get(implode('.', $config));
}
public static function load(string $config, ?string $file = null)
{
	if (!isset(self::$loaded[$config])) {
		if (empty($file)) {
			$file = self::path($config);
		}
		if (! file_exists($file)) {
			throw ConfigException::configDontExist($config, $file);
		}
		if (! in_array($file, get_included_files(), true)) {
			
			// if we try to retrieve a configuration like app.language, the system will modify 
			// the schema defined by default during the construction by Expect::mixed(), 
			// which will corrupt the expected result (the default values will be lost)
			// self::instance()->configurator->addSchema($config, Expect::mixed()); 👎
			
			// with this approach, I am sure that the initial schema will not be modified 
			// and the others can be added without problem
			self::instance()->configurator->addSchema($config, Expect::mixed(), false); 👍
			
			self::instance()->configurator->merge([$config => require($file)]);
			self::$loaded[$config] = $file;
		}
	}
}

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

Successfully merging this pull request may close these issues.

2 participants