-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Validate env vars in config #23888
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
Changes from all commits
68ee20e
d33b1c8
e7b9656
1e59cf7
a90d34b
e5f02cd
c76adcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
use Symfony\Component\Config\Definition\Exception\ForbiddenOverwriteException; | ||
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; | ||
use Symfony\Component\Config\Definition\Exception\InvalidTypeException; | ||
use Symfony\Component\Config\Definition\Exception\UnsetKeyException; | ||
|
||
/** | ||
* The base node class. | ||
|
@@ -25,6 +26,9 @@ abstract class BaseNode implements NodeInterface | |
{ | ||
const DEFAULT_PATH_SEPARATOR = '.'; | ||
|
||
private static $placeholderUniquePrefix; | ||
private static $placeholders = array(); | ||
|
||
protected $name; | ||
protected $parent; | ||
protected $normalizationClosures = array(); | ||
|
@@ -36,6 +40,8 @@ abstract class BaseNode implements NodeInterface | |
protected $attributes = array(); | ||
protected $pathSeparator; | ||
|
||
private $handlingPlaceholder; | ||
|
||
/** | ||
* @throws \InvalidArgumentException if the name contains a period | ||
*/ | ||
|
@@ -50,6 +56,47 @@ public function __construct(?string $name, NodeInterface $parent = null, string | |
$this->pathSeparator = $pathSeparator; | ||
} | ||
|
||
/** | ||
* Register possible (dummy) values for a dynamic placeholder value. | ||
* | ||
* Matching configuration values will be processed with a provided value, one by one. After a provided value is | ||
* successfully processed the configuration value is returned as is, thus preserving the placeholder. | ||
* | ||
* @internal | ||
*/ | ||
public static function setPlaceholder(string $placeholder, array $values): void | ||
{ | ||
if (!$values) { | ||
throw new \InvalidArgumentException('At least one value must be provided.'); | ||
} | ||
|
||
self::$placeholders[$placeholder] = $values; | ||
} | ||
|
||
/** | ||
* Sets a common prefix for dynamic placeholder values. | ||
* | ||
* Matching configuration values will be skipped from being processed and are returned as is, thus preserving the | ||
* placeholder. An exact match provided by {@see setPlaceholder()} might take precedence. | ||
* | ||
* @internal | ||
*/ | ||
public static function setPlaceholderUniquePrefix(string $prefix): void | ||
{ | ||
self::$placeholderUniquePrefix = $prefix; | ||
} | ||
|
||
/** | ||
* Resets all current placeholders available. | ||
* | ||
* @internal | ||
*/ | ||
public static function resetPlaceholders(): void | ||
{ | ||
self::$placeholderUniquePrefix = null; | ||
self::$placeholders = array(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having static properties like this is very dangerous as we need to make sure that they are reset properly. I can see the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not that dangerous: placeholders are unique strings so there is not really any way to mess up with them, even if someone forgets to reset them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can settle indeed, i dont see a elegant workaround to avoid static currently, that is without overhauling the config component itself :) Now marked internal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also we deal with it properly where needed; it's not scattered all over the environment. |
||
} | ||
|
||
public function setAttribute($key, $value) | ||
{ | ||
$this->attributes[$key] = $value; | ||
|
@@ -249,8 +296,34 @@ final public function merge($leftSide, $rightSide) | |
)); | ||
} | ||
|
||
$this->validateType($leftSide); | ||
$this->validateType($rightSide); | ||
if ($leftSide !== $leftPlaceholders = self::resolvePlaceholderValue($leftSide)) { | ||
foreach ($leftPlaceholders as $leftPlaceholder) { | ||
$this->handlingPlaceholder = $leftSide; | ||
try { | ||
$this->merge($leftPlaceholder, $rightSide); | ||
} finally { | ||
$this->handlingPlaceholder = null; | ||
} | ||
} | ||
|
||
return $rightSide; | ||
} | ||
|
||
if ($rightSide !== $rightPlaceholders = self::resolvePlaceholderValue($rightSide)) { | ||
foreach ($rightPlaceholders as $rightPlaceholder) { | ||
$this->handlingPlaceholder = $rightSide; | ||
try { | ||
$this->merge($leftSide, $rightPlaceholder); | ||
} finally { | ||
$this->handlingPlaceholder = null; | ||
} | ||
} | ||
|
||
return $rightSide; | ||
} | ||
|
||
$this->doValidateType($leftSide); | ||
$this->doValidateType($rightSide); | ||
|
||
return $this->mergeValues($leftSide, $rightSide); | ||
} | ||
|
@@ -267,6 +340,20 @@ final public function normalize($value) | |
$value = $closure($value); | ||
} | ||
|
||
// resolve placeholder value | ||
if ($value !== $placeholders = self::resolvePlaceholderValue($value)) { | ||
foreach ($placeholders as $placeholder) { | ||
$this->handlingPlaceholder = $value; | ||
try { | ||
$this->normalize($placeholder); | ||
} finally { | ||
$this->handlingPlaceholder = null; | ||
} | ||
} | ||
|
||
return $value; | ||
} | ||
|
||
// replace value with their equivalent | ||
foreach ($this->equivalentValues as $data) { | ||
if ($data[0] === $value) { | ||
|
@@ -275,7 +362,7 @@ final public function normalize($value) | |
} | ||
|
||
// validate type | ||
$this->validateType($value); | ||
$this->doValidateType($value); | ||
|
||
// normalize value | ||
return $this->normalizeValue($value); | ||
|
@@ -308,7 +395,20 @@ public function getParent() | |
*/ | ||
final public function finalize($value) | ||
{ | ||
$this->validateType($value); | ||
if ($value !== $placeholders = self::resolvePlaceholderValue($value)) { | ||
foreach ($placeholders as $placeholder) { | ||
$this->handlingPlaceholder = $value; | ||
try { | ||
$this->finalize($placeholder); | ||
} finally { | ||
$this->handlingPlaceholder = null; | ||
10000 } | ||
} | ||
|
||
return $value; | ||
} | ||
|
||
$this->doValidateType($value); | ||
|
||
$value = $this->finalizeValue($value); | ||
|
||
|
@@ -318,6 +418,10 @@ final public function finalize($value) | |
try { | ||
$value = $closure($value); | ||
} catch (Exception $e) { | ||
if ($e instanceof UnsetKeyException && null !== $this->handlingPlaceholder) { | ||
continue; | ||
} | ||
|
||
throw $e; | ||
} catch (\Exception $e) { | ||
throw new InvalidConfigurationException(sprintf('Invalid configuration for path "%s": %s', $this->getPath(), $e->getMessage()), $e->getCode(), $e); | ||
|
@@ -363,4 +467,85 @@ abstract protected function mergeValues($leftSide, $rightSide); | |
* @return mixed The finalized value | ||
*/ | ||
abstract protected function finalizeValue($value); | ||
|
||
/** | ||
* Tests if placeholder values are allowed for this node. | ||
*/ | ||
protected function allowPlaceholders(): bool | ||
{ | ||
return true; | ||
} | ||
|
||
/** | ||
* Gets allowed dynamic types for this node. | ||
*/ | ||
protected function getValidPlaceholderTypes(): array | ||
{ | ||
return array(); | ||
} | ||
|
||
private static function resolvePlaceholderValue($value) | ||
{ | ||
if (\is_string($value)) { | ||
if (isset(self::$placeholders[$value])) { | ||
return self::$placeholders[$value]; | ||
} | ||
|
||
if (0 === strpos($value, self::$placeholderUniquePrefix)) { | ||
return array(); | ||
} | ||
} | ||
|
||
return $value; | ||
} | ||
|
||
private static function getType($value): string | ||
{ | ||
switch ($type = \gettype($value)) { | ||
case 'boolean': | ||
return 'bool'; | ||
case 'double': | ||
return 'float'; | ||
case 'integer': | ||
return 'int'; | ||
} | ||
|
||
return $type; | ||
} | ||
|
||
private function doValidateType($value): void | ||
{ | ||
if (null === $this->handlingPlaceholder || null === $value) { | ||
$this->validateType($value); | ||
|
||
return; | ||
} | ||
|
||
if (!$this->allowPlaceholders()) { | ||
$e = new InvalidTypeException(sprintf('A dynamic value is not compatible with a "%s" node type at path "%s".', get_class($this), $this->getPath())); | ||
$e->setPath($this->getPath()); | ||
|
||
throw $e; | ||
} | ||
|
||
$knownTypes = array_keys(self::$placeholders[$this->handlingPlaceholder]); | ||
$validTypes = $this->getValidPlaceholderTypes(); | ||
|
||
if (array_diff($knownTypes, $validTypes)) { | ||
$e = new InvalidTypeException(sprintf( | ||
'Invalid type for path "%s". Expected %s, but got %s.', | ||
$this->getPath(), | ||
1 === count($validTypes) ? '"'.reset($validTypes).'"' : 'one of "'.implode('", "', $validTypes).'"', | ||
1 === count($knownTypes) ? '"'.reset($knownTypes).'"' : 'one of "'.implode('", "', $knownTypes).'"' | ||
)); | ||
if ($hint = $this->getInfo()) { | ||
$e->addHint($hint); | ||
} | ||
$e->setPath($this->getPath()); | ||
|
||
throw $e; | ||
} | ||
|
||
$this->validateType($value); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ | |
* This class is the entry point for config normalization/merging/finalization. | ||
* | ||
* @author Johannes M. Schmitt <schmittjoh@gmail.com> | ||
* | ||
* @final since version 4.1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated to this PR? If yes, should be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure :) https://github.com/symfony/symfony/pull/23888/files#diff-e321d9e71b12d99326a74385f020e2eeR66 see also #23888 (comment) i think it's reasonable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then: missing changelog/upgrade entries There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changelog entry added. Suggestion for an upgrade note? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done :) |
||
*/ | ||
class Processor | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you made it false by default for
ArrayNode
? It seems this will limit how much of a config you can change in other peoples bundles, i.e. you can only putenv(json:FOO)
if the owner things of you in advance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments on closed PRs are likely to be lost, please consider opening a new issue/PR if you think something is buggy or can be improved. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case i asked for it on another closed PR :P But yeah.. not sure if there's any real issue now (hence i asked :D)
@mcfedr see symfony/symfony-docs#8382 (comment) , tried to explain the general proces there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, makes sense, so for simple_array nodes it will accept
env(json:FOO)
but not when it should validate the structure.