From 4c46f07f2e51adc682a5886e3dfcd15a8d7699a8 Mon Sep 17 00:00:00 2001 From: Thomas Gelf Date: Sat, 2 Sep 2017 23:29:06 +0200 Subject: [PATCH] Arguments: externalize, improve code, fix issues --- .../Director/Objects/Extension/Arguments.php | 61 ++++++++ library/Director/Objects/IcingaArguments.php | 8 +- library/Director/Objects/IcingaCommand.php | 7 +- .../Objects/IcingaCommandArgument.php | 143 ++++++++++++------ library/Director/Objects/IcingaObject.php | 78 +++------- .../Director/Objects/ObjectWithArguments.php | 18 +++ 6 files changed, 203 insertions(+), 112 deletions(-) create mode 100644 library/Director/Objects/Extension/Arguments.php create mode 100644 library/Director/Objects/ObjectWithArguments.php diff --git a/library/Director/Objects/Extension/Arguments.php b/library/Director/Objects/Extension/Arguments.php new file mode 100644 index 00000000..3acbdd34 --- /dev/null +++ b/library/Director/Objects/Extension/Arguments.php @@ -0,0 +1,61 @@ +arguments === null) { + if ($this->hasBeenLoadedFromDb()) { + $this->arguments = IcingaArguments::loadForStoredObject($this); + } else { + $this->arguments = new IcingaArguments($this); + } + } + + return $this->arguments; + } + + public function gotArguments() + { + return null !== $this->arguments; + } + + public function unsetArguments() + { + unset($this->arguments); + } + + /** + * @return string + */ + protected function renderArguments() + { + return $this->arguments()->toConfigString(); + } + + /** + * @param $value + * @return $this + */ + protected function setArguments($value) + { + $this->arguments()->setArguments($value); + return $this; + } + + /** + * @return array + */ + protected function getArguments() + { + return $this->arguments()->toPlainObject(); + } +} diff --git a/library/Director/Objects/IcingaArguments.php b/library/Director/Objects/IcingaArguments.php index 7cf330a8..e594d6fa 100644 --- a/library/Director/Objects/IcingaArguments.php +++ b/library/Director/Objects/IcingaArguments.php @@ -7,12 +7,12 @@ use Iterator; use Countable; use Icinga\Module\Director\IcingaConfig\IcingaConfigRenderer; use Icinga\Module\Director\IcingaConfig\IcingaConfigHelper as c; -use Icinga\Module\Director\Objects\IcingaCommandArgument; class IcingaArguments implements Iterator, Countable, IcingaConfigRenderer { protected $storedArguments = array(); + /** @var IcingaCommandArgument[] */ protected $arguments = array(); protected $modified = false; @@ -291,6 +291,12 @@ class IcingaArguments implements Iterator, Countable, IcingaConfigRenderer array $chosenProperties = null, $resolveIds = true ) { + if ($chosenProperties !== null) { + throw new ProgrammingError( + 'IcingaArguments does not support chosenProperties[]' + ); + } + $args = array(); foreach ($this->arguments as $arg) { if ($arg->shouldBeRemoved()) { diff --git a/library/Director/Objects/IcingaCommand.php b/library/Director/Objects/IcingaCommand.php index 6c082471..f8fe2d3e 100644 --- a/library/Director/Objects/IcingaCommand.php +++ b/library/Director/Objects/IcingaCommand.php @@ -4,9 +4,12 @@ namespace Icinga\Module\Director\Objects; use Icinga\Module\Director\IcingaConfig\IcingaConfigHelper as c; use Icinga\Module\Director\IcingaConfig\IcingaLegacyConfigHelper as c1; +use Icinga\Module\Director\Objects\Extension\Arguments; -class IcingaCommand extends IcingaObject +class IcingaCommand extends IcingaObject implements ObjectWithArguments { + use Arguments; + protected $table = 'icinga_command'; protected $type = 'CheckCommand'; @@ -28,8 +31,6 @@ class IcingaCommand extends IcingaObject protected $supportsImports = true; - protected $supportsArguments = true; - protected $supportedInLegacy = true; protected $intervalProperties = array( diff --git a/library/Director/Objects/IcingaCommandArgument.php b/library/Director/Objects/IcingaCommandArgument.php index 1f6f380c..507e01da 100644 --- a/library/Director/Objects/IcingaCommandArgument.php +++ b/library/Director/Objects/IcingaCommandArgument.php @@ -2,7 +2,7 @@ namespace Icinga\Module\Director\Objects; -use Icinga\Module\Director\Objects\IcingaObject; +use Icinga\Exception\ProgrammingError; use Icinga\Module\Director\IcingaConfig\IcingaConfigHelper as c; class IcingaCommandArgument extends IcingaObject @@ -11,6 +11,8 @@ class IcingaCommandArgument extends IcingaObject protected $table = 'icinga_command_argument'; + protected $supportsImports = false; + protected $booleans = array( 'skip_key' => 'skip_key', 'repeat_key' => 'repeat_key', @@ -65,67 +67,108 @@ class IcingaCommandArgument extends IcingaObject return $this; } + protected function makePlainArgumentValue($value, $format) + { + if ($format === 'expression') { + return (object) [ + 'type' => 'Function', + // TODO: Not for dummy comment + 'body' => $value + ]; + } else { + // json or string + return $value; + } + } + + protected function extractValueFromPlain($plain) + { + if ($plain->argument_value) { + return $this->makePlainArgumentValue( + $plain->argument_value, + $plain->argument_format + ); + } else { + return null; + } + } + + protected function transformPlainArgumentValue($plain) + { + if (property_exists($plain, 'argument_value')) { + $plain->value = $this->makePlainArgumentValue( + $plain->argument_value, + $plain->argument_format + ); + unset($plain->argument_value); + unset($plain->argument_format); + } + } + + public function toCompatPlainObject($skipDefaults = false) + { + $plain = parent::toPlainObject( + false, + $skipDefaults, + null, + false + ); + + unset($plain->id); + + $this->transformPlainArgumentValue($plain); + + // Will happen only combined with $skipDefaults + if (array_keys((array) $plain) === ['value']) { + return $plain->value; + } else { + if (property_exists($plain, 'sort_order') && $plain->sort_order !== null) { + $plain->order = $plain->sort_order; + unset($plain->sort_order); + } + + return $plain; + } + } + + public function toFullPlainObject($skipDefaults = false) + { + $plain = parent::toPlainObject( + false, + $skipDefaults, + null, + false + ); + + unset($plain->id); + unset($plain->argument_format); + + return $plain; + } + public function toPlainObject( $resolved = false, $skipDefaults = false, array $chosenProperties = null, $resolveIds = true ) { - // TODO: skipDefaults? - - $data = array(); - if ($this->argument_value) { - switch ($this->argument_format) { - case 'string': - case 'json': - $data['value'] = $this->argument_value; - break; - case 'expression': - $data['value'] = (object) array( - 'type' => 'Function', - // TODO: Not for dummy comment - 'body' => $this->argument_value - ); - break; - } + if ($resolved) { + throw new ProgrammingError( + 'A single CommandArgument cannot be resolved' + ); } - if ($this->sort_order !== null) { - $data['order'] = $this->sort_order; - } - - if ($this->set_if) { - $data['set_if'] = $this->set_if; - } - - if ($this->required !== null) { - $data['required'] = $this->required === 'y'; - } - - if ($this->repeat_key !== null) { - $data['repeat_key'] = $this->repeat_key === 'y'; - } - - if ($this->description) { - $data['description'] = $this->description; + if ($chosenProperties) { + throw new ProgrammingError( + 'IcingaCommandArgument does not support chosenProperties[]' + ); } + // $resolveIds is misused here if ($resolveIds) { - if (array_keys($data) === array('value')) { - return $data['value']; - } else { - return (object) $data; - } + return $this->toCompatPlainObject($skipDefaults); } else { - unset($data['value']); - unset($data['order']); - $data['sort_order'] = $this->sort_order; - $data['command_id'] = $this->command_id; - $data['argument_name'] = $this->argument_name; - $data['argument_value'] = $this->argument_value; - $data['argument_format'] = $this->argument_format; - $data['set_if_format'] = $this->set_if_format; - return (object) $data; + return $this->toFullPlainObject($skipDefaults); } } diff --git a/library/Director/Objects/IcingaObject.php b/library/Director/Objects/IcingaObject.php index d834a6b3..0d658c24 100644 --- a/library/Director/Objects/IcingaObject.php +++ b/library/Director/Objects/IcingaObject.php @@ -37,9 +37,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer /** @var bool Whether this Object makes use of (time) ranges */ protected $supportsRanges = false; - /** @var bool Whether this object supports (command) Arguments */ - protected $supportsArguments = false; - /** @var bool Whether inheritance via "imports" property is supported */ protected $supportsImports = false; @@ -121,8 +118,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer /** @var IcingaTimePeriodRanges - TODO: generic ranges */ private $ranges; - private $arguments; - private $shouldBeRemoved = false; private $resolveCache = array(); @@ -342,7 +337,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer $dummy->prefetchAllRelatedTypes(); } - /** * Whether this Object supports custom variables * @@ -380,7 +374,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer */ public function supportsArguments() { - return $this->supportsArguments; + return $this instanceof ObjectWithArguments; } /** @@ -515,6 +509,9 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer public function hasBeenModified() { + if (parent::hasBeenModified()) { + return true; + } $this->resolveUnresolvedRelatedProperties(); if ($this->supportsCustomVars() && $this->vars !== null && $this->vars()->hasBeenModified()) { @@ -533,7 +530,10 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer return true; } - if ($this->supportsArguments() && $this->arguments !== null && $this->arguments()->hasBeenModified()) { + if ($this instanceof ObjectWithArguments + && $this->gotArguments() + && $this->arguments()->hasBeenModified() + ) { return true; } @@ -549,7 +549,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer } } - return parent::hasBeenModified(); + return false; } protected function hasUnresolvedRelatedProperty($name) @@ -648,7 +648,9 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer //TODO: allow for deep keys $this->vars()->set(substr($key, 5), $value); return $this; - } elseif (substr($key, 0, 10) === 'arguments.') { + } elseif ($this instanceof ObjectWithArguments + && substr($key, 0, 10) === 'arguments.' + ) { $this->arguments()->set(substr($key, 10), $value); return $this; } @@ -709,17 +711,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer return $this; } - protected function setArguments($value) - { - $this->arguments()->setArguments($value); - return $this; - } - - protected function getArguments() - { - return $this->arguments()->toPlainObject(); - } - protected function getRanges() { return $this->ranges()->getValues(); @@ -804,20 +795,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer return $this->rangeClass; } - public function arguments() - { - $this->assertArgumentsSupport(); - if ($this->arguments === null) { - if ($this->hasBeenLoadedFromDb()) { - $this->arguments = IcingaArguments::loadForStoredObject($this); - } else { - $this->arguments = new IcingaArguments($this); - } - } - - return $this->arguments; - } - /** * @return IcingaObjectImports */ @@ -1218,18 +1195,6 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer return $this; } - protected function assertArgumentsSupport() - { - if (! $this->supportsArguments()) { - throw new ProgrammingError( - 'Objects of type "%s" have no arguments', - $this->getType() - ); - } - - return $this; - } - protected function assertImportsSupport() { if (! $this->supportsImports()) { @@ -1406,8 +1371,8 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer */ protected function storeArguments() { - if ($this->supportsArguments()) { - $this->arguments !== null && $this->arguments()->store(); + if ($this instanceof ObjectWithArguments) { + $this->gotArguments() && $this->arguments()->store(); } return $this; @@ -1999,11 +1964,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer */ protected function renderArguments() { - if ($this->supportsArguments()) { - return $this->arguments()->toConfigString(); - } else { - return ''; - } + return ''; } protected function renderRelatedSets() @@ -2554,8 +2515,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer } } - if ($this->supportsArguments()) { - // TODO: resolve + if ($this instanceof ObjectWithArguments) { $props['arguments'] = $this->arguments()->toPlainObject( $resolved, $skipDefaults @@ -2766,7 +2726,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer } } - if ($this->supportsArguments()) { + if ($this instanceof ObjectWithArguments) { $args = $this->arguments()->toUnmodifiedPlainObject(); if (! empty($args)) { $props['arguments'] = $args; @@ -2825,7 +2785,9 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer unset($this->groups); unset($this->imports); unset($this->ranges); - unset($this->arguments); + if ($this instanceof ObjectWithArguments) { + $this->unsetArguments(); + } parent::__destruct(); } diff --git a/library/Director/Objects/ObjectWithArguments.php b/library/Director/Objects/ObjectWithArguments.php new file mode 100644 index 00000000..2f994606 --- /dev/null +++ b/library/Director/Objects/ObjectWithArguments.php @@ -0,0 +1,18 @@ +