[icinga-checkins] icinga.org: icingaweb2-module-director/feature/nested-apply-rules-12033: Forms: better error handling

git at icinga.org git at icinga.org
Fri Oct 21 10:08:06 CEST 2016


Module: icingaweb2-module-director
Branch: feature/nested-apply-rules-12033
Commit: 98708e649640adc9d7b86af8d0d88720fb267031
URL:    https://git.icinga.org/?p=icingaweb2-module-director.git;a=commit;h=98708e649640adc9d7b86af8d0d88720fb267031

Author: Thomas Gelf <thomas at gelf.net>
Date:   Fri Oct 14 13:35:30 2016 +0000

Forms: better error handling

fixes #12926

---

 .../Director/Objects/IcingaTemplateResolver.php    |    5 ++
 library/Director/Web/Form/DirectorObjectForm.php   |   66 ++++++++------------
 .../Director/Web/Form/Element/ExtensibleSet.php    |   11 ++++
 .../Director/Web/Form/IcingaObjectFieldLoader.php  |    1 -
 library/Director/Web/Form/QuickForm.php            |   20 +++++-
 public/css/module.less                             |    1 -
 6 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/library/Director/Objects/IcingaTemplateResolver.php b/library/Director/Objects/IcingaTemplateResolver.php
index b22c688..a002a01 100644
--- a/library/Director/Objects/IcingaTemplateResolver.php
+++ b/library/Director/Objects/IcingaTemplateResolver.php
@@ -2,6 +2,7 @@
 
 namespace Icinga\Module\Director\Objects;
 
+use Icinga\Exception\NotFoundError;
 use Icinga\Module\Director\Db;
 use Icinga\Module\Director\Exception\NestingError;
 
@@ -247,6 +248,10 @@ class IcingaTemplateResolver
 
     protected function getIdForName($name)
     {
+        if (! array_key_exists($name, self::$nameToId[$this->type])) {
+            throw new NotFoundError('There is no such import: "%s"', $name);
+        }
+
         return self::$nameToId[$this->type][$name];
     }
 
diff --git a/library/Director/Web/Form/DirectorObjectForm.php b/library/Director/Web/Form/DirectorObjectForm.php
index 037e176..402ad85 100644
--- a/library/Director/Web/Form/DirectorObjectForm.php
+++ b/library/Director/Web/Form/DirectorObjectForm.php
@@ -76,11 +76,12 @@ abstract class DirectorObjectForm extends QuickForm
         $object = $this->object;
 
         if (! $object instanceof IcingaObject) {
-            return $this;
+            return $this->resolvedImports = false;
         }
         if (! $object->supportsImports()) {
-            return $this;
+            return $this->resolvedImports = false;
         }
+
         if ($this->hasBeenSent()) {
             if ($el = $this->getElement('imports')) {
                 $this->populate($this->getRequest()->getPost());
@@ -93,6 +94,9 @@ abstract class DirectorObjectForm extends QuickForm
         } catch (NestingError $e) {
             $this->addUniqueErrorMessage($e->getMessage());
             return $this->resolvedImports = false;
+        } catch (Exception $e) {
+            $this->addException($e, 'imports');
+            return $this->resolvedImports = false;
         }
 
         return $this->resolvedImports = true;
@@ -194,8 +198,13 @@ abstract class DirectorObjectForm extends QuickForm
 
     protected function handleProperties($object, & $values)
     {
+        $resolve = $this->assertResolvedImports();
         if ($this->hasBeenSent()) {
             foreach ($values as $key => $value) {
+                if ($key === 'imports') {
+                    continue;
+                }
+
                 try {
                     $object->set($key, $value);
                     if ($object instanceof IcingaObject) {
@@ -203,39 +212,18 @@ abstract class DirectorObjectForm extends QuickForm
                     }
 
                 } catch (Exception $e) {
-
-                    $file = preg_split('/[\/\\\]/', $e->getFile(), -1, PREG_SPLIT_NO_EMPTY);
-                    $file = array_pop($file);
-                    $msg = sprintf(
-                        '%s (%s:%d)',
-                        $e->getMessage(),
-                        $file,
-                        $e->getLine()
-                    );
-
-                    if ($el = $this->getElement($key)) {
-                        // TODO: to be preferred $el->addError($e->getMessage());
-                        $this->addError($msg);
-                    } else {
-                        $this->addError($msg);
-                    }
+                    $this->addException($e, $key);
                 }
             }
         }
 
         if ($object instanceof IcingaObject) {
-            try {
-                $props = (array) $object->toPlainObject(
-                    false,
-                    false,
-                    null,
-                    false // Do not resolve IDs
-                );
-            } catch (NestingError $e) {
-                $this->addUniqueErrorMessage($e->getMessage());
-                $props = $object->getProperties();
-            }
-
+            $props = (array) $object->toPlainObject(
+                false,
+                false,
+                null,
+                false // Do not resolve IDs
+            );
         } else {
             $props = $object->getProperties();
             unset($props['vars']);
@@ -253,23 +241,19 @@ abstract class DirectorObjectForm extends QuickForm
             return $this;
         }
 
-        $inherited = (object) array();
-        $origins   = (object) array();
-
-        if ($object->supportsImports()) {
-            try {
-                $inherited = $object->getInheritedProperties();
-                $origins   = $object->getOriginsProperties();
-            } catch (NestingError $e) {
-                $this->addUniqueErrorMessage($e->getMessage());
-            }
+        if ($resolve) {
+            $inherited = $object->getInheritedProperties();
+            $origins   = $object->getOriginsProperties();
+        } else {
+            $inherited = (object) array();
+            $origins   = (object) array();
         }
 
         foreach ($props as $k => $v) {
             $this->setElementValue($k, $v);
             if ($k !== 'object_name' && property_exists($inherited, $k)) {
                 $el = $this->getElement($k);
-                if ($el) {
+                if ($el && $resolve) {
                     $this->setInheritedValue($el, $inherited->$k, $origins->$k);
                 }
             }
diff --git a/library/Director/Web/Form/Element/ExtensibleSet.php b/library/Director/Web/Form/Element/ExtensibleSet.php
index 38e28d0..7096203 100644
--- a/library/Director/Web/Form/Element/ExtensibleSet.php
+++ b/library/Director/Web/Form/Element/ExtensibleSet.php
@@ -31,6 +31,17 @@ class ExtensibleSet extends FormElement
     }
 
     /**
+     * We do not want one message per entry
+     *
+     * @codingStandardsIgnoreStart
+     */
+    protected function _getErrorMessages()
+    {
+        return $this->_errorMessages;
+        // @codingStandardsIgnoreEnd
+    }
+
+    /**
      * @codingStandardsIgnoreStart
      */
     protected function _filterValue(&$value, &$key)
diff --git a/library/Director/Web/Form/IcingaObjectFieldLoader.php b/library/Director/Web/Form/IcingaObjectFieldLoader.php
index a6dad1c..5076dac 100644
--- a/library/Director/Web/Form/IcingaObjectFieldLoader.php
+++ b/library/Director/Web/Form/IcingaObjectFieldLoader.php
@@ -2,7 +2,6 @@
 
 namespace Icinga\Module\Director\Web\Form;
 
-use Icinga\Module\Director\Exception\NestingError;
 use Icinga\Module\Director\Objects\IcingaObject;
 use Icinga\Module\Director\Objects\IcingaServiceSet;
 use Icinga\Module\Director\Objects\DirectorDatafield;
diff --git a/library/Director/Web/Form/QuickForm.php b/library/Director/Web/Form/QuickForm.php
index 76beab6..4bf1452 100644
--- a/library/Director/Web/Form/QuickForm.php
+++ b/library/Director/Web/Form/QuickForm.php
@@ -316,7 +316,7 @@ abstract class QuickForm extends QuickBaseForm
                     try {
                         $this->onSuccess();
                     } catch (Exception $e) {
-                        $this->addError($e->getMessage());
+                        $this->addException($e);
                         $this->onFailure();
                     }
                 } else {
@@ -332,6 +332,24 @@ abstract class QuickForm extends QuickBaseForm
         return $this;
     }
 
+    public function addException(Exception $e, $elementName = null, $withDetails = true)
+    {
+        $file = preg_split('/[\/\\\]/', $e->getFile(), -1, PREG_SPLIT_NO_EMPTY);
+        $file = array_pop($file);
+        $msg = sprintf(
+            '%s (%s:%d)',
+            $e->getMessage(),
+            $file,
+            $e->getLine()
+        );
+
+        if ($el = $this->getElement($elementName)) {
+            $el->addError($msg);
+        } else {
+            $this->addError($msg);
+        }
+    }
+
     public function onSuccess()
     {
         $this->redirectOnSuccess();
diff --git a/public/css/module.less b/public/css/module.less
index f4f3110..be73828 100644
--- a/public/css/module.less
+++ b/public/css/module.less
@@ -714,7 +714,6 @@ form textarea {
 form dd ul.errors {
     list-style-type: none;
     padding-left: 0.3em;
-    font-size: 0.857em;
 
     li {
         color: @colorCritical;



More information about the icinga-checkins mailing list