From aaa6a56146837a12cdb4fdbab88cd3a77ce6f1a0 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 16 Apr 2014 11:50:58 +0200 Subject: [PATCH 01/29] Refactor authentication config form tests and fix auth backend validation refs #6011 fixes #5712 --- application/controllers/ConfigController.php | 25 +- .../Config/Authentication/BaseBackendForm.php | 46 +-- .../Config/Authentication/DbBackendForm.php | 90 ++-- .../Config/Authentication/LdapBackendForm.php | 114 +++--- .../Config/Authentication/ReorderForm.php | 138 +++---- files | 384 ++++++++++++++++++ library/Icinga/Data/ResourceFactory.php | 5 - .../Authentication/BaseBackendFormTest.php | 61 +++ .../Authentication/DbBackendFormTest.php | 56 +++ .../Authentication/LdapBackendFormTest.php | 56 +++ .../Config/Authentication/ReorderFormTest.php | 80 ++++ .../forms/Config/AuthenticationFormTest.php | 157 ------- 12 files changed, 834 insertions(+), 378 deletions(-) create mode 100644 files create mode 100644 test/php/application/forms/Config/Authentication/BaseBackendFormTest.php create mode 100644 test/php/application/forms/Config/Authentication/DbBackendFormTest.php create mode 100644 test/php/application/forms/Config/Authentication/LdapBackendFormTest.php create mode 100644 test/php/application/forms/Config/Authentication/ReorderFormTest.php delete mode 100644 test/php/application/forms/Config/AuthenticationFormTest.php diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index 11292de74..778a086e4 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -1,5 +1,4 @@ view->tabs->activate('authentication'); $order = array_keys($config->toArray()); - $this->view->backends = array(); $this->view->messageBox = new AlertMessageBox(true); - foreach ($config as $backend=>$backendConfig) { + $backends = array(); + foreach ($order as $backend) { $form = new ReorderForm(); $form->setName('form_reorder_backend_' . $backend); - $form->setAuthenticationBackend($backend); + $form->setBackendName($backend); $form->setCurrentOrder($order); $form->setRequest($this->_request); - if (!$showOnly && $form->isSubmittedAndValid()) { + if ($form->isSubmittedAndValid()) { if ($this->writeAuthenticationFile($form->getReorderedConfig($config))) { $this->addSuccessMessage('Authentication Order Updated'); $this->redirectNow('config/authentication'); } + return; } - $this->view->backends[] = (object) array( - 'name' => $backend, - 'reorderForm' => $form + $backends[] = (object) array( + 'name' => $backend, + 'reorderForm' => $form ); } - $this->render('authentication'); + + $this->view->backends = $backends; } /** @@ -558,4 +556,3 @@ class ConfigController extends BaseConfigController } } } -// @codingStandardsIgnoreEnd diff --git a/application/forms/Config/Authentication/BaseBackendForm.php b/application/forms/Config/Authentication/BaseBackendForm.php index 1fec3a0ed..13a30957c 100644 --- a/application/forms/Config/Authentication/BaseBackendForm.php +++ b/application/forms/Config/Authentication/BaseBackendForm.php @@ -30,9 +30,10 @@ namespace Icinga\Form\Config\Authentication; use \Zend_Config; -use \Icinga\Web\Form\Decorator\HelpText; -use \Icinga\Data\ResourceFactory; -use \Icinga\Web\Form; +use \Zend_Form_Element_Checkbox; +use Icinga\Web\Form; +use Icinga\Data\ResourceFactory; +use Icinga\Web\Form\Decorator\HelpText; /** * Base form for authentication backend forms @@ -46,26 +47,26 @@ abstract class BaseBackendForm extends Form * * @var string */ - private $backendName = ''; + protected $backendName = ''; /** * The backend configuration as a Zend_Config object * * @var Zend_Config */ - private $backend; + protected $backend; /** * The resources to use instead of the factory provided ones (use for testing) * * @var Zend_Config */ - private $resources; + protected $resources; /** * Set the name of the currently displayed backend * - * @param string $name The name to be stored as the section when persisting + * @param string $name The name to be stored as the section when persisting */ public function setBackendName($name) { @@ -75,7 +76,7 @@ abstract class BaseBackendForm extends Form /** * Return the backend name of this form * - * @return string + * @return string */ public function getBackendName() { @@ -85,7 +86,7 @@ abstract class BaseBackendForm extends Form /** * Return the backend configuration or a empty Zend_Config object if none is given * - * @return Zend_Config + * @return Zend_Config */ public function getBackend() { @@ -95,7 +96,7 @@ abstract class BaseBackendForm extends Form /** * Set the backend configuration for initial population * - * @param Zend_Config $backend The backend to display in this form + * @param Zend_Config $backend The backend to display in this form */ public function setBackend(Zend_Config $backend) { @@ -104,9 +105,8 @@ abstract class BaseBackendForm extends Form /** * Set an alternative array of resources that should be used instead of the DBFactory resource set - * (used for testing) * - * @param array $resources The resources to use for populating the db selection field + * @param array $resources The resources to use for populating the db selection field */ public function setResources(array $resources) { @@ -114,9 +114,9 @@ abstract class BaseBackendForm extends Form } /** - * Return content of the resources.ini or previously set resources for displaying in the database selection field + * Return content of the resources.ini or previously set resources * - * @return array + * @return array */ public function getResources() { @@ -130,13 +130,13 @@ abstract class BaseBackendForm extends Form /** * Add checkbox at the beginning of the form which allows to skip logic connection validation */ - private function addForceCreationCheckbox() + protected function addForceCreationCheckbox() { - $checkbox = new \Zend_Form_Element_Checkbox( + $checkbox = new Zend_Form_Element_Checkbox( array( 'name' => 'backend_force_creation', - 'label' => 'Force Changes', - 'helptext' => 'Check this box to enforce changes without connectivity validation', + 'label' => t('Force Changes'), + 'helptext' => t('Check this box to enforce changes without connectivity validation'), 'order' => 0 ) ); @@ -150,16 +150,16 @@ abstract class BaseBackendForm extends Form * If logic validation fails, the 'backend_force_creation' checkbox is prepended to the form to allow users to * skip the logic connection validation. * - * @param array $data The form input to validate + * @param array $data The form input to validate * - * @return bool True when validation succeeded, false if not + * @return bool Whether validation succeeded or not */ public function isValid($data) { if (!parent::isValid($data)) { return false; } - if ($this->getRequest()->getPost('backend_force_creation')) { + if (isset($data['backend_force_creation']) && $data['backend_force_creation']) { return true; } if (!$this->isValidAuthenticationBackend()) { @@ -173,7 +173,7 @@ abstract class BaseBackendForm extends Form * Return an array containing all sections defined by this form as the key and all settings * as an key-value sub-array * - * @return array + * @return array */ abstract public function getConfig(); @@ -183,7 +183,7 @@ abstract class BaseBackendForm extends Form * An implementation should not throw any exception, but use the add/setErrorMessages method of * Zend_Form. If the 'backend_force_creation' checkbox is set, this method won't be called. * - * @return bool True when validation succeeded, otherwise false + * @return bool Whether validation succeeded or not */ abstract public function isValidAuthenticationBackend(); } diff --git a/application/forms/Config/Authentication/DbBackendForm.php b/application/forms/Config/Authentication/DbBackendForm.php index 437d4ae8c..2d04b1bcc 100644 --- a/application/forms/Config/Authentication/DbBackendForm.php +++ b/application/forms/Config/Authentication/DbBackendForm.php @@ -29,8 +29,10 @@ namespace Icinga\Form\Config\Authentication; -use \Icinga\Authentication\Backend\DbUserBackend; +use \Exception; use \Zend_Config; +use Icinga\Data\ResourceFactory; +use Icinga\Authentication\UserBackend; /** * Form class for adding/modifying database authentication backends @@ -38,21 +40,23 @@ use \Zend_Config; class DbBackendForm extends BaseBackendForm { /** - * Return a list of all database resource ready to be used as the multiOptions - * attribute in a Zend_Form_Element_Select object + * Return content of the resources.ini or previously set resources * - * @return array + * @return array */ - private function getDatabaseResources() + public function getResources() { - $backends = array(); - foreach ($this->getResources() as $resname => $resource) { - if ($resource['type'] !== 'db') { - continue; + if ($this->resources === null) { + $res = ResourceFactory::getResourceConfigs('db')->toArray(); + + foreach (array_keys($res) as $key) { + $res[$key] = $key; } - $backends[$resname] = $resname; + + return $res; + } else { + return $this->resources; } - return $backends; } /** @@ -70,9 +74,9 @@ class DbBackendForm extends BaseBackendForm 'backend_' . $name . '_name', array( 'required' => true, - 'allowEmpty' => false, - 'label' => 'Backend Name', - 'helptext' => 'The name of this authentication provider', + 'allowEmpty' => false, + 'label' => t('Backend Name'), + 'helptext' => t('The name of this authentication provider'), 'value' => $this->getBackendName() ) ); @@ -81,12 +85,12 @@ class DbBackendForm extends BaseBackendForm 'select', 'backend_' . $name . '_resource', array( - 'label' => 'Database Connection', - 'required' => true, - 'allowEmpty' => false, - 'helptext' => 'The database connection to use for authenticating with this provider', - 'value' => $this->getBackend()->get('resource'), - 'multiOptions' => $this->getDatabaseResources() + 'required' => true, + 'allowEmpty' => false, + 'label' => t('Database Connection'), + 'helptext' => t('The database connection to use for authenticating with this provider'), + 'value' => $this->getBackend()->get('resource'), + 'multiOptions' => $this->getResources() ) ); @@ -112,53 +116,39 @@ class DbBackendForm extends BaseBackendForm */ public function getConfig() { - $name = $this->getBackendName(); - $prefix = 'backend_' . $this->filterName($name) . '_'; - + $prefix = 'backend_' . $this->filterName($this->getBackendName()) . '_'; $section = $this->getValue($prefix . 'name'); $cfg = array( - 'backend' => 'db', - 'target' => 'user', - 'resource' => $this->getValue($prefix . 'resource'), - ); - return array( - $section => $cfg + 'backend' => 'db', + 'resource' => $this->getValue($prefix . 'resource'), ); + + return array($section => $cfg); } /** * Validate the current configuration by creating a backend and requesting the user count * - * @return bool True when the backend is valid, false otherwise + * @return bool Whether validation succeeded or not + * * @see BaseBackendForm::isValidAuthenticationBackend */ public function isValidAuthenticationBackend() { - // @TODO fix validation of authentication backends (AK #5712) - return true; try { - $name = $this->getBackendName(); - $dbBackend = new DbUserBackend( - new Zend_Config( - array( - 'backend' => 'db', - 'target' => 'user', - 'resource' => $this->getValue('backend_' . $this->filterName($name) . '_resource'), - ) - ) - ); - $dbBackend->connect(); - if ($dbBackend->getUserCount() < 1) { - $this->addErrorMessage("No users found under the specified database backend"); + $cfg = $this->getConfig(); + $backendName = key($cfg); + $testConn = UserBackend::create($backendName, new Zend_Config($cfg[$backendName])); + + if ($testConn->count() === 0) { + $this->addErrorMessage(t('No users found under the specified database backend')); return false; } - } catch (\Exception $e) { - $this->addErrorMessage("Using the specified backend failed: " . $e->getMessage()); - return false; - } catch (\Zend_Db_Statement_Exception $e) { - $this->addErrorMessage("Using the specified backend failed: " . $e->getMessage()); + } catch (Exception $e) { + $this->addErrorMessage(sprintf(t('Using the specified backend failed: %s'), $e->getMessage())); return false; } + return true; } } diff --git a/application/forms/Config/Authentication/LdapBackendForm.php b/application/forms/Config/Authentication/LdapBackendForm.php index d52cf451b..396212361 100644 --- a/application/forms/Config/Authentication/LdapBackendForm.php +++ b/application/forms/Config/Authentication/LdapBackendForm.php @@ -30,11 +30,10 @@ namespace Icinga\Form\Config\Authentication; use \Exception; -use Icinga\Data\ResourceFactory; use \Zend_Config; -use \Icinga\Web\Form; -use \Icinga\Authentication\Backend\LdapUserBackend; -use \Icinga\Protocol\Ldap\Connection as LdapConnection; +use Icinga\Web\Form; +use Icinga\Data\ResourceFactory; +use Icinga\Authentication\UserBackend; /** * Form for adding or modifying LDAP authentication backends @@ -42,14 +41,31 @@ use \Icinga\Protocol\Ldap\Connection as LdapConnection; class LdapBackendForm extends BaseBackendForm { /** - * Create this form and add all required elements + * Return content of the resources.ini or previously set resources * - * @param $options Only useful for testing purposes: - * 'resources' => All available resources. + * @return array + */ + public function getResources() + { + if ($this->resources === null) { + $res = ResourceFactory::getResourceConfigs('ldap')->toArray(); + + foreach (array_keys($res) as $key) { + $res[$key] = $key; + } + + return $res; + } else { + return $this->resources; + } + } + + /** + * Create this form and add all required elements * * @see Form::create() */ - public function create($options = array()) + public function create() { $this->setName('form_modify_backend'); $name = $this->filterName($this->getBackendName()); @@ -57,12 +73,12 @@ class LdapBackendForm extends BaseBackendForm $this->addElement( 'text', - 'backend_'.$name.'_name', + 'backend_' . $name . '_name', array( 'required' => true, - 'allowEmpty' => false, - 'label' => 'Backend Name', - 'helptext' => 'The name of this authentication backend', + 'allowEmpty' => false, + 'label' => t('Backend Name'), + 'helptext' => t('The name of this authentication backend'), 'value' => $this->getBackendName() ) ); @@ -71,13 +87,12 @@ class LdapBackendForm extends BaseBackendForm 'select', 'backend_' . $name . '_resource', array( - 'label' => 'Database Connection', - 'required' => true, - 'allowEmpty' => false, - 'helptext' => 'The database connection to use for authenticating with this provider', - 'value' => $this->getBackend()->get('resource'), - 'multiOptions' => array_key_exists('resources', $options) ? - $options['resources'] : $this->getLdapResources() + 'required' => true, + 'allowEmpty' => false, + 'label' => t('LDAP resource'), + 'helptext' => t('The resource to use for authenticating with this provider'), + 'value' => $this->getBackend()->get('resource'), + 'multiOptions' => $this->getResources() ) ); @@ -85,10 +100,10 @@ class LdapBackendForm extends BaseBackendForm 'text', 'backend_' . $name . '_user_class', array( - 'label' => 'LDAP User Object Class', - 'value' => $backend->get('user_class', 'inetOrgPerson'), - 'helptext' => 'The object class used for storing users on the ldap server', - 'required' => true + 'required' => true, + 'label' => t('LDAP User Object Class'), + 'helptext' => t('The object class used for storing users on the ldap server'), + 'value' => $backend->get('user_class', 'inetOrgPerson') ) ); @@ -96,10 +111,10 @@ class LdapBackendForm extends BaseBackendForm 'text', 'backend_' . $name . '_user_name_attribute', array( - 'label' => 'LDAP User Name Attribute', - 'value' => $backend->get('user_name_attribute', 'uid'), - 'helptext' => 'The attribute name used for storing the user name on the ldap server', - 'required' => true + 'required' => true, + 'label' => t('LDAP User Name Attribute'), + 'helptext' => t('The attribute name used for storing the user name on the ldap server'), + 'value' => $backend->get('user_name_attribute', 'uid') ) ); @@ -125,52 +140,41 @@ class LdapBackendForm extends BaseBackendForm */ public function getConfig() { - $name = $this->getBackendName(); - $prefix = 'backend_' . $this->filterName($name) . '_'; - + $prefix = 'backend_' . $this->filterName($this->getBackendName()) . '_'; $section = $this->getValue($prefix . 'name'); $cfg = array( - 'target' => 'user', - 'resource' => $this->getValue($prefix . 'resource'), - 'user_class' => $this->getValue($prefix . 'user_class'), - 'user_name_attribute' => $this->getValue($prefix . 'user_name_attribute') - ); - return array( - $section => $cfg + 'backend' => 'ldap', + 'resource' => $this->getValue($prefix . 'resource'), + 'user_class' => $this->getValue($prefix . 'user_class'), + 'user_name_attribute' => $this->getValue($prefix . 'user_name_attribute') ); + + return array($section => $cfg); } /** * Validate the current configuration by creating a backend and requesting the user count * - * @return bool True when the backend is valid, false otherwise + * @return bool Whether validation succeeded or not + * * @see BaseBackendForm::isValidAuthenticationBacken */ public function isValidAuthenticationBackend() { try { $cfg = $this->getConfig(); - $backendName = 'backend_' . $this->filterName($this->getBackendName()) . '_name'; - $backendConfig = new Zend_Config($cfg[$this->getValue($backendName)]); - $testConn = new LdapUserBackend($backendConfig); - if ($testConn->getUserCount() === 0) { - throw new Exception('No Users Found On Directory Server'); + $backendName = key($cfg); + $testConn = UserBackend::create($backendName, new Zend_Config($cfg[$backendName])); + + if ($testConn->count() === 0) { + $this->addErrorMessage(t('No users found on directory server')); + return false; } } catch (Exception $exc) { - $this->addErrorMessage( - 'Connection Validation Failed:' . $exc->getMessage() - ); + $this->addErrorMessage(sprintf(t('Connection validation failed: %s'), $exc->getMessage())); return false; } + return true; } - - private function getLdapResources() - { - $res = ResourceFactory::getResourceConfigs('ldap')->toArray(); - foreach ($res as $key => $value) { - $res[$key] = $key; - } - return $res; - } } diff --git a/application/forms/Config/Authentication/ReorderForm.php b/application/forms/Config/Authentication/ReorderForm.php index f81ee5b0d..b6a3b81f0 100644 --- a/application/forms/Config/Authentication/ReorderForm.php +++ b/application/forms/Config/Authentication/ReorderForm.php @@ -30,12 +30,10 @@ namespace Icinga\Form\Config\Authentication; use \Zend_Config; -use \Icinga\Web\Form; -use \Icinga\Web\Url; +use Icinga\Web\Form; /** - * Form for modifying the authentication provider order. - * + * Form for modifying the authentication provider order */ class ReorderForm extends Form { @@ -44,19 +42,20 @@ class ReorderForm extends Form * * @var string */ - private $backend = null; + protected $backend; /** * The current ordering of all backends, required to determine possible changes * * @var array */ - private $currentOrder = array(); + protected $currentOrder = array(); /** * Set an array with the current order of all backends * - * @param array $order An array containing backend names in the order they are defined in the authentication.ini + * @param array $order An array containing backend names in the order + * they are defined in the authentication.ini */ public function setCurrentOrder(array $order) { @@ -66,20 +65,17 @@ class ReorderForm extends Form /** * Set the name of the authentication backend for which to create the form * - * @param string $backend The name of the authentication backend + * @param string $backend The name of the authentication backend */ - public function setAuthenticationBackend($backend) + public function setBackendName($backend) { $this->backend = $backend; } /** - * Return the name of the currently set backend as it will appear in the forms + * Return the name of the currently set backend as it will appear in the form * - * This calls the Zend Filtername function in order to filter specific chars - * - * @return string The filtered name of the backend - * @see Form::filterName() + * @return string The name of the backend */ public function getBackendName() { @@ -87,28 +83,24 @@ class ReorderForm extends Form } /** - * Create this form. - * - * Note: The form action will be set here to the authentication overview + * Create this form * * @see Form::create */ public function create() { - $this->upForm = new Form(); - $this->downForm = new Form(); - if ($this->moveElementUp($this->backend, $this->currentOrder) !== $this->currentOrder) { + $upForm = new Form(); - $this->upForm->addElement( + $upForm->addElement( 'hidden', 'form_backend_order', array( 'required' => true, - 'value' => join(',', $this->moveElementUp($this->backend, $this->currentOrder)) + 'value' => join(',', $this->moveElementUp($this->backend, $this->currentOrder)) ) ); - $this->upForm->addElement( + $upForm->addElement( 'button', 'btn_' . $this->getBackendName() . '_reorder_up', array( @@ -116,21 +108,25 @@ class ReorderForm extends Form 'escape' => false, 'value' => 'btn_' . $this->getBackendName() . '_reorder_up', 'name' => 'btn_' . $this->getBackendName() . '_reorder_up', - 'label' => $this->getView()->icon('up.png', 'Move up in authentication order'), + 'label' => $this->getView()->icon('up.png', t('Move up in authentication order')) ) ); + + $this->addSubForm($upForm, 'btn_reorder_up'); } if ($this->moveElementDown($this->backend, $this->currentOrder) !== $this->currentOrder) { - $this->downForm->addElement( + $downForm = new Form(); + + $downForm->addElement( 'hidden', 'form_backend_order', array( 'required' => true, - 'value' => join(',', $this->moveElementDown($this->backend, $this->currentOrder)) + 'value' => join(',', $this->moveElementDown($this->backend, $this->currentOrder)) ) ); - $this->downForm->addElement( + $downForm->addElement( 'button', 'btn_' . $this->getBackendName() . '_reorder_down', array( @@ -138,72 +134,68 @@ class ReorderForm extends Form 'escape' => false, 'value' => 'btn_' . $this->getBackendName() . '_reorder_down', 'name' => 'btn_' . $this->getBackendName() . '_reorder_down', - 'label' => $this->getView()->icon('down.png', 'Move down in authentication order'), - + 'label' => $this->getView()->icon('down.png', t('Move down in authentication order')) ) ); + + $this->addSubForm($downForm, 'btn_reorder_down'); } - $this->setAction(Url::fromPath("config/authentication", array())->getAbsoluteUrl()); } /** - * Return the result of $this->getValues but flatten the result + * Return the flattened result of $this->getValues * - * The result will be a key=>value array without subarrays + * @return array The currently set values * - * @param bool $supressArrayNotation passed to getValues - * - * @return array The currently set values * @see Form::getValues() */ - public function getFlattenedValues($supressArrayNotation = false) + protected function getFlattenedValues() { - $values = parent::getValues($supressArrayNotation); $result = array(); - foreach ($values as $key => &$value) { + foreach (parent::getValues() as $key => $value) { if (is_array($value)) { $result += $value; } else { $result[$key] = $value; } } + return $result; } /** * Determine whether this form is submitted by testing the submit buttons of both subforms * - * @return bool True when the form is submitted, otherwise false + * @return bool Whether the form has been submitted or not */ public function isSubmitted() { $checkData = $this->getRequest()->getParams(); - return isset ($checkData['btn_' . $this->getBackendName() . '_reorder_up']) || - isset ($checkData['btn_' . $this->getBackendName() . '_reorder_down']); + return isset($checkData['btn_' . $this->getBackendName() . '_reorder_up']) || + isset($checkData['btn_' . $this->getBackendName() . '_reorder_down']); } /** - * Return the reordered configuration after a reorder button has been submited + * Return the reordered configuration after a reorder button has been submitted * - * @param Zend_Config $config The configuration to reorder + * @param Zend_Config $config The configuration to reorder * - * @return array An array containing the reordered configuration + * @return array An array containing the reordered configuration */ public function getReorderedConfig(Zend_Config $config) { $originalConfig = $config->toArray(); - $reordered = array(); $newOrder = $this->getFlattenedValues(); $order = explode(',', $newOrder['form_backend_order']); + + $reordered = array(); foreach ($order as $key) { - if (!isset($originalConfig[$key])) { - continue; + if (isset($originalConfig[$key])) { + $reordered[$key] = $originalConfig[$key]; } - $reordered[$key] = $originalConfig[$key]; } return $reordered; - } /** @@ -216,28 +208,27 @@ class ReorderForm extends Form * moveElementUp('third', $array); // returns ['first', 'third', 'second'] * * - * @param string $key The key to bubble up one slot - * @param array $array The array to work with + * @param string $key The key to bubble up one slot + * @param array $array The array to work with * - * @return array The modified array + * @return array The modified array */ - private static function moveElementUp($key, array $array) + protected static function moveElementUp($key, array $array) { - $swap = null; - for ($i=0; $i * - * @param string $key The key to bubble up one slot - * @param array $array The array to work with + * @param string $key The key to bubble up one slot + * @param array $array The array to work with * - * @return array The modified array + * @return array The modified array */ - private static function moveElementDown($key, array $array) + protected static function moveElementDown($key, array $array) { - $swap = null; - for ($i=0; $iis_valid; + } +} + +class BaseBackendFormTest extends BaseTestCase +{ + public function testIsForceCreationCheckboxBeingAdded() + { + $form = new BackendForm(); + $form->is_valid = false; + + $this->assertFalse($form->isValid(array())); + $this->assertNotNull( + $form->getElement('backend_force_creation'), + 'Checkbox to force a backend\'s creation is not being added though the backend is invalid' + ); + } + + public function testIsForceCreationCheckboxNotBeingAdded() + { + $form = new BackendForm(); + $form->is_valid = true; + + $this->assertTrue($form->isValid(array())); + $this->assertNull( + $form->getElement('backend_force_creation'), + 'Checkbox to force a backend\'s creation is being added though the backend is valid' + ); + } + + public function testIsTheFormValidIfForceCreationTrue() + { + $form = new BackendForm(); + $form->is_valid = false; + + $this->assertTrue( + $form->isValid(array('backend_force_creation' => 1)), + 'BaseBackendForm with invalid backend is not valid though force creation is set' + ); + } +} diff --git a/test/php/application/forms/Config/Authentication/DbBackendFormTest.php b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php new file mode 100644 index 000000000..8e9488450 --- /dev/null +++ b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php @@ -0,0 +1,56 @@ +shouldReceive('create')->with('test', Mockery::type('\Zend_Config'))->andReturnUsing( + function () { return Mockery::mock(array('count' => 1)); } + ); + + $form = new DbBackendForm(); + $form->setBackendName('test'); + $form->setResources(array('test_db_backend' => null)); + $form->create(); + $form->populate(array('backend_test_resource' => 'test_db_backend')); + + $this->assertTrue( + $form->isValidAuthenticationBackend(), + 'DbBackendForm claims that a valid authentication backend with users is not valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testInvalidBackendIsNotValid() + { + Mockery::mock('alias:Icinga\Authentication\UserBackend') + ->shouldReceive('create')->with('test', Mockery::type('\Zend_Config'))->andReturnUsing( + function () { return Mockery::mock(array('count' => 0)); } + ); + + $form = new DbBackendForm(); + $form->setBackendName('test'); + $form->setResources(array('test_db_backend' => null)); + $form->create(); + $form->populate(array('backend_test_resource' => 'test_db_backend')); + + $this->assertFalse( + $form->isValidAuthenticationBackend(), + 'DbBackendForm claims that an invalid authentication backend without users is valid' + ); + } +} diff --git a/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php new file mode 100644 index 000000000..9493eda47 --- /dev/null +++ b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php @@ -0,0 +1,56 @@ +shouldReceive('create')->with('test', Mockery::type('\Zend_Config'))->andReturnUsing( + function () { return Mockery::mock(array('count' => 1)); } + ); + + $form = new LdapBackendForm(); + $form->setBackendName('test'); + $form->setResources(array('test_ldap_backend' => null)); + $form->create(); + $form->populate(array('backend_test_resource' => 'test_ldap_backend')); + + $this->assertTrue( + $form->isValidAuthenticationBackend(), + 'LdapBackendForm claims that a valid authentication backend with users is not valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testInvalidBackendIsNotValid() + { + Mockery::mock('alias:Icinga\Authentication\UserBackend') + ->shouldReceive('create')->with('test', Mockery::type('\Zend_Config'))->andReturnUsing( + function () { return Mockery::mock(array('count' => 0)); } + ); + + $form = new LdapBackendForm(); + $form->setBackendName('test'); + $form->setResources(array('test_ldap_backend' => null)); + $form->create(); + $form->populate(array('backend_test_resource' => 'test_ldap_backend')); + + $this->assertFalse( + $form->isValidAuthenticationBackend(), + 'LdapBackendForm claims that an invalid authentication backend without users is valid' + ); + } +} diff --git a/test/php/application/forms/Config/Authentication/ReorderFormTest.php b/test/php/application/forms/Config/Authentication/ReorderFormTest.php new file mode 100644 index 000000000..a61efc48a --- /dev/null +++ b/test/php/application/forms/Config/Authentication/ReorderFormTest.php @@ -0,0 +1,80 @@ + $this->order); + } +} + +class ReorderFormTest extends BaseTestCase +{ + public function setUp() + { + parent::setUp(); + $this->viewMock = Mockery::mock('\Zend_View'); + $this->viewMock->shouldReceive('icon')->andReturn(''); + } + + public function testMoveBackendUp() + { + $config = new Zend_Config( + array( + 'test1' => '', + 'test2' => '', + 'test3' => '' + ) + ); + $oldOrder = array_keys($config->toArray()); + + $form = new RequestLessReorderForm(); + $form->setCurrentOrder($oldOrder); + $form->setBackendName('test3'); + $form->setView($this->viewMock); + $form->create(); + + $form->order = $form->getSubForm('btn_reorder_up')->getElement('form_backend_order')->getValue(); + $this->assertSame( + $form->getReorderedConfig($config), + array('test1' => '', 'test3' => '', 'test2' => ''), + 'Moving elements up with ReorderForm does not seem to properly work' + ); + } + + public function testMoveBackendDown() + { + $config = new Zend_Config( + array( + 'test1' => '', + 'test2' => '', + 'test3' => '' + ) + ); + $oldOrder = array_keys($config->toArray()); + + $form = new RequestLessReorderForm(); + $form->setCurrentOrder($oldOrder); + $form->setBackendName('test1'); + $form->setView($this->viewMock); + $form->create(); + + $form->order = $form->getSubForm('btn_reorder_down')->getElement('form_backend_order')->getValue(); + $this->assertSame( + $form->getReorderedConfig($config), + array('test2' => '', 'test1' => '', 'test3' => ''), + 'Moving elements down with ReorderForm does not seem to properly work' + ); + } +} diff --git a/test/php/application/forms/Config/AuthenticationFormTest.php b/test/php/application/forms/Config/AuthenticationFormTest.php deleted file mode 100644 index 45c27b062..000000000 --- a/test/php/application/forms/Config/AuthenticationFormTest.php +++ /dev/null @@ -1,157 +0,0 @@ -viewMock = Mockery::mock('\Zend_View'); - $this->viewMock->shouldReceive('icon')->andReturn(''); - } - - /** - * Test the ldap provider form population from config - */ - public function testLdapProvider() - { - $form = $this->createForm('Icinga\Form\Config\Authentication\LdapBackendForm'); - $config = new Zend_Config( - array( - 'backend' => 'ldap', - 'target' => 'user', - 'user_class' => 'testClass', - 'user_name_attribute' => 'testAttribute' - ) - ); - $form->setBackendName('testldap'); - $form->setBackend($config); - $form->create(array('resources' => array())); - - // parameters to be hidden - $notShown = array('backend', 'target'); - foreach ($config->toArray() as $name => $value) { - if (in_array($name, $notShown)) { - continue; - } - $this->assertEquals( - $value, - $form->getValue('backend_testldap_' . $name), - 'Asserting the ' . $name . ' parameter to be correctly populated for a ldap authentication form' - ); - } - } - - /** - * Test the database provider form population from config - */ - public function testDbProvider() - { - $form = $this->createForm('Icinga\Form\Config\Authentication\DbBackendForm'); - $config = new Zend_Config( - array( - 'backend' => 'db', - 'target' => 'user', - 'resource' => 'db_resource' - ) - ); - $form->setResources( - array( - 'db_resource' => array( - 'type' => 'db' - ) - ) - ); - - $form->setBackendName('test-db'); - $form->setBackend($config); - $form->create(); - - // parameters to be hidden - $notShown = array('backend', 'target'); - foreach ($config->toArray() as $name => $value) { - if (in_array($name, $notShown)) { - continue; - } - $this->assertEquals( - $value, - $form->getValue('backend_testdb_' . $name), - 'Asserting the ' . $name . ' parameter to be correctly populated for a db authentication form' - ); - } - } - - /** - * Test whether order modifications via 'priority' are considered - */ - public function testModifyOrder() - { - $form = $this->createForm('Icinga\Form\Config\Authentication\ReorderForm'); - $form->setAuthenticationBackend('backend2'); - $form->setCurrentOrder(array('backend1', 'backend2', 'backend3', 'backend4')); - $form->setView($this->viewMock); - - $form->create(); - $this->assertSame( - 2, - count($form->getSubForms()), - 'Assert that a form for moving backend up and down exists' - ); - $this->assertTrue( - $form->upForm->getElement('form_backend_order') !== null, - 'Assert that a "move backend up" button exists' - ); - $this->assertSame( - array('backend2', 'backend1', 'backend3', 'backend4'), - explode(',', $form->upForm->getElement('form_backend_order')->getValue()), - 'Assert the "move backend up" button containing the correct order' - ); - - $this->assertTrue( - $form->downForm->getElement('form_backend_order') !== null, - 'Assert that a "move backend down" button exists' - ); - $this->assertSame( - array('backend1', 'backend3', 'backend2', 'backend4'), - explode(',', $form->downForm->getElement('form_backend_order')->getValue()), - 'Assert the "move backend up" button containing the correct order' - ); - } - - /** - * Test whether the reorder form doesn't display senseless ordering (like moving the uppermost element up or - * the lowermost down) - */ - public function testInvalidOrderingNotShown() - { - $form = $this->createForm('Icinga\Form\Config\Authentication\ReorderForm'); - $form->setAuthenticationBackend('backend1'); - $form->setCurrentOrder(array('backend1', 'backend2', 'backend3', 'backend4')); - $form->setView($this->viewMock); - - $form->create(); - $this->assertSame( - 2, - count($form->getSubForms()), - 'Assert that a form for moving backend up and down exists, even when moving up is not possible' - ); - $this->assertTrue( - $form->downForm->getElement('form_backend_order') !== null, - 'Assert that a "move backend down" button exists when moving up is not possible' - ); - $this->assertTrue( - $form->upForm->getElement('form_backend_order') === null, - 'Assert that a "move backend up" button does not exist when moving up is not possible' - ); - } -} From 656e6bd0fd8e418d3daacb0ba7e8da6f93771b57 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 16 Apr 2014 12:46:07 +0200 Subject: [PATCH 02/29] Drop obsolete CreateResourceForm class refs #6011 --- .../forms/Config/Resource/CreateResourceForm.php | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 application/forms/Config/Resource/CreateResourceForm.php diff --git a/application/forms/Config/Resource/CreateResourceForm.php b/application/forms/Config/Resource/CreateResourceForm.php deleted file mode 100644 index a688ae62f..000000000 --- a/application/forms/Config/Resource/CreateResourceForm.php +++ /dev/null @@ -1,13 +0,0 @@ - Date: Wed, 16 Apr 2014 12:58:45 +0200 Subject: [PATCH 03/29] Rename EditResourceForm to ResourceForm refs #6011 --- application/controllers/ConfigController.php | 6 +++--- .../Resource/{EditResourceForm.php => ResourceForm.php} | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) rename application/forms/Config/Resource/{EditResourceForm.php => ResourceForm.php} (99%) diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index 778a086e4..fe73ea22c 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -37,7 +37,7 @@ use \Icinga\Form\Config\GeneralForm; use \Icinga\Form\Config\Authentication\ReorderForm; use \Icinga\Form\Config\Authentication\LdapBackendForm; use \Icinga\Form\Config\Authentication\DbBackendForm; -use \Icinga\Form\Config\Resource\EditResourceForm; +use \Icinga\Form\Config\Resource\ResourceForm; use \Icinga\Form\Config\LoggingForm; use \Icinga\Form\Config\ConfirmRemovalForm; use \Icinga\Config\PreservingIniWriter; @@ -396,7 +396,7 @@ class ConfigController extends BaseConfigController { $this->view->resourceTypes = $this->resourceTypes; $resources = IcingaConfig::app('resources', true); - $form = new EditResourceForm(); + $form = new ResourceForm(); $form->setRequest($this->_request); if ($form->isSubmittedAndValid()) { $name = $form->getName(); @@ -428,7 +428,7 @@ class ConfigController extends BaseConfigController $this->render('resource/modify'); return; } - $form = new EditResourceForm(); + $form = new ResourceForm(); if ($this->_request->isPost() === false) { $form->setOldName($name); $form->setName($name); diff --git a/application/forms/Config/Resource/EditResourceForm.php b/application/forms/Config/Resource/ResourceForm.php similarity index 99% rename from application/forms/Config/Resource/EditResourceForm.php rename to application/forms/Config/Resource/ResourceForm.php index bbc9d5ea9..0568c4a96 100644 --- a/application/forms/Config/Resource/EditResourceForm.php +++ b/application/forms/Config/Resource/ResourceForm.php @@ -38,7 +38,7 @@ use Icinga\Data\ResourceFactory; /** * Form for modifying a monitoring backend */ -class EditResourceForm extends Form +class ResourceForm extends Form { /** * The currently edited resource. From 21b949758f0046a287a9da8c02860d5aa285cfd3 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 16 Apr 2014 16:41:23 +0200 Subject: [PATCH 04/29] Refactor ResourceForm tests refs #6011 --- .../Config/Authentication/LdapBackendForm.php | 2 +- .../forms/Config/Resource/ResourceForm.php | 355 ++++++++++-------- .../Icinga/Protocol/Livestatus/Connection.php | 9 + library/Icinga/Web/Form/Element/Number.php | 28 +- .../Authentication/DbBackendFormTest.php | 8 +- .../Authentication/LdapBackendFormTest.php | 8 +- .../Config/Resource/ResourceFormTest.php | 258 +++++++++++++ 7 files changed, 509 insertions(+), 159 deletions(-) create mode 100644 test/php/application/forms/Config/Resource/ResourceFormTest.php diff --git a/application/forms/Config/Authentication/LdapBackendForm.php b/application/forms/Config/Authentication/LdapBackendForm.php index 396212361..15ceac0ad 100644 --- a/application/forms/Config/Authentication/LdapBackendForm.php +++ b/application/forms/Config/Authentication/LdapBackendForm.php @@ -89,7 +89,7 @@ class LdapBackendForm extends BaseBackendForm array( 'required' => true, 'allowEmpty' => false, - 'label' => t('LDAP resource'), + 'label' => t('LDAP Resource'), 'helptext' => t('The resource to use for authenticating with this provider'), 'value' => $this->getBackend()->get('resource'), 'multiOptions' => $this->getResources() diff --git a/application/forms/Config/Resource/ResourceForm.php b/application/forms/Config/Resource/ResourceForm.php index 0568c4a96..b8d82c600 100644 --- a/application/forms/Config/Resource/ResourceForm.php +++ b/application/forms/Config/Resource/ResourceForm.php @@ -29,40 +29,41 @@ namespace Icinga\Form\Config\Resource; -use \Zend_Config; +use Exception; +use Zend_Config; +use Zend_Form_Element_Checkbox; use Icinga\Web\Form; -use Icinga\Logger\Logger; -use Icinga\Web\Form\Decorator\HelpText; use Icinga\Data\ResourceFactory; +use Icinga\Web\Form\Element\Number; +use Icinga\Web\Form\Decorator\HelpText; -/** - * Form for modifying a monitoring backend - */ class ResourceForm extends Form { /** - * The currently edited resource. + * The resource * * @var Zend_Config */ - private $resource; + protected $resource; /** + * The (new) name of the resource + * * @var string */ - private $name = ''; + protected $name; /** + * The old name of the resource + * * @var string */ - private $oldName = ''; + protected $oldName; /** - * Return the current resource name. + * Set the current resource name * - * @param string $name - * - * @return void|\Zend_Form + * @param string $name The name to set */ public function setName($name) { @@ -70,18 +71,24 @@ class ResourceForm extends Form } /** + * Get the current resource name + * * @return null|string */ public function getName() { - return $this->getValue('resource_all_name'); + $name = $this->getValue('resource_all_name'); + if (!$name) { + return $this->name; + } + + return $name; } /** - * Set the original name of the resource. This value is persisted using - * a hidden field. + * Set the original name of the resource * - * @param $name + * @param string $name The name to set */ public function setOldName($name) { @@ -89,24 +96,55 @@ class ResourceForm extends Form } /** - * Get the resource name that was initially set. + * Get the resource name that was initially set + * + * @return null|string */ public function getOldName() { - return $this->getValue('resource_all_name_old'); + $oldName = $this->getValue('resource_all_name_old'); + if (!$oldName) { + return $this->oldName; + } + + return $oldName; } - private function addDbForm() + /** + * Set the resource configuration to edit. + * + * @param Zend_Config $resource The config to set + */ + public function setResource(Zend_Config $resource) + { + $this->resource = $resource; + } + + /** + * Get the current resource configuration. + * + * @return Zend_Config + */ + public function getResource() + { + if (!isset($this->resource)) { + $this->resource = new Zend_Config(array('type' => 'db')); + } + + return $this->resource; + } + + protected function addDbForm() { $this->addElement( 'select', 'resource_db_db', array( - 'label' => 'Database Type', - 'value' => $this->getResource()->get('db', 'mysql'), - 'required' => true, - 'helptext' => 'The type of SQL database you want to create.', - 'multiOptions' => array( + 'required' => true, + 'label' => t('Database Type'), + 'helptext' => t('The type of SQL database you want to create.'), + 'value' => $this->getResource()->get('db', 'mysql'), + 'multiOptions' => array( 'mysql' => 'MySQL', 'pgsql' => 'PostgreSQL' //'oracle' => 'Oracle' @@ -118,24 +156,22 @@ class ResourceForm extends Form 'text', 'resource_db_host', array ( - 'label' => 'Host', - 'value' => $this->getResource()->get('host', 'localhost'), - 'required' => true, - 'helptext' => 'The hostname of the database.' + 'required' => true, + 'label' => t('Host'), + 'helptext' => t('The hostname of the database.'), + 'value' => $this->getResource()->get('host', 'localhost') ) ); $this->addElement( - 'text', - 'resource_db_port', - array( - 'label' => 'Port', - 'value' => $this->getResource()->get('port', 3306), - 'required' => true, - 'validators' => array( - array('regex', false, '/^[0-9]+$/') - ), - 'helptext' => 'The port number to use.' + new Number( + array( + 'name' => 'resource_db_port', + 'required' => true, + 'label' => t('Port'), + 'helptext' => t('The port to use.'), + 'value' => $this->getResource()->get('port', 3306) + ) ) ); @@ -143,10 +179,10 @@ class ResourceForm extends Form 'text', 'resource_db_dbname', array( - 'label' => 'Database Name', - 'value' => $this->getResource()->get('dbname', ''), 'required' => true, - 'helptext' => 'The name of the database to use' + 'label' => t('Database Name'), + 'helptext' => t('The name of the database to use'), + 'value' => $this->getResource()->get('dbname', '') ) ); @@ -154,10 +190,10 @@ class ResourceForm extends Form 'text', 'resource_db_username', array ( - 'label' => 'Username', - 'value' => $this->getResource()->get('username', ''), - 'required' => true, - 'helptext' => 'The user name to use for authentication.' + 'required' => true, + 'label' => t('Username'), + 'helptext' => t('The user name to use for authentication.'), + 'value' => $this->getResource()->get('username', '') ) ); @@ -165,64 +201,65 @@ class ResourceForm extends Form 'password', 'resource_db_password', array( - 'label' => 'Password', + 'required' => true, 'renderPassword' => true, - 'value' => $this->getResource()->get('password', ''), - 'helptext' => 'The password to use for authentication', - 'required' => true + 'label' => t('Password'), + 'helptext' => t('The password to use for authentication'), + 'value' => $this->getResource()->get('password', '') ) ); } - private function addStatusdatForm() + protected function addStatusdatForm() { $this->addElement( 'text', 'resource_statusdat_status_file', array( - 'label' => 'Status.dat File', - 'value' => $this->getResource()->get('status_file', '/usr/local/icinga/var/status.dat'), - 'required' => true, - 'helptext' => 'Location of your icinga status.dat file' + 'required' => true, + 'label' => t('Filepath'), + 'helptext' => t('Location of your icinga status.dat file'), + 'value' => $this->getResource()->get('status_file', '/usr/local/icinga/var/status.dat') ) ); + $this->addElement( 'text', 'resource_statusdat_object_file', array( - 'label' => 'Objects.cache File', - 'value' => $this->getResource()->get('status_file', '/usr/local/icinga/var/objects.cache'), - 'required' => true, - 'helptext' => 'Location of your icinga objects.cache file' + 'required' => true, + 'label' => t('Filepath'), + 'helptext' => t('Location of your icinga objects.cache file'), + 'value' => $this->getResource()->get('status_file', '/usr/local/icinga/var/objects.cache') ) ); } - private function addLivestatusForm() + protected function addLivestatusForm() { $this->addElement( 'text', 'resource_livestatus_socket', array( - 'label' => 'Livestatus Socket Location', - 'required' => true, - 'helptext' => 'The path to your livestatus socket used for querying monitoring data', - 'value' => $this->getResource()->socket, + 'required' => true, + 'label' => t('Socket'), + 'helptext' => t('The path to your livestatus socket used for querying monitoring data'), + 'value' => $this->getResource()->get('socket', '/usr/local/icinga/var/rw/livestatus') ) ); } - private function addLdapForm() + protected function addLdapForm() { $this->addElement( 'text', 'resource_ldap_hostname', array( - 'label' => 'LDAP Server Host', - 'allowEmpty' => false, - 'value' => $this->getResource()->get('hostname', 'localhost'), - 'helptext' => 'The hostname or address of the LDAP server to use for authentication', - 'required' => true + 'required' => true, + 'allowEmpty' => false, + 'label' => t('Host'), + 'helptext' => t('The hostname or address of the LDAP server to use for authentication'), + 'value' => $this->getResource()->get('hostname', 'localhost') ) ); @@ -230,10 +267,10 @@ class ResourceForm extends Form 'text', 'resource_ldap_root_dn', array( - 'label' => 'LDAP Root DN', - 'value' => $this->getResource()->get('root_dn', 'ou=people,dc=icinga,dc=org'), - 'helptext' => 'The path where users can be found on the ldap server', - 'required' => true + 'required' => true, + 'label' => t('Root DN'), + 'helptext' => t('The path where users can be found on the ldap server'), + 'value' => $this->getResource()->get('root_dn', 'ou=people,dc=icinga,dc=org') ) ); @@ -241,10 +278,10 @@ class ResourceForm extends Form 'text', 'resource_ldap_bind_dn', array( - 'label' => 'LDAP Bind DN', - 'value' => $this->getResource()->get('bind_dn', 'cn=admin,cn=config'), - 'helptext' => 'The user dn to use for querying the ldap server', - 'required' => true + 'required' => true, + 'label' => t('Bind DN'), + 'helptext' => t('The user dn to use for querying the ldap server'), + 'value' => $this->getResource()->get('bind_dn', 'cn=admin,cn=config') ) ); @@ -252,77 +289,73 @@ class ResourceForm extends Form 'password', 'resource_ldap_bind_pw', array( - 'label' => 'LDAP Bind Password', + 'required' => true, 'renderPassword' => true, - 'value' => $this->getResource()->get('bind_pw', ''), - 'helptext' => 'The password to use for querying the ldap server', - 'required' => true + 'label' => t('Bind Password'), + 'helptext' => t('The password to use for querying the ldap server'), + 'value' => $this->getResource()->get('bind_pw', '') ) ); } - /** - * Set the resource configuration to edit. - * - * @param Zend_Config $resource - */ - public function setResource(Zend_Config $resource) + protected function addFileForm() { - $this->resource = $resource; + $this->addElement( + 'text', + 'resource_file_filename', + array( + 'required' => true, + 'label' => t('Filepath'), + 'helptext' => t('The filename to fetch information from'), + 'value' => $this->getResource()->get('filename', '') + ) + ); + + $this->addElement( + 'text', + 'resource_file_fields', + array( + 'required' => true, + 'label' => t('Pattern'), + 'helptext' => t('The regular expression by which to identify columns'), + 'value' => $this->getResource()->get('fields', '') + ) + ); } - /** - * Get the current resource configuration. - * - * @return Zend_Config - */ - public function getResource() - { - if (!isset($this->resource)) { - // Init empty resource - $this->resource = new Zend_Config( - array('type' => 'db') - ); - } - return $this->resource; - } - - /** - * Add a field to change the resource name and one hidden field - * to save the previous resource name. - */ - private function addNameFields() + protected function addNameFields() { $this->addElement( 'text', 'resource_all_name', array( - 'label' => 'Resource Name', - 'value' => $this->name, - 'helptext' => 'The unique name of this resource', - 'required' => true + 'required' => true, + 'label' => t('Resource Name'), + 'helptext' => t('The unique name of this resource'), + 'value' => $this->getName() ) ); + $this->addElement( 'hidden', 'resource_all_name_old', array( - 'value' => $this->oldName + 'value' => $this->getOldName() ) ); } /** - * Add checkbox at the beginning of the form which allows to skip logic connection validation + * Add checkbox at the beginning of the form which allows to skip connection validation */ - private function addForceCreationCheckbox() + protected function addForceCreationCheckbox() { - $checkbox = new \Zend_Form_Element_Checkbox( + $checkbox = new Zend_Form_Element_Checkbox( array( - 'name' => 'backend_force_creation', - 'label' => 'Force Changes', - 'helptext' => 'Check this box to enforce changes without connectivity validation', - 'order' => 0 + 'order' => 0, + 'name' => 'resource_force_creation', + 'label' => t('Force Changes'), + 'helptext' => t('Check this box to enforce changes without connectivity validation') ) ); $checkbox->addDecorator(new HelpText()); @@ -332,21 +365,22 @@ class ResourceForm extends Form /** * Add a select box for choosing the type to use for this backend */ - private function addTypeSelectionBox() + protected function addTypeSelectionBox() { $this->addElement( 'select', 'resource_type', array( - 'label' => 'Resource Type', - 'value' => $this->getResource()->type, - 'required' => true, - 'helptext' => 'The type of resource.', - 'multiOptions' => array( - 'db' => 'SQL Database', - 'ldap' => 'Ldap', + 'required' => true, + 'label' => t('Resource Type'), + 'helptext' => t('The type of resource'), + 'value' => $this->getResource()->type, + 'multiOptions' => array( + 'db' => t('SQL Database'), + 'ldap' => 'LDAP', 'statusdat' => 'Status.dat', - 'livestatus' => 'Livestatus' + 'livestatus' => 'Livestatus', + 'file' => t('File') ) ) ); @@ -354,21 +388,21 @@ class ResourceForm extends Form } /** - * Validate this form with the Zend validation mechanism and perform a validation of the connection. + * Validate this form with the Zend validation mechanism and perform a validation of the connection * - * If validation fails, the 'backend_force_creation' checkbox is prepended to the form to allow users to - * skip the logic connection validation. + * If validation fails, the 'resource_force_creation' checkbox is prepended to the form to allow users to + * skip the connection validation * - * @param array $data The form input to validate + * @param array $data The form input to validate * - * @return bool True when validation succeeded, false if not + * @return bool True when validation succeeded, false if not */ public function isValid($data) { if (!parent::isValid($data)) { return false; } - if ($this->getRequest()->getPost('backend_force_creation')) { + if (isset($data['resource_force_creation']) && $data['resource_force_creation']) { return true; } if (!$this->isValidResource()) { @@ -380,14 +414,15 @@ class ResourceForm extends Form /** * Test if the changed resource is a valid resource, by instantiating it and - * checking if connection is possible. + * checking if a connection is possible * - * @return bool True when connection to the resource is possible. + * @return bool True when a connection to the resource is possible */ - private function isValidResource() + public function isValidResource() { + $config = $this->getConfig(); + try { - $config = $this->getConfig(); switch ($config->type) { case 'db': $resource = ResourceFactory::createResource($config); @@ -396,23 +431,33 @@ class ResourceForm extends Form case 'statusdat': if (!file_exists($config->object_file) || !file_exists($config->status_file)) { $this->addErrorMessage( - 'Connectivity validation failed, the provided file or socket does not exist.' + t('Connectivity validation failed, the provided file does not exist.') ); return false; } break; case 'livestatus': - // TODO: Implement check + $resource = ResourceFactory::createResource($config); + $resource->connect()->disconnect(); break; case 'ldap': $resource = ResourceFactory::createResource($config); $resource->connect(); break; + case 'file': + if (!file_exists($config->filename)) { + $this->addErrorMessage( + t('Connectivity validation failed, the provided file does not exist.') + ); + return false; + } + break; } - } catch (\Exception $exc) { - $this->addErrorMessage('Connectivity validation failed, connection to the given resource not possible.'); + } catch (Exception $e) { + $this->addErrorMessage(t('Connectivity validation failed, connection to the given resource not possible.')); return false; } + return true; } @@ -420,6 +465,7 @@ class ResourceForm extends Form { $this->addNameFields(); $this->addTypeSelectionBox(); + switch ($this->getRequest()->getParam('resource_type', $this->getResource()->type)) { case 'db': $this->addDbForm(); @@ -433,30 +479,33 @@ class ResourceForm extends Form case 'ldap': $this->addLdapForm(); break; + case 'file': + $this->addFileForm(); + break; } + $this->setSubmitLabel('{{SAVE_ICON}} Save Changes'); } /** * Return a configuration containing the backend settings entered in this form * - * @return Zend_Config The updated configuration for this backend + * @return Zend_Config The updated configuration for this backend */ public function getConfig() { $values = $this->getValues(); - $type = $values['resource_type']; - $result = array('type' => $type); + + $result = array('type' => $values['resource_type']); foreach ($values as $key => $value) { if ($key !== 'resource_type' && $key !== 'resource_all_name' && $key !== 'resource_all_name_old') { $configKey = explode('_', $key, 3); - if (sizeof($configKey) < 3) { - Logger::warning('EditResourceForm: invalid form key "' . $key . '" was ignored.'); - continue; + if (count($configKey) === 3) { + $result[$configKey[2]] = $value; } - $result[$configKey[2]] = $value; } } + return new Zend_Config($result); } } diff --git a/library/Icinga/Protocol/Livestatus/Connection.php b/library/Icinga/Protocol/Livestatus/Connection.php index 395d21442..fe440c788 100644 --- a/library/Icinga/Protocol/Livestatus/Connection.php +++ b/library/Icinga/Protocol/Livestatus/Connection.php @@ -273,6 +273,15 @@ class Connection } } + public function connect() + { + if (!$this->connection) { + $this->getConnection(); + } + + return $this; + } + public function disconnect() { if ($this->connection) { diff --git a/library/Icinga/Web/Form/Element/Number.php b/library/Icinga/Web/Form/Element/Number.php index 9fa0fe76f..8749c07ea 100644 --- a/library/Icinga/Web/Form/Element/Number.php +++ b/library/Icinga/Web/Form/Element/Number.php @@ -29,16 +29,38 @@ namespace Icinga\Web\Form\Element; +use Zend_Form_Element_Xhtml; + /** * Number form element - * - * @TODO: The given label for this element is not displayed. (Reason unknown) */ -class Number extends \Zend_Form_Element_Xhtml +class Number extends Zend_Form_Element_Xhtml { /** * Default form view helper to use for rendering + * * @var string */ public $helper = "formNumber"; + + /** + * Check whether $value is of type integer + * + * @param string $value The value to check + * @param mixed $context Context to use + * + * @return bool + */ + public function isValid($value, $context = null) + { + if (parent::isValid($value, $context)) { + if (is_numeric($value)) { + return true; + } + + $this->addError(t('Please enter a number.')); + } + + return false; + } } diff --git a/test/php/application/forms/Config/Authentication/DbBackendFormTest.php b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php index 8e9488450..3e515c97a 100644 --- a/test/php/application/forms/Config/Authentication/DbBackendFormTest.php +++ b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php @@ -4,12 +4,18 @@ namespace Tests\Icinga\Form\Config\Authentication; -use \Mockery; +use Mockery; use Icinga\Test\BaseTestCase; use Icinga\Form\Config\Authentication\DbBackendForm; class DbBackendFormTest extends BaseTestCase { + public function tearDown() + { + parent::tearDown(); + Mockery::close(); // Necessary because some tests run in a separate process + } + /** * @runInSeparateProcess */ diff --git a/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php index 9493eda47..1da715f1a 100644 --- a/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php +++ b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php @@ -4,12 +4,18 @@ namespace Tests\Icinga\Form\Config\Authentication; -use \Mockery; +use Mockery; use Icinga\Test\BaseTestCase; use Icinga\Form\Config\Authentication\LdapBackendForm; class LdapBackendFormTest extends BaseTestCase { + public function tearDown() + { + parent::tearDown(); + Mockery::close(); // Necessary because some tests run in a separate process + } + /** * @runInSeparateProcess */ diff --git a/test/php/application/forms/Config/Resource/ResourceFormTest.php b/test/php/application/forms/Config/Resource/ResourceFormTest.php new file mode 100644 index 000000000..cbaa55502 --- /dev/null +++ b/test/php/application/forms/Config/Resource/ResourceFormTest.php @@ -0,0 +1,258 @@ +is_valid; + } +} + +class ResourceFormTest extends BaseTestCase +{ + public function tearDown() + { + parent::tearDown(); + Mockery::close(); // Necessary because some tests run in a separate process + } + + public function testIsForceCreationCheckboxBeingAdded() + { + $form = new TestResourceForm(); + $form->is_valid = false; + + $this->assertFalse($form->isValid(array())); + $this->assertNotNull( + $form->getElement('resource_force_creation'), + 'Checkbox to force the creation of a resource is not being added though the resource is invalid' + ); + } + + public function testIsForceCreationCheckboxNotBeingAdded() + { + $form = new TestResourceForm(); + $form->is_valid = true; + + $this->assertTrue($form->isValid(array())); + $this->assertNull( + $form->getElement('resource_force_creation'), + 'Checkbox to force the creation of a resource is being added though the resource is valid' + ); + } + + public function testIsTheFormValidIfForceCreationTrue() + { + $form = new TestResourceForm(); + $form->is_valid = false; + + $this->assertTrue( + $form->isValid(array('resource_force_creation' => 1)), + 'ResourceForm with invalid resource is not valid though force creation is set' + ); + } + + /** + * @runInSeparateProcess + */ + public function testValidDbResourceIsValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('getConnection')->atMost()->twice()->andReturn(Mockery::self())->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'db'))); + + $this->assertTrue( + $form->isValidResource(), + 'ResourceForm claims that a valid db resource is not valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testInvalidDbResourceIsNotValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('getConnection')->once()->andThrow('\Exception')->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'db'))); + + $this->assertFalse( + $form->isValidResource(), + 'ResourceForm claims that an invalid db resource is valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testValidLdapResourceIsValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('connect')->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'ldap'))); + + $this->assertTrue( + $form->isValidResource(), + 'ResourceForm claims that a valid ldap resource is not valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testInvalidLdapResourceIsNotValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('connect')->once()->andThrow('\Exception')->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'ldap'))); + + $this->assertFalse( + $form->isValidResource(), + 'ResourceForm claims that an invalid ldap resource is valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testValidLivestatusResourceIsValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('connect')->andReturn(Mockery::self()) + ->shouldReceive('disconnect')->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'livestatus'))); + + $this->assertTrue( + $form->isValidResource(), + 'ResourceForm claims that a valid livestatus resource is not valid' + ); + } + + /** + * @runInSeparateProcess + */ + public function testInvalidLivestatusResourceIsNotValid() + { + $this->setUpResourceFactoryMock( + Mockery::mock()->shouldReceive('connect')->once()->andThrow('\Exception')->getMock() + ); + $form = $this->buildResourceForm(new Zend_Config(array('type' => 'livestatus'))); + + $this->assertFalse( + $form->isValidResource(), + 'ResourceForm claims that an invalid livestatus resource is valid' + ); + } + + public function testValidFileResourceIsValid() + { + $form = $this->buildResourceForm( + new Zend_Config( + array( + 'type' => 'file', + 'filename' => BaseTestCase::$testDir . '/res/status/icinga.status.dat' + ) + ) + ); + + $this->assertTrue( + $form->isValidResource(), + 'ResourceForm claims that a valid file resource is not valid' + ); + } + + public function testInvalidFileResourceIsNotValid() + { + $form = $this->buildResourceForm( + new Zend_Config( + array( + 'type' => 'file', + 'filename' => 'not_existing' + ) + ) + ); + + $this->assertFalse( + $form->isValidResource(), + 'ResourceForm claims that an invalid file resource is valid' + ); + } + + public function testValidStatusdatResourceIsValid() + { + $form = $this->buildResourceForm( + new Zend_Config( + array( + 'type' => 'statusdat', + 'status_file' => BaseTestCase::$testDir . '/res/status/icinga.status.dat', + 'object_file' => BaseTestCase::$testDir . '/res/status/icinga.objects.cache', + ) + ) + ); + + $this->assertTrue( + $form->isValidResource(), + 'ResourceForm claims that a valid statusdat resource is not valid' + ); + } + + public function testInvalidStatusdatResourceIsNotValid() + { + $form = $this->buildResourceForm( + new Zend_Config( + array( + 'type' => 'statusdat', + 'status_file' => 'not_existing', + 'object_file' => 'not_existing' + ) + ) + ); + + $this->assertFalse( + $form->isValidResource(), + 'ResourceForm claims that an invalid statusdat resource is valid' + ); + } + + protected function buildResourceForm($resourceConfig) + { + $form = new ResourceForm(); + $form->setRequest($this->getRequestMock()); + $form->setResource($resourceConfig); + $form->create(); + + return $form; + } + + protected function getRequestMock() + { + return Mockery::mock('\Zend_Controller_Request_Abstract') + ->shouldReceive('getParam') + ->with(Mockery::type('string'), Mockery::type('string')) + ->andReturnUsing(function ($name, $default) { return $default; }) + ->getMock(); + } + + protected function setUpResourceFactoryMock($resourceMock) + { + Mockery::mock('alias:Icinga\Data\ResourceFactory') + ->shouldReceive('createResource') + ->with(Mockery::type('\Zend_Config')) + ->andReturn($resourceMock); + } +} From f7051ca992f30981ccb479fdc15e280062203dc4 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 16 Apr 2014 17:04:21 +0200 Subject: [PATCH 05/29] Remove form tests that cover only graphical tests refs #6011 --- application/forms/TestForm.php | 34 --- .../forms/Config/GeneralFormTest.php | 197 ------------------ .../forms/Config/LoggingFormTest.php | 136 ------------ .../forms/Preference/GeneralFormTest.php | 58 ------ 4 files changed, 425 deletions(-) delete mode 100644 application/forms/TestForm.php delete mode 100644 test/php/application/forms/Config/GeneralFormTest.php delete mode 100644 test/php/application/forms/Config/LoggingFormTest.php delete mode 100644 test/php/application/forms/Preference/GeneralFormTest.php diff --git a/application/forms/TestForm.php b/application/forms/TestForm.php deleted file mode 100644 index ff6008c22..000000000 --- a/application/forms/TestForm.php +++ /dev/null @@ -1,34 +0,0 @@ - - * @license http://www.gnu.org/licenses/gpl-2.0.txt GPL, version 2 - * @author Icinga Development Team - * - */ -// {{{ICINGA_LICENSE_HEADER}}} - -namespace Icinga\Form; - -class TestForm -{ -} diff --git a/test/php/application/forms/Config/GeneralFormTest.php b/test/php/application/forms/Config/GeneralFormTest.php deleted file mode 100644 index 82d173080..000000000 --- a/test/php/application/forms/Config/GeneralFormTest.php +++ /dev/null @@ -1,197 +0,0 @@ -viewMock = Mockery::mock('\Zend_View'); - $this->viewMock->shouldReceive('icon')->andReturn(''); - } - - private function isHiddenElement($value, $htmlString) - { - $html = new DOMDocument(); - $html->loadHTML($htmlString); - $hidden = $html->getElementsByTagName('noscript'); - - foreach ($hidden as $node) { - foreach ($node->childNodes as $child) { - if ($child->hasAttributes() === false) { - continue; - } - if (strpos($child->attributes->getNamedItem('id')->value, $value . '-element') !== false) { - return true; - } - } - } - return false; - } - - public function testCorrectFieldPopulation() - { - $form = $this->createForm('Icinga\Form\Config\GeneralForm'); - $form->setDateFormatter(new Zend_View_Helper_DateFormat($this->getRequest())); - $form->setConfiguration( - new Zend_Config( - array( - 'global' => array( - 'environment' => 'development', - 'timezone' => 'Europe/Berlin', - 'indexModule' => 'monitoring', - 'indexController' => 'dashboard', - 'moduleFolder' => '/my/module/path', - 'dateFormat' => 'd-m/Y', - 'timeFormat' => 'A:i' - ), - 'preferences' => array( - 'type' => 'ini', - 'configPath' => './my/path' - ) - ) - ) - ); - $form->setResources( - array( - 'db' => array( - 'type' => 'db' - ) - ) - ); - $form->setConfigDir('/tmp'); - $form->setView($this->viewMock); - - $form->create(); - - $this->assertEquals( - 1, - $form->getValue('environment'), - 'Asserting the checkbox for devlopment being set to true' - ); - $this->assertEquals( - 'Europe/Berlin', - $form->getValue('timezone'), - 'Asserting the correct timezone to be displayed' - ); - $this->assertEquals( - '/my/module/path', - $form->getValue('module_folder'), - 'Asserting the correct module folder to be set' - ); - $this->assertEquals( - 'd-m/Y', - $form->getValue('date_format'), - 'Asserting the correct data format to be set' - ); - $this->assertEquals( - 'A:i', - $form->getValue('time_format'), - 'Asserting the correct time to be set' - ); - $this->assertEquals( - 'ini', - $form->getValue('preferences_type'), - 'Asserting the correct preference type to be set' - ); - $this->assertEquals( - './my/path', - $form->getValue('preferences_ini_path'), - 'Asserting the correct ini path to be set' - ); - $this->assertEquals( - '', - $form->getValue('preferences_db_resource'), - 'Asserting the database resource not to be set' - ); - } - - public function testCorrectConditionalIniFieldRendering() - { - $form = $this->createForm('Icinga\Form\Config\GeneralForm'); - $form->setDateFormatter(new Zend_View_Helper_DateFormat($this->getRequest())); - $form->setConfiguration( - new Zend_Config( - array( - 'preferences' => array( - 'type' => 'ini', - 'configPath' => './my/path' - ) - ) - ) - ); - $form->setConfigDir('/tmp'); - $form->setResources( - array( - 'db' => array( - 'type' => 'db' - ) - ) - ); - $form->setView($this->viewMock); - - $form->create(); - $view = new Zend_View(); - - $this->assertFalse( - $this->isHiddenElement('preferences_ini_path', $form->render($view)), - "Asserting the ini path field to be displayed when an ini preference is set" - ); - $this->assertTrue( - $this->isHiddenElement('preferences_db_resource', $form->render($view)), - "Asserting the db resource to be hidden when an ini preference is set" - ); - } - - public function testCorrectConditionalDbFieldRendering() - { - $form = $this->createForm('Icinga\Form\Config\GeneralForm'); - $form->setDateFormatter(new Zend_View_Helper_DateFormat($this->getRequest())); - $form->setConfiguration( - new Zend_Config( - array( - 'preferences' => array( - 'type' => 'db', - 'configPath' => './my/path', - 'resource' => 'my_resource' - ) - ) - ) - ); - $form->setConfigDir('/tmp'); - $form->setResources( - array( - 'db' => array( - 'type' => 'db' - ) - ) - ); - $form->setView($this->viewMock); - - $form->create(); - $view = new Zend_View(); - - $this->assertTrue( - $this->isHiddenElement('preferences_ini_path', $form->render($view)), - "Asserting the ini path field to be hidden when db preference is set" - ); - $this->assertFalse( - $this->isHiddenElement('preferences_ini_resource', $form->render($view)), - "Asserting the db resource to be displayed when db preference is set" - ); - } -} diff --git a/test/php/application/forms/Config/LoggingFormTest.php b/test/php/application/forms/Config/LoggingFormTest.php deleted file mode 100644 index 6bc22f824..000000000 --- a/test/php/application/forms/Config/LoggingFormTest.php +++ /dev/null @@ -1,136 +0,0 @@ -createForm('Icinga\Form\Config\LoggingForm'); - $config = new Zend_Config( - array( - 'logging' => array( - 'enable' => 1, - 'target' => '/some/path', - 'verbose' => 0, - 'type' => 'stream', - 'debug' => array( - 'enable' => 1, - 'target' => '/some/debug/path', - 'type' => 'stream' - ) - ) - ) - ); - $form->setConfiguration($config); - $form->setBaseDir('basedir'); - $form->create(); - - $this->assertEquals( - '0', - $form->getValue('logging_app_verbose'), - 'Asserting the logging verbose tick not to be set' - ); - $this->assertEquals( - '/some/path', - $form->getValue('logging_app_target'), - 'Asserting the logging path to be set' - ); - $this->assertEquals( - 1, - $form->getValue('logging_debug_enable'), - 'Asserting the debug log enable tick to be set' - ); - $this->assertEquals( - '/some/debug/path', - $form->getValue('logging_debug_target'), - 'Asserting the debug log path to be set' - ); - } - - /** - * Test the logging form to create correct modified configurations when submit - */ - public function testCorrectConfigCreation() - { - $form = $this->createForm( - 'Icinga\Form\Config\LoggingForm', - array( - 'logging_enable' => 1, - 'logging_app_target' => 'some/new/target', - 'logging_app_verbose' => 1, - 'logging_debug_enable' => 0, - 'logging_debug_target' => 'a/new/target' - ) - ); - $baseConfig = new Zend_Config( - array( - 'global' => array( - 'option' => 'value' - ), - 'logging' => array( - 'enable' => 1, - 'target' => '/some/path', - 'verbose' => 0, - 'type' => 'stream', - 'debug' => array( - 'enable' => 1, - 'target' => '/some/debug/path', - 'type' => 'stream' - ) - ) - ) - ); - $form->setConfiguration($baseConfig); - $form->setBaseDir('basedir'); - $form->create(); - $form->populate($this->getRequest()->getParams()); - $config = $form->getConfig(); - $this->assertEquals( - 'value', - $config->global->option, - 'Asserting global options not to be altered when changing log' - ); - $this->assertEquals( - 1, - $config->logging->enable, - 'Asserting logging to stay enabled when enable is ticked' - ); - $this->assertEquals( - 'some/new/target', - $config->logging->target, - 'Asserting target modifications to be applied' - ); - $this->assertEquals( - 1, - $config->logging->verbose, - 'Asserting ticking the verbose checkbox to be applied' - ); - $this->assertEquals( - 'stream', - $config->logging->type, - 'Asserting the type to stay "stream"' - ); - $this->assertEquals( - 0, - $config->logging->debug->enable, - 'Asserting debug log to be disabled' - ); - $this->assertEquals( - 'a/new/target', - $config->logging->debug->target, - 'Asserting the debug log target modifications to be applied' - ); - } -} diff --git a/test/php/application/forms/Preference/GeneralFormTest.php b/test/php/application/forms/Preference/GeneralFormTest.php deleted file mode 100644 index a397cc815..000000000 --- a/test/php/application/forms/Preference/GeneralFormTest.php +++ /dev/null @@ -1,58 +0,0 @@ -createForm('Icinga\Form\Preference\GeneralForm'); - $form->setDateFormatter(new Zend_View_Helper_DateFormat($this->getRequest())); - $form->setRequest($this->getRequest()); - $form->create(); - $this->assertSame( - 1, - $form->getElement('timezone')->getAttrib('disabled'), - 'Asserting form elements to be disabled when not set in a preference' - ); - } - - /** - * Test whether fields with preferences are enabled - */ - public function testEnableFormIfUsingPreference() - { - $form = $this->createForm('Icinga\Form\Preference\GeneralForm'); - $form->setDateFormatter(new Zend_View_Helper_DateFormat($this->getRequest())); - $form->setRequest($this->getRequest()); - $form->setUserPreferences( - new Preferences( - array( - 'app.timezone' => 'Europe/Berlin' - ) - ) - ); - $form->create(); - $this->assertSame( - null, - $form->getElement('timezone')->getAttrib('disabled'), - 'Asserting form elements to be disabled when not set in a preference' - ); - } -} From 24156040356cdf6462d4393348e63a9d80a74e14 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 13:03:30 +0200 Subject: [PATCH 06/29] Add test for Zend_View_Helper_DateFormat refs #6011 --- application/views/helpers/DateFormat.php | 24 +-- .../views/helpers/DateFormatTest.php | 166 ++++++++++++++++++ .../views/helpers/DateFormatTest/config.ini | 3 + 3 files changed, 182 insertions(+), 11 deletions(-) create mode 100644 test/php/application/views/helpers/DateFormatTest.php create mode 100644 test/php/application/views/helpers/DateFormatTest/config.ini diff --git a/application/views/helpers/DateFormat.php b/application/views/helpers/DateFormat.php index 76408de6b..80c6bc50a 100644 --- a/application/views/helpers/DateFormat.php +++ b/application/views/helpers/DateFormat.php @@ -1,6 +1,4 @@ format($format); } @@ -96,6 +96,7 @@ class Zend_View_Helper_DateFormat extends Zend_View_Helper_Abstract * Format date according to user's format * * @param int $timestamp A unix timestamp + * * @return string The formatted date string */ public function formatDate($timestamp) @@ -107,6 +108,7 @@ class Zend_View_Helper_DateFormat extends Zend_View_Helper_Abstract * Format time according to user's format * * @param int $timestamp A unix timestamp + * * @return string The formatted time string */ public function formatTime($timestamp) @@ -133,7 +135,8 @@ class Zend_View_Helper_DateFormat extends Zend_View_Helper_Abstract public function getDateFormat() { return $this->request->getUser()->getPreferences()->get( - 'app.dateFormat', Config::app()->global !== null ? Config::app()->global->get('dateFormat', 'd/m/Y') : 'd/m/Y' + 'app.dateFormat', + Config::app()->global !== null ? Config::app()->global->get('dateFormat', 'd/m/Y') : 'd/m/Y' ); } @@ -145,7 +148,8 @@ class Zend_View_Helper_DateFormat extends Zend_View_Helper_Abstract public function getTimeFormat() { return $this->request->getUser()->getPreferences()->get( - 'app.timeFormat', Config::app()->global !== null ? Config::app()->global->get('timeFormat', 'g:i A') : 'g:i A' + 'app.timeFormat', + Config::app()->global !== null ? Config::app()->global->get('timeFormat', 'g:i A') : 'g:i A' ); } @@ -159,5 +163,3 @@ class Zend_View_Helper_DateFormat extends Zend_View_Helper_Abstract return $this->getDateFormat() . ' ' . $this->getTimeFormat(); } } - -// @codingStandardsIgnoreStop diff --git a/test/php/application/views/helpers/DateFormatTest.php b/test/php/application/views/helpers/DateFormatTest.php new file mode 100644 index 000000000..04e5e6a90 --- /dev/null +++ b/test/php/application/views/helpers/DateFormatTest.php @@ -0,0 +1,166 @@ +oldConfigDir = Config::$configDir; + Config::$configDir = dirname(__FILE__) . '/DateFormatTest'; + } + + public function tearDown() + { + parent::tearDown(); + Config::$configDir = $this->oldConfigDir; + DateTimeFactory::setConfig(array('timezone' => new DateTimeZone(date_default_timezone_get()))); + } + + public function testFormatReturnsCorrectDateWithTimezoneApplied() + { + DateTimeFactory::setConfig(array('timezone' => new DateTimeZone('Europe/Berlin'))); + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock()); + + $this->assertEquals( + '12:05', + $helper->format(1397729100, 'H:i'), + 'Zend_View_Helper_DateFormat::format does not return a valid' . + ' formatted time or does not apply the user\'s timezone' + ); + } + + public function testFormatDateReturnsCorrectDate() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock('d_m_y')); + + $this->assertEquals( + '17_04_14', + $helper->formatDate(1397729100), + 'Zend_View_Helper_DateFormat::formatDate does not return a valid formatted date' + ); + } + + public function testFormatTimeReturnsCorrectTime() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock(null, 'H:i')); + + $this->assertEquals( + '10:05', + $helper->formatTime(1397729100), + 'Zend_View_Helper_DateFormat::formatTime does not return a valid formatted time' + ); + } + + public function testFormatDatetimeReturnsCorrectDatetime() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock('d m Y', 'H:i a')); + + $this->assertEquals( + '17 04 2014 10:05 am', + $helper->formatDateTime(1397729100), + 'Zend_View_Helper_DateFormat::formatDateTime does not return a valid formatted date and time' + ); + } + + public function testGetDateFormatReturnsCorrectFormat() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock('d/m-y')); + + $this->assertEquals( + 'd/m-y', + $helper->getDateFormat(), + 'Zend_View_Helper_DateFormat::getDateFormat does not return the user\'s date format' + ); + } + + public function testGetTimeFormatReturnsCorrectFormat() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock(null, 'H.i A')); + + $this->assertEquals( + 'H.i A', + $helper->getTimeFormat(), + 'Zend_View_Helper_DateFormat::getTimeFormat does not return the user\'s time format' + ); + } + + public function testGetDatetimeFormatReturnsCorrectFormat() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock('d/m-y', 'H.i A')); + + $this->assertEquals( + 'd/m-y H.i A', + $helper->getDateTimeFormat(), + 'Zend_View_Helper_DateFormat::getDateTimeFormat does not return the user\'s date and time format' + ); + } + + public function testGetDateFormatReturnsFormatFromConfig() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock()); + + $this->assertEquals( + 'd-m-y', + $helper->getDateFormat(), + 'Zend_View_Helper_DateFormat::getDateFormat does not return the format set' . + ' in the global configuration if the user\'s preferences do not provide one' + ); + } + + public function testGetTimeFormatReturnsFormatFromConfig() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock()); + + $this->assertEquals( + 'G:i a', + $helper->getTimeFormat(), + 'Zend_View_Helper_DateFormat::getTimeFormat does not return the format set' . + ' in the global configuration if the user\'s preferences do not provide one' + ); + } + + public function testGetDatetimeFormatReturnsFormatFromConfig() + { + $helper = new Zend_View_Helper_DateFormat($this->getRequestMock()); + + $this->assertEquals( + 'd-m-y G:i a', + $helper->getDateTimeFormat(), + 'Zend_View_Helper_DateFormat::getDateTimeFormat does not return the format set' . + ' in the global configuration if the user\'s preferences do not provide one' + ); + } + + protected function getRequestMock($dateFormat = null, $timeFormat = null) + { + $mock = Mockery::mock('\Zend_Controller_Request_Abstract'); + $mock->shouldReceive('getUser->getPreferences->get') + ->with(Mockery::type('string'), Mockery::type('string')) + ->andReturnUsing( + function ($ident, $default) use ($dateFormat, $timeFormat) { + if ($dateFormat !== null && $ident === 'app.dateFormat') { + return $dateFormat; + } elseif ($timeFormat !== null && $ident === 'app.timeFormat') { + return $timeFormat; + } else { + return $default; + } + } + ); + + return $mock; + } +} diff --git a/test/php/application/views/helpers/DateFormatTest/config.ini b/test/php/application/views/helpers/DateFormatTest/config.ini new file mode 100644 index 000000000..f095d27ca --- /dev/null +++ b/test/php/application/views/helpers/DateFormatTest/config.ini @@ -0,0 +1,3 @@ +[global] +dateFormat = "d-m-y" +timeFormat = "G:i a" \ No newline at end of file From 1145fc118e1c1dbcc63ba716a445b51d1a609a12 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 13:08:46 +0200 Subject: [PATCH 07/29] Move session tests to correct sub-folder refs #6011 --- .../php/library/Icinga/{ => Web}/Session/PhpSessionTest.php | 2 +- .../Icinga/{ => Web}/Session/SessionNamespaceTest.php | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) rename test/php/library/Icinga/{ => Web}/Session/PhpSessionTest.php (98%) rename test/php/library/Icinga/{ => Web}/Session/SessionNamespaceTest.php (97%) diff --git a/test/php/library/Icinga/Session/PhpSessionTest.php b/test/php/library/Icinga/Web/Session/PhpSessionTest.php similarity index 98% rename from test/php/library/Icinga/Session/PhpSessionTest.php rename to test/php/library/Icinga/Web/Session/PhpSessionTest.php index 3c93a25db..49641c60d 100644 --- a/test/php/library/Icinga/Session/PhpSessionTest.php +++ b/test/php/library/Icinga/Web/Session/PhpSessionTest.php @@ -2,7 +2,7 @@ // {{{ICINGA_LICENSE_HEADER}}} // {{{ICINGA_LICENSE_HEADER}}} -namespace Tests\Icinga\Session; +namespace Tests\Icinga\Web\Session; use Icinga\Test\BaseTestCase; use Icinga\Web\Session\PhpSession; diff --git a/test/php/library/Icinga/Session/SessionNamespaceTest.php b/test/php/library/Icinga/Web/Session/SessionNamespaceTest.php similarity index 97% rename from test/php/library/Icinga/Session/SessionNamespaceTest.php rename to test/php/library/Icinga/Web/Session/SessionNamespaceTest.php index 652459d98..db7be8c64 100644 --- a/test/php/library/Icinga/Session/SessionNamespaceTest.php +++ b/test/php/library/Icinga/Web/Session/SessionNamespaceTest.php @@ -2,10 +2,10 @@ // {{{ICINGA_LICENSE_HEADER}}} // {{{ICINGA_LICENSE_HEADER}}} -namespace Tests\Icinga\Session; +namespace Tests\Icinga\Web\Session; -use \Exception; -use \Mockery; +use Exception; +use Mockery; use Icinga\Test\BaseTestCase; use Icinga\Web\Session\SessionNamespace; From e27e538bd9b04f8c4a608fee1b25623de6e72b09 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 14:15:29 +0200 Subject: [PATCH 08/29] Add test for Icinga\Util\Translator refs #6011 --- library/Icinga/Util/Translator.php | 2 +- .../library/Icinga/Util/TranslatorTest.php | 78 ++++++++++++++++++ .../locale/de_DE/LC_MESSAGES/icingatest.mo | Bin 0 -> 111 bytes .../locale/de_DE/LC_MESSAGES/icingatest.po | 2 + .../locale/fr_FR/LC_MESSAGES/icingatest.mo | Bin 0 -> 111 bytes .../locale/fr_FR/LC_MESSAGES/icingatest.po | 2 + 6 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 test/php/library/Icinga/Util/TranslatorTest.php create mode 100644 test/php/res/locale/de_DE/LC_MESSAGES/icingatest.mo create mode 100644 test/php/res/locale/de_DE/LC_MESSAGES/icingatest.po create mode 100644 test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.mo create mode 100644 test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.po diff --git a/library/Icinga/Util/Translator.php b/library/Icinga/Util/Translator.php index 61d667463..733806c72 100644 --- a/library/Icinga/Util/Translator.php +++ b/library/Icinga/Util/Translator.php @@ -29,7 +29,7 @@ namespace Icinga\Util; -use \Exception; +use Exception; /** * Helper class to ease internationalization when using gettext diff --git a/test/php/library/Icinga/Util/TranslatorTest.php b/test/php/library/Icinga/Util/TranslatorTest.php new file mode 100644 index 000000000..0610ba87c --- /dev/null +++ b/test/php/library/Icinga/Util/TranslatorTest.php @@ -0,0 +1,78 @@ +assertEquals( + array('de_DE', 'fr_FR'), + Translator::getAvailableLocaleCodes(), + 'Translator::getAvailableLocaleCodes does not return all available locale codes' + ); + } + + public function testWhetherSetupLocaleSetsUpTheGivenLocale() + { + Translator::setupLocale('de_DE'); + $this->assertContains( + setlocale(LC_ALL, 0), + array('de_DE', 'de_DE.UTF-8'), + 'Translator::setupLocale does not properly set up a given locale' + ); + } + + /** + * @expectedException \Exception + */ + public function testWhetherSetupLocaleThrowsAnExceptionWhenGivenAnInvalidLocale() + { + Translator::setupLocale('foobar'); + } + + public function testWhetherSetupLocaleSetsCAsLocaleWhenGivenAnInvalidLocale() + { + try { + Translator::setupLocale('foobar'); + $this->fail('Translator::setupLocale does not throw an exception when given an invalid locale'); + } catch (Exception $e) { + $this->assertEquals( + 'C', + setlocale(LC_ALL, 0), + 'Translator::setupLocale does not set C as locale in case the given one is invalid' + ); + } + } + + public function testWhetherTranslateReturnsTheCorrectMessageForTheCurrentLocale() + { + Translator::setupLocale('de_DE'); + + $this->assertEquals( + 'Lorem ipsum dolor sit amet!', + Translator::translate('Lorem ipsum dolor sit amet', 'icingatest'), + 'Translator::translate does not translate the given message correctly to German' + ); + + Translator::setupLocale('fr_FR'); + + $this->assertEquals( + 'Lorem ipsum dolor sit amet?', + Translator::translate('Lorem ipsum dolor sit amet', 'icingatest'), + 'Translator::translate does not translate the given message correctly to French' + ); + } +} diff --git a/test/php/res/locale/de_DE/LC_MESSAGES/icingatest.mo b/test/php/res/locale/de_DE/LC_MESSAGES/icingatest.mo new file mode 100644 index 0000000000000000000000000000000000000000..487e490237f1702eefb05f227d70d93ed0d484ca GIT binary patch literal 111 zcmca7#4?ou2pEA_28dOFm>Gz5fLIEMEr3`Wh=YL`1VHLQz$d>bHCG|Cptv+wAtgU2 Tzeu4tvqT{=H?;(hlp+HFK^+!k literal 0 HcmV?d00001 diff --git a/test/php/res/locale/de_DE/LC_MESSAGES/icingatest.po b/test/php/res/locale/de_DE/LC_MESSAGES/icingatest.po new file mode 100644 index 000000000..2bf77ccae --- /dev/null +++ b/test/php/res/locale/de_DE/LC_MESSAGES/icingatest.po @@ -0,0 +1,2 @@ +msgid "Lorem ipsum dolor sit amet" +msgstr "Lorem ipsum dolor sit amet!" \ No newline at end of file diff --git a/test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.mo b/test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.mo new file mode 100644 index 0000000000000000000000000000000000000000..0f7d69f2a5b04cb7b2e3e753e1b3724044ce22ec GIT binary patch literal 111 zcmca7#4?ou2pEA_28dOFm>Gz5fLIEMEr3`Wh=YL`1VHLQz$d>bHCG|Cptv+wAtgU2 Tzeu4tvqT{=H?;(hlsy9gL0J}p literal 0 HcmV?d00001 diff --git a/test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.po b/test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.po new file mode 100644 index 000000000..8ae17d6aa --- /dev/null +++ b/test/php/res/locale/fr_FR/LC_MESSAGES/icingatest.po @@ -0,0 +1,2 @@ +msgid "Lorem ipsum dolor sit amet" +msgstr "Lorem ipsum dolor sit amet?" \ No newline at end of file From 3e83854e04f70067f5ddc0feafdf4e55980560e6 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 14:29:21 +0200 Subject: [PATCH 09/29] Add test for Icinga\Util\String refs #6011 --- test/php/library/Icinga/Util/StringTest.php | 29 +++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 test/php/library/Icinga/Util/StringTest.php diff --git a/test/php/library/Icinga/Util/StringTest.php b/test/php/library/Icinga/Util/StringTest.php new file mode 100644 index 000000000..8ecf61edc --- /dev/null +++ b/test/php/library/Icinga/Util/StringTest.php @@ -0,0 +1,29 @@ +assertEquals( + array('one', 'two', 'three'), + String::trimSplit(' one ,two , three'), + 'String::trimSplit does not properly split a string and/or trim its elements' + ); + } + + public function testWhetherTrimSplitSplitsByTheGivenDelimiter() + { + $this->assertEquals( + array('one', 'two', 'three'), + String::trimSplit('one.two.three', '.'), + 'String::trimSplit does not split a string by the given delimiter' + ); + } +} From 1db9b1ede423be0105902a30ebf3adba4c589acb Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 15:33:57 +0200 Subject: [PATCH 10/29] Move non-factory functions from DateTimeFactory to Icinga\Util\Format --- library/Icinga/Util/DateTimeFactory.php | 35 -------------- library/Icinga/Util/Format.php | 47 +++++++++++++++++++ .../controllers/TimelineController.php | 5 +- .../library/Monitoring/Timeline/TimeRange.php | 18 +++---- 4 files changed, 59 insertions(+), 46 deletions(-) diff --git a/library/Icinga/Util/DateTimeFactory.php b/library/Icinga/Util/DateTimeFactory.php index 17aa72948..9dd854aa7 100644 --- a/library/Icinga/Util/DateTimeFactory.php +++ b/library/Icinga/Util/DateTimeFactory.php @@ -91,39 +91,4 @@ class DateTimeFactory implements ConfigAwareFactory { return new DateTime($time, $timeZone !== null ? $timeZone : self::$timeZone); } - - /** - * Return the amount of seconds based on the given month - * - * @param DateTime $dateTime The date and time to use - * - * @return int - */ - public static function getSecondsByMonth(DateTime $dateTime) - { - return (int) $dateTime->format('t') * 24 * 3600; - } - - /** - * Return the amount of seconds based on the given year - * - * @param DateTime $dateTime The date and time to use - * - * @return int - */ - public static function getSecondsByYear(DateTime $dateTime) - { - return (self::isLeapYear($dateTime) ? 366 : 365) * 24 * 3600; - } - - /** - * Return whether the given year is a leap year - * - * @param DateTime $dateTime The date and time to check - * @return bool - */ - public static function isLeapYear(DateTime $dateTime) - { - return $dateTime->format('L') == 1; - } } diff --git a/library/Icinga/Util/Format.php b/library/Icinga/Util/Format.php index b6f97ff98..8c55cbafa 100644 --- a/library/Icinga/Util/Format.php +++ b/library/Icinga/Util/Format.php @@ -29,6 +29,7 @@ namespace Icinga\Util; +use DateTime; use Icinga\Exception\ProgrammingError; class Format @@ -150,4 +151,50 @@ class Format $units[$pow] ); } + + /** + * Return the amount of seconds based on the given month + * + * @param DateTime|int $dateTimeOrTimestamp The date and time to use + * + * @return int + */ + public static function secondsByMonth($dateTimeOrTimestamp) + { + if (!($dt = $dateTimeOrTimestamp) instanceof DateTime) { + $dt = new DateTime(); + $dt->setTimestamp($dateTimeOrTimestamp); + } + + return (int) $dt->format('t') * 24 * 3600; + } + + /** + * Return the amount of seconds based on the given year + * + * @param DateTime|int $dateTimeOrTimestamp The date and time to use + * + * @return int + */ + public static function secondsByYear($dateTimeOrTimestamp) + { + return (self::isLeapYear($dateTimeOrTimestamp) ? 366 : 365) * 24 * 3600; + } + + /** + * Return whether the given year is a leap year + * + * @param DateTime|int $dateTimeOrTimestamp The date and time to use + * + * @return bool + */ + public static function isLeapYear($dateTimeOrTimestamp) + { + if (!($dt = $dateTimeOrTimestamp) instanceof DateTime) { + $dt = new DateTime(); + $dt->setTimestamp($dateTimeOrTimestamp); + } + + return $dt->format('L') == 1; + } } diff --git a/modules/monitoring/application/controllers/TimelineController.php b/modules/monitoring/application/controllers/TimelineController.php index 47917995d..48efe0845 100644 --- a/modules/monitoring/application/controllers/TimelineController.php +++ b/modules/monitoring/application/controllers/TimelineController.php @@ -6,6 +6,7 @@ use \DateTime; use \DateInterval; use \Zend_Config; use Icinga\Web\Url; +use Icinga\Util\Format; use Icinga\Application\Config; use Icinga\Util\DateTimeFactory; use Icinga\Web\Controller\ActionController; @@ -170,13 +171,13 @@ class Monitoring_TimelineController extends ActionController case '1m': $dateCopy = clone $dateTime; for ($i = 0; $i < 6; $i++) { - $dateCopy->sub(new DateInterval('PT' . DateTimeFactory::getSecondsByMonth($dateCopy) . 'S')); + $dateCopy->sub(new DateInterval('PT' . Format::secondsByMonth($dateCopy) . 'S')); } return $dateCopy->add(new DateInterval('PT1S'))->diff($dateTime); case '1y': $dateCopy = clone $dateTime; for ($i = 0; $i < 4; $i++) { - $dateCopy->sub(new DateInterval('PT' . DateTimeFactory::getSecondsByYear($dateCopy) . 'S')); + $dateCopy->sub(new DateInterval('PT' . Format::secondsByYear($dateCopy) . 'S')); } return $dateCopy->add(new DateInterval('PT1S'))->diff($dateTime); default: diff --git a/modules/monitoring/library/Monitoring/Timeline/TimeRange.php b/modules/monitoring/library/Monitoring/Timeline/TimeRange.php index 93c81f98f..7ea4787b8 100644 --- a/modules/monitoring/library/Monitoring/Timeline/TimeRange.php +++ b/modules/monitoring/library/Monitoring/Timeline/TimeRange.php @@ -4,11 +4,11 @@ namespace Icinga\Module\Monitoring\Timeline; -use \StdClass; -use \Iterator; -use \DateTime; -use \DateInterval; -use Icinga\Util\DateTimeFactory; +use StdClass; +use Iterator; +use DateTime; +use DateInterval; +use Icinga\Util\Format; /** * A range of time split into a specific interval @@ -172,17 +172,17 @@ class TimeRange implements Iterator } elseif ($this->interval->m) { for ($i = 0; $i < $this->interval->m; $i++) { if ($this->negative) { - $dateTime->sub(new DateInterval('PT' . DateTimeFactory::getSecondsByMonth($dateTime) . 'S')); + $dateTime->sub(new DateInterval('PT' . Format::secondsByMonth($dateTime) . 'S')); } else { - $dateTime->add(new DateInterval('PT' . DateTimeFactory::getSecondsByMonth($dateTime) . 'S')); + $dateTime->add(new DateInterval('PT' . Format::secondsByMonth($dateTime) . 'S')); } } } elseif ($this->interval->y) { for ($i = 0; $i < $this->interval->y; $i++) { if ($this->negative) { - $dateTime->sub(new DateInterval('PT' . DateTimeFactory::getSecondsByYear($dateTime) . 'S')); + $dateTime->sub(new DateInterval('PT' . Format::secondsByYear($dateTime) . 'S')); } else { - $dateTime->add(new DateInterval('PT' . DateTimeFactory::getSecondsByYear($dateTime) . 'S')); + $dateTime->add(new DateInterval('PT' . Format::secondsByYear($dateTime) . 'S')); } } } From 891d36dbd7c2cd4d4ff5daf1d7fc4acf586a57e7 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 16:38:56 +0200 Subject: [PATCH 11/29] Make DateTimeFactory expecting a string instead of a DateTimeZone --- .../Icinga/Application/ApplicationBootstrap.php | 7 +------ library/Icinga/Application/Web.php | 7 +++---- library/Icinga/Test/BaseTestCase.php | 3 +-- library/Icinga/Util/DateTimeFactory.php | 14 +++++++++----- .../application/views/helpers/DateFormatTest.php | 5 ++--- 5 files changed, 16 insertions(+), 20 deletions(-) diff --git a/library/Icinga/Application/ApplicationBootstrap.php b/library/Icinga/Application/ApplicationBootstrap.php index 342ffd88d..df8d6a0e1 100644 --- a/library/Icinga/Application/ApplicationBootstrap.php +++ b/library/Icinga/Application/ApplicationBootstrap.php @@ -456,13 +456,8 @@ abstract class ApplicationBootstrap protected function setupTimezone() { $timeZoneString = $this->config->global !== null ? $this->config->global->get('timezone', 'UTC') : 'UTC'; - try { - $tz = new DateTimeZone($timeZoneString); - } catch (Exception $e) { - throw new ConfigurationError(t('Invalid timezone') . ' "' . $timeZoneString . '"'); - } date_default_timezone_set($timeZoneString); - DateTimeFactory::setConfig(array('timezone' => $tz)); + DateTimeFactory::setConfig(array('timezone' => $timeZoneString)); return $this; } diff --git a/library/Icinga/Application/Web.php b/library/Icinga/Application/Web.php index 9b6c6463b..6ce858d10 100644 --- a/library/Icinga/Application/Web.php +++ b/library/Icinga/Application/Web.php @@ -337,13 +337,12 @@ class Web extends ApplicationBootstrap } try { - $tz = new DateTimeZone($userTimezone); - } catch (Exception $e) { + DateTimeFactory::setConfig(array('timezone' => $userTimezone)); + date_default_timezone_set($userTimezone); + } catch (ConfigurationError $e) { return parent::setupTimezone(); } - date_default_timezone_set($userTimezone); - DateTimeFactory::setConfig(array('timezone' => $tz)); return $this; } diff --git a/library/Icinga/Test/BaseTestCase.php b/library/Icinga/Test/BaseTestCase.php index 847559739..e426dce9c 100644 --- a/library/Icinga/Test/BaseTestCase.php +++ b/library/Icinga/Test/BaseTestCase.php @@ -22,7 +22,6 @@ namespace { namespace Icinga\Test { use \Exception; - use \DateTimeZone; use \RuntimeException; use \Mockery; use \Zend_Config; @@ -113,7 +112,7 @@ namespace Icinga\Test { public static function setupTimezone() { date_default_timezone_set('UTC'); - DateTimeFactory::setConfig(array('timezone' => new DateTimeZone('UTC'))); + DateTimeFactory::setConfig(array('timezone' => 'UTC')); } /** diff --git a/library/Icinga/Util/DateTimeFactory.php b/library/Icinga/Util/DateTimeFactory.php index 9dd854aa7..413af958c 100644 --- a/library/Icinga/Util/DateTimeFactory.php +++ b/library/Icinga/Util/DateTimeFactory.php @@ -29,8 +29,9 @@ namespace Icinga\Util; -use \DateTime; -use \DateTimeZone; +use Exception; +use DateTime; +use DateTimeZone; use Icinga\Util\ConfigAwareFactory; use Icinga\Exception\ConfigurationError; @@ -55,10 +56,13 @@ class DateTimeFactory implements ConfigAwareFactory */ public static function setConfig($config) { - if (!array_key_exists('timezone', $config)) { - throw new ConfigurationError(t('"DateTimeFactory" expects a valid time zone to be set via "setConfig"')); + try { + $tz = new DateTimeZone(isset($config['timezone']) ? $config['timezone'] : ''); + } catch (Exception $e) { + throw new ConfigurationError('"DateTimeFactory" expects a valid time zone be set via "setConfig"'); } - self::$timeZone = $config['timezone']; + + self::$timeZone = $tz; } /** diff --git a/test/php/application/views/helpers/DateFormatTest.php b/test/php/application/views/helpers/DateFormatTest.php index 04e5e6a90..8de8442f6 100644 --- a/test/php/application/views/helpers/DateFormatTest.php +++ b/test/php/application/views/helpers/DateFormatTest.php @@ -5,7 +5,6 @@ namespace Tests\Icinga\Views\Helper; use Mockery; -use DateTimeZone; use Zend_View_Helper_DateFormat; use Icinga\Test\BaseTestCase; use Icinga\Application\Config; @@ -26,12 +25,12 @@ class DateFormatTest extends BaseTestCase { parent::tearDown(); Config::$configDir = $this->oldConfigDir; - DateTimeFactory::setConfig(array('timezone' => new DateTimeZone(date_default_timezone_get()))); + DateTimeFactory::setConfig(array('timezone' => date_default_timezone_get())); } public function testFormatReturnsCorrectDateWithTimezoneApplied() { - DateTimeFactory::setConfig(array('timezone' => new DateTimeZone('Europe/Berlin'))); + DateTimeFactory::setConfig(array('timezone' => 'Europe/Berlin')); $helper = new Zend_View_Helper_DateFormat($this->getRequestMock()); $this->assertEquals( From e10193f570f7cdfd860fdc6fb76f11476ced2aac Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 17 Apr 2014 16:39:27 +0200 Subject: [PATCH 12/29] Add test for Icinga\Util\DateTimeFactory refs #6011 --- library/Icinga/Util/DateTimeFactory.php | 14 ++-- .../Icinga/Util/DateTimeFactoryTest.php | 70 +++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 test/php/library/Icinga/Util/DateTimeFactoryTest.php diff --git a/library/Icinga/Util/DateTimeFactory.php b/library/Icinga/Util/DateTimeFactory.php index 413af958c..5f6700882 100644 --- a/library/Icinga/Util/DateTimeFactory.php +++ b/library/Icinga/Util/DateTimeFactory.php @@ -42,17 +42,19 @@ class DateTimeFactory implements ConfigAwareFactory { /** * Time zone used throughout DateTime object creation + * * @var DateTimeZone */ - private static $timeZone; + protected static $timeZone; /** * Set the factory's config * * Set the factory's time zone via key timezone in the given config array * - * @param array $config - * @throws \Icinga\Exception\ConfigurationError if the given config is not valid + * @param array $config An array with key 'timezone' + * + * @throws ConfigurationError if the given array misses the key 'timezone' */ public static function setConfig($config) { @@ -66,14 +68,16 @@ class DateTimeFactory implements ConfigAwareFactory } /** - * Return new DateTime object using the given format, time and set time zone + * Return new DateTime object using the given format, time and set timezone * * Wraps DateTime::createFromFormat() * * @param string $format * @param string $time * @param DateTimeZone $timeZone + * * @return DateTime + * * @see DateTime::createFromFormat() */ public static function parse($time, $format, DateTimeZone $timeZone = null) @@ -88,7 +92,9 @@ class DateTimeFactory implements ConfigAwareFactory * * @param string $time * @param DateTimeZone $timeZone + * * @return DateTime + * * @see DateTime::__construct() */ public static function create($time = 'now', DateTimeZone $timeZone = null) diff --git a/test/php/library/Icinga/Util/DateTimeFactoryTest.php b/test/php/library/Icinga/Util/DateTimeFactoryTest.php new file mode 100644 index 000000000..1b7778587 --- /dev/null +++ b/test/php/library/Icinga/Util/DateTimeFactoryTest.php @@ -0,0 +1,70 @@ + 'invalid')); + } + + public function testWhetherParseWorksWithASpecificTimezone() + { + $dt = DateTimeFactory::parse('17-04-14 17:00', 'd-m-y H:i', new DateTimeZone('Europe/Berlin')); + $dt->setTimezone(new DateTimeZone('UTC')); + + $this->assertEquals( + '15', + $dt->format('H'), + 'DateTimeFactory::parse does not properly parse a given datetime or does not respect the given timezone' + ); + } + + public function testWhetherParseWorksWithoutASpecificTimezone() + { + $this->assertEquals( + '15', + DateTimeFactory::parse('17-04-14 15:00', 'd-m-y H:i')->format('H'), + 'DateTimeFactory::parse does not properly parse a given datetime' + ); + } + + public function testWhetherCreateWorksWithASpecificTimezone() + { + $dt = DateTimeFactory::create('2014-04-17 5PM', new DateTimeZone('Europe/Berlin')); + $dt->setTimezone(new DateTimeZone('UTC')); + + $this->assertEquals( + '15', + $dt->format('H'), + 'DateTimeFactory::create does not properly parse a given datetime or does not respect the given timezone' + ); + } + + public function testWhetherCreateWorksWithoutASpecificTimezone() + { + $this->assertEquals( + '15', + DateTimeFactory::create('2014-04-17 3PM')->format('H'), + 'DateTimeFactory::create does not properly parse a given datetime' + ); + } +} From be410a685b8e83d8fc751cbdb7c46ebbe218f5e7 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 09:43:53 +0200 Subject: [PATCH 13/29] Add test for Icinga\User refs #6011 --- library/Icinga/User.php | 100 ++++++++++++++------------- test/php/library/Icinga/UserTest.php | 41 ++++++++--- 2 files changed, 86 insertions(+), 55 deletions(-) diff --git a/library/Icinga/User.php b/library/Icinga/User.php index 5d7887af3..9a759e529 100644 --- a/library/Icinga/User.php +++ b/library/Icinga/User.php @@ -29,18 +29,15 @@ namespace Icinga; -use \DateTimeZone; -use \Exception; -use \InvalidArgumentException; +use DateTimeZone; +use InvalidArgumentException; use Icinga\User\Preferences; use Icinga\User\Message; -use Icinga\Application\Config; /** * This class represents an authorized user * - * You can retrieve authorization information (@TODO: Not implemented yet) or - * to retrieve user information + * You can retrieve authorization information (@TODO: Not implemented yet) or user information */ class User { @@ -49,85 +46,85 @@ class User * * @var string */ - private $username; + protected $username; /** * Firstname * * @var string */ - private $firstname; + protected $firstname; /** * Lastname * * @var string */ - private $lastname; + protected $lastname; /** * Users email address * * @var string */ - private $email; + protected $email; /** * Domain * * @var string */ - private $domain; + protected $domain; /** - * More information about user + * More information about this user * * @var array */ - private $additionalInformation = array(); + protected $additionalInformation = array(); /** * Set of permissions * * @var array */ - private $permissions = array(); + protected $permissions = array(); /** * Set of restrictions * * @var array */ - private $restrictions = array(); + protected $restrictions = array(); /** * Groups for this user * * @var array */ - private $groups = array(); + protected $groups = array(); /** * Preferences object * * @var Preferences */ - private $preferences; + protected $preferences; /** * Queued notifications for this user. * * @var array() */ - private $messages; + protected $messages; /** * Creates a user object given the provided information * - * @param string $username - * @param string $firstname - * @param string $lastname - * @param string $email + * @param string $username + * @param string $firstname + * @param string $lastname + * @param string $email */ public function __construct($username, $firstname = null, $lastname = null, $email = null) { @@ -149,7 +146,7 @@ class User /** * Setter for preferences * - * @param Preferences $preferences + * @param Preferences $preferences */ public function setPreferences(Preferences $preferences) { @@ -159,20 +156,21 @@ class User /** * Getter for preferences * - * @return Preferences + * @return Preferences */ public function getPreferences() { if ($this->preferences === null) { $this->preferences = new Preferences(); } + return $this->preferences; } /** * Return all groups this user belongs to * - * @return array + * @return array */ public function getGroups() { @@ -181,6 +179,8 @@ class User /** * Set the groups this user belongs to + * + * @param array $groups */ public function setGroups(array $groups) { @@ -190,8 +190,9 @@ class User /** * Return true if the user is a member of this group * - * @param string $group - * @return boolean + * @param string $group + * + * @return boolean */ public function isMemberOf($group) { @@ -201,7 +202,7 @@ class User /** * Return permission information for this user * - * @return Array + * @return array */ public function getPermissions() { @@ -211,7 +212,7 @@ class User /** * Setter for permissions * - * @param array $permissions + * @param array $permissions */ public function setPermissions(array $permissions) { @@ -222,6 +223,7 @@ class User * Return restriction information for this user * * @param string $name + * * @return array */ public function getRestrictions($name) @@ -229,13 +231,14 @@ class User if (array_key_exists($name, $this->restrictions)) { return $this->restrictions[$name]; } + return array(); } /** * Settter for restrictions * - * @param array $restrictions + * @param array $restrictions */ public function setRestrictions(array $restrictions) { @@ -245,7 +248,7 @@ class User /** * Getter for username * - * @return string + * @return string */ public function getUsername() { @@ -255,7 +258,7 @@ class User /** * Setter for username * - * @param string $name + * @param string $name */ public function setUsername($name) { @@ -265,7 +268,7 @@ class User /** * Getter for firstname * - * @return string + * @return string */ public function getFirstname() { @@ -275,7 +278,7 @@ class User /** * Setter for firstname * - * @param string $name + * @param string $name */ public function setFirstname($name) { @@ -285,7 +288,7 @@ class User /** * Getter for lastname * - * @return string + * @return string */ public function getLastname() { @@ -295,7 +298,7 @@ class User /** * Setter for lastname * - * @param string $name + * @param string $name */ public function setLastname($name) { @@ -305,7 +308,7 @@ class User /** * Getter for email * - * @return string + * @return string */ public function getEmail() { @@ -315,8 +318,9 @@ class User /** * Setter for mail * - * @param string $mail - * @throws InvalidArgumentException When an invalid mail is provided + * @param string $mail + * + * @throws InvalidArgumentException When an invalid mail is provided */ public function setEmail($mail) { @@ -330,7 +334,7 @@ class User /** * Setter for domain * - * @param string $domain + * @param string $domain */ public function setDomain($domain) { @@ -340,7 +344,7 @@ class User /** * Getter for domain * - * @return string + * @return string */ public function getDomain() { @@ -351,8 +355,8 @@ class User /** * Set additional information about user * - * @param string $key - * @param string $value + * @param string $key + * @param string $value */ public function setAdditional($key, $value) { @@ -362,14 +366,15 @@ class User /** * Getter for additional information * - * @param string $key - * @return mixed|null + * @param string $key + * @return mixed|null */ public function getAdditional($key) { if (isset($this->additionalInformation[$key])) { return $this->additionalInformation[$key]; } + return null; } @@ -386,6 +391,7 @@ class User if ($tz === null) { $tz = date_default_timezone_get(); } + return new DateTimeZone($tz); } @@ -394,7 +400,7 @@ class User * * This function does NOT automatically write to the session, messages will not be persisted until you do. * - * @param Message $msg The message + * @param Message $msg The message */ public function addMessage(Message $msg) { @@ -404,7 +410,7 @@ class User /** * Get all currently pending messages * - * @return array the messages + * @return array The messages */ public function getMessages() { diff --git a/test/php/library/Icinga/UserTest.php b/test/php/library/Icinga/UserTest.php index 4e753308f..ebb39293e 100644 --- a/test/php/library/Icinga/UserTest.php +++ b/test/php/library/Icinga/UserTest.php @@ -4,9 +4,9 @@ namespace Tests\Icinga; -use \DateTimeZone; +use Mockery; +use DateTimeZone; use Icinga\User; -use Icinga\User\Preferences; use Icinga\Test\BaseTestCase; class UserTest extends BaseTestCase @@ -14,9 +14,13 @@ class UserTest extends BaseTestCase public function testGetDefaultTimezoneIfTimezoneNotSet() { $user = new User('unittest'); - $prefs = new Preferences(array()); + $prefs = Mockery::mock('Icinga\User\Preferences'); + $prefs->shouldReceive('get')->with('timezone')->andReturnNull(); $user->setPreferences($prefs); - $this->assertEquals($user->getTimeZone(), new DateTimeZone(date_default_timezone_get()), + + $this->assertEquals( + new DateTimeZone(date_default_timezone_get()), + $user->getTimeZone(), 'User\'s timezone does not match the default timezone' ); } @@ -25,14 +29,35 @@ class UserTest extends BaseTestCase { $explicitTz = 'Europe/Berlin'; $user = new User('unittest'); - $prefs = new Preferences(array( - 'timezone' => $explicitTz - )); + $prefs = Mockery::mock('Icinga\User\Preferences'); + $prefs->shouldReceive('get')->with('timezone')->andReturn($explicitTz); $user->setPreferences($prefs); - $this->assertEquals($user->getTimeZone(), new DateTimeZone($explicitTz), + $this->assertEquals( + new DateTimeZone($explicitTz), + $user->getTimeZone(), 'User\'s timezone does not match the timezone set by himself' ); } + public function testWhetherValidEmailsCanBeSet() + { + $user = new User('unittest'); + $user->setEmail('mySampleEmail@someDomain.org'); + + $this->assertEquals( + $user->getEmail(), + 'mySampleEmail@someDomain.org', + 'Valid emails set with setEmail are not returned by getEmail' + ); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testWhetherInvalidEmailsCannotBeSet() + { + $user = new User('unittest'); + $user->setEmail('mySampleEmail at someDomain dot org'); + } } From d44aaeb8d7ea105d83de37a92c351224496264ed Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 10:18:31 +0200 Subject: [PATCH 14/29] Add test for Icinga\User\Preferences refs #6011 --- library/Icinga/User/Preferences.php | 30 ++++++------ .../library/Icinga/User/PreferencesTest.php | 47 +++++++++++++++++++ 2 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 test/php/library/Icinga/User/PreferencesTest.php diff --git a/library/Icinga/User/Preferences.php b/library/Icinga/User/Preferences.php index 0e4b808d2..25c5e15ce 100644 --- a/library/Icinga/User/Preferences.php +++ b/library/Icinga/User/Preferences.php @@ -44,8 +44,6 @@ use Countable; * * $preferences = new Preferences(array('aPreference' => 'value')); // Start with initial preferences * - * $prefrences = $user->getPreferences(); // Retrieve preferences from a \Icinga\User instance - * * $preferences->aNewPreference = 'value'; // Set a preference * * unset($preferences->aPreference); // Unset a preference @@ -60,12 +58,12 @@ class Preferences implements Countable * * @var array */ - private $preferences = array(); + protected $preferences = array(); /** * Constructor * - * @param array $preferences Preferences key-value array + * @param array $preferences Preferences key-value array */ public function __construct(array $preferences = array()) { @@ -75,7 +73,7 @@ class Preferences implements Countable /** * Count all preferences * - * @return int The number of preferences + * @return int The number of preferences */ public function count() { @@ -85,7 +83,7 @@ class Preferences implements Countable /** * Determine whether a preference exists * - * @param string $name + * @param string $name * * @return bool */ @@ -97,8 +95,8 @@ class Preferences implements Countable /** * Write data to a preference * - * @param string $name - * @param mixed $value + * @param string $name + * @param mixed $value */ public function __set($name, $value) { @@ -108,8 +106,8 @@ class Preferences implements Countable /** * Retrieve a preference and return $default if the preference is not set * - * @param string $name - * @param mixed $default + * @param string $name + * @param mixed $default * * @return mixed */ @@ -118,13 +116,14 @@ class Preferences implements Countable if (array_key_exists($name, $this->preferences)) { return $this->preferences[$name]; } + return $default; } /** * Magic method so that $obj->value will work. * - * @param string $name + * @param string $name * * @return mixed */ @@ -136,7 +135,7 @@ class Preferences implements Countable /** * Remove a given preference * - * @param string $name Preference name + * @param string $name Preference name */ public function remove($name) { @@ -146,7 +145,8 @@ class Preferences implements Countable /** * Determine if a preference is set and is not NULL * - * @param string $name Preference name + * @param string $name Preference name + * * @return bool */ public function __isset($name) @@ -157,7 +157,7 @@ class Preferences implements Countable /** * Unset a given preference * - * @param string $name Preference name + * @param string $name Preference name */ public function __unset($name) { @@ -167,7 +167,7 @@ class Preferences implements Countable /** * Get preferences as array * - * @return array + * @return array */ public function toArray() { diff --git a/test/php/library/Icinga/User/PreferencesTest.php b/test/php/library/Icinga/User/PreferencesTest.php new file mode 100644 index 000000000..da5c318fa --- /dev/null +++ b/test/php/library/Icinga/User/PreferencesTest.php @@ -0,0 +1,47 @@ +key = 'value'; + $this->assertTrue(isset($prefs->key)); + $this->assertEquals('value', $prefs->key); + } + + public function testWhetherPreferencesCanBeAccessed() + { + $prefs = new Preferences(array('key' => 'value')); + + $this->assertTrue($prefs->has('key')); + $this->assertEquals('value', $prefs->get('key')); + } + + public function testWhetherPreferencesCanBeRemoved() + { + $prefs = new Preferences(array('key' => 'value')); + + unset($prefs->key); + $this->assertFalse(isset($prefs->key)); + + $prefs->key = 'value'; + $prefs->remove('key'); + $this->assertFalse($prefs->has('key')); + } + + public function testWhetherPreferencesAreCountable() + { + $prefs = new Preferences(array('key1' => '1', 'key2' => '2')); + + $this->assertEquals(2, count($prefs)); + } +} From 9efe71441ad936c558151aa0144856f42122d6d5 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 12:25:08 +0200 Subject: [PATCH 15/29] Add test for Icinga\User\Preferences\Store\DbStore refs #6011 --- .../Icinga/User/Preferences/Store/DbStore.php | 4 +- .../library/Icinga/User/Store/DbStoreTest.php | 176 ++++++++++++++++++ 2 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 test/php/library/Icinga/User/Store/DbStoreTest.php diff --git a/library/Icinga/User/Preferences/Store/DbStore.php b/library/Icinga/User/Preferences/Store/DbStore.php index 367552b7f..e60746c1e 100644 --- a/library/Icinga/User/Preferences/Store/DbStore.php +++ b/library/Icinga/User/Preferences/Store/DbStore.php @@ -4,8 +4,8 @@ namespace Icinga\User\Preferences\Store; -use \Exception; -use \Zend_Db_Select; +use Exception; +use Zend_Db_Select; use Icinga\Exception\NotReadableError; use Icinga\Exception\NotWritableError; use Icinga\User\Preferences; diff --git a/test/php/library/Icinga/User/Store/DbStoreTest.php b/test/php/library/Icinga/User/Store/DbStoreTest.php new file mode 100644 index 000000000..6af17bcba --- /dev/null +++ b/test/php/library/Icinga/User/Store/DbStoreTest.php @@ -0,0 +1,176 @@ +insertions[$row[DbStore::COLUMN_PREFERENCE]] = $row[DbStore::COLUMN_VALUE]; + } + + public function update($table, $columns, $where) + { + $this->updates[$where[DbStore::COLUMN_PREFERENCE . '=?']] = $columns[DbStore::COLUMN_VALUE]; + } + + public function delete($table, $where) + { + $this->deletions = array_merge( + $this->deletions, + $where[DbStore::COLUMN_PREFERENCE . ' IN (?)'] + ); + } +} + +class FaultyDatabaseMock extends DatabaseMock +{ + public function insert($table, $row) + { + throw new Exception(); + } + + public function update($table, $columns, $where) + { + throw new Exception(); + } + + public function delete($table, $where) + { + throw new Exception(); + } +} + +class DbStoreWithSetPreferences extends DbStore +{ + public function setPreferences(array $preferences) + { + $this->preferences = $preferences; + } +} + +class DbStoreTest extends BaseTestCase +{ + public function testWhetherPreferenceInsertionWorks() + { + $dbMock = new DatabaseMock(); + $store = $this->getStore($dbMock); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array('key' => 'value')) + ) + ); + + $this->assertArrayHasKey('key', $dbMock->insertions, 'DbStore::save does not insert new preferences'); + $this->assertEmpty($dbMock->updates, 'DbStore::save updates *new* preferences'); + $this->assertEmpty($dbMock->deletions, 'DbStore::save deletes *new* preferences'); + } + + /** + * @expectedException Icinga\Exception\NotWritableError + */ + public function testWhetherPreferenceInsertionThrowsNotWritableError() + { + $store = $this->getStore(new FaultyDatabaseMock()); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array('key' => 'value')) + ) + ); + } + + public function testWhetherPreferenceUpdatesWork() + { + $dbMock = new DatabaseMock(); + $store = $this->getStore($dbMock); + $store->setPreferences(array('key' => 'value')); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array('key' => 'eulav')) + ) + ); + + $this->assertArrayHasKey('key', $dbMock->updates, 'DbStore::save does not update existing preferences'); + $this->assertEmpty($dbMock->insertions, 'DbStore::save inserts *existing* preferences'); + $this->assertEmpty($dbMock->deletions, 'DbStore::save inserts *existing* preferneces'); + } + + /** + * @expectedException Icinga\Exception\NotWritableError + */ + public function testWhetherPreferenceUpdatesThrowNotWritableError() + { + $store = $this->getStore(new FaultyDatabaseMock()); + $store->setPreferences(array('key' => 'value')); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array('key' => 'eulav')) + ) + ); + } + + public function testWhetherPreferenceDeletionWorks() + { + $dbMock = new DatabaseMock(); + $store = $this->getStore($dbMock); + $store->setPreferences(array('key' => 'value')); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array()) + ) + ); + + $this->assertContains('key', $dbMock->deletions, 'DbStore::save does not delete removed preferences'); + $this->assertEmpty($dbMock->insertions, 'DbStore::save inserts *removed* preferences'); + $this->assertEmpty($dbMock->updates, 'DbStore::save updates *removed* preferences'); + } + + /** + * @expectedException Icinga\Exception\NotWritableError + */ + public function testWhetherPreferenceDeletionThrowsNotWritableError() + { + $store = $this->getStore(new FaultyDatabaseMock()); + $store->setPreferences(array('key' => 'value')); + $store->save( + Mockery::mock( + 'Icinga\User\Preferences', + array('toArray' => array()) + ) + ); + } + + protected function getStore($dbMock) + { + return new DbStoreWithSetPreferences( + new Zend_Config( + array( + 'connection' => Mockery::mock(array('getConnection' => $dbMock)) + ) + ), + Mockery::mock('Icinga\User', array('getUsername' => 'unittest')) + ); + } +} From c93159d287679af930b7a2f2e5b725bbe0721fad Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 13:00:03 +0200 Subject: [PATCH 16/29] Add test for Icinga\User\Preferences\Store\IniStore refs #6011 --- .../User/Preferences/Store/IniStore.php | 2 +- .../Icinga/User/Store/IniStoreTest.php | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 test/php/library/Icinga/User/Store/IniStoreTest.php diff --git a/library/Icinga/User/Preferences/Store/IniStore.php b/library/Icinga/User/Preferences/Store/IniStore.php index c3521b422..7363f2569 100644 --- a/library/Icinga/User/Preferences/Store/IniStore.php +++ b/library/Icinga/User/Preferences/Store/IniStore.php @@ -4,7 +4,7 @@ namespace Icinga\User\Preferences\Store; -use \Zend_Config; +use Zend_Config; use Icinga\Util\File; use Icinga\Config\PreservingIniWriter; use Icinga\Exception\NotReadableError; diff --git a/test/php/library/Icinga/User/Store/IniStoreTest.php b/test/php/library/Icinga/User/Store/IniStoreTest.php new file mode 100644 index 000000000..d6637d2f2 --- /dev/null +++ b/test/php/library/Icinga/User/Store/IniStoreTest.php @@ -0,0 +1,67 @@ +preferences = $preferences; + } + + public function getPreferences() + { + return $this->preferences; + } +} + +class IniStoreTest extends BaseTestCase +{ + public function testWhetherPreferenceChangesAreApplied() + { + $store = $this->getStore(); + $store->setPreferences(array('key1' => '1')); + + $store->save( + Mockery::mock('Icinga\User\Preferences', array('toArray' => array('key1' => '11', 'key2' => '2'))) + ); + $this->assertEquals( + array('key1' => '11', 'key2' => '2'), + $store->getPreferences(), + 'IniStore::save does not properly apply changed preferences' + ); + } + + public function testWhetherPreferenceDeletionsAreApplied() + { + $store = $this->getStore(); + $store->setPreferences(array('key' => 'value')); + + $store->save(Mockery::mock('Icinga\User\Preferences', array('toArray' => array()))); + $this->assertEmpty($store->getPreferences(), 'IniStore::save does not delete removed preferences'); + } + + protected function getStore() + { + return new IniStoreWithSetGetPreferencesAndEmptyWrite( + new Zend_Config( + array( + 'location' => 'some/Path/To/Some/Directory' + ) + ), + Mockery::mock('Icinga\User', array('getUsername' => 'unittest')) + ); + } +} From 45d7864198f2d87150e2f90b68c8657901cdc19e Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 14:00:52 +0200 Subject: [PATCH 17/29] Refactor test for Icinga\Test\BaseTestCase refs #6011 --- library/Icinga/Test/BaseTestCase.php | 12 +- ...estCaseDbTest.php => BaseTestCaseTest.php} | 145 +++++++++++------- 2 files changed, 93 insertions(+), 64 deletions(-) rename test/php/library/Icinga/Test/{BaseTestCaseDbTest.php => BaseTestCaseTest.php} (52%) diff --git a/library/Icinga/Test/BaseTestCase.php b/library/Icinga/Test/BaseTestCase.php index e426dce9c..d88245ccd 100644 --- a/library/Icinga/Test/BaseTestCase.php +++ b/library/Icinga/Test/BaseTestCase.php @@ -21,11 +21,11 @@ namespace { namespace Icinga\Test { - use \Exception; - use \RuntimeException; - use \Mockery; - use \Zend_Config; - use \Zend_Test_PHPUnit_ControllerTestCase; + use Exception; + use RuntimeException; + use Mockery; + use Zend_Config; + use Zend_Test_PHPUnit_ControllerTestCase; use Icinga\Application\Icinga; use Icinga\Util\DateTimeFactory; use Icinga\Data\ResourceFactory; @@ -85,7 +85,7 @@ namespace Icinga\Test { * * @var array */ - private static $dbConfiguration = array( + protected static $dbConfiguration = array( 'mysql' => array( 'type' => 'db', 'db' => 'mysql', diff --git a/test/php/library/Icinga/Test/BaseTestCaseDbTest.php b/test/php/library/Icinga/Test/BaseTestCaseTest.php similarity index 52% rename from test/php/library/Icinga/Test/BaseTestCaseDbTest.php rename to test/php/library/Icinga/Test/BaseTestCaseTest.php index 21b5c53a1..043d3c839 100644 --- a/test/php/library/Icinga/Test/BaseTestCaseDbTest.php +++ b/test/php/library/Icinga/Test/BaseTestCaseTest.php @@ -4,15 +4,17 @@ namespace Tests\Icinga\Test; +use Mockery; use Icinga\Test\BaseTestCase; -class BaseTestCaseDbTest extends BaseTestCase +class BaseTestCaseTest extends BaseTestCase { - private $emptySqlDumpFile; + protected $emptySqlDumpFile; public function tearDown() { parent::tearDown(); + if ($this->emptySqlDumpFile) { unlink($this->emptySqlDumpFile); } @@ -21,7 +23,7 @@ class BaseTestCaseDbTest extends BaseTestCase /** * @dataProvider mysqlDb */ - public function testMySqlProviderAnnotation($resource) + public function testWhetherMySqlProviderAnnotationSetsUpZendDbAdapter($resource) { $this->setupDbProvider($resource); $this->assertInstanceOf('Zend_Db_Adapter_Pdo_Mysql', $resource->getConnection()); @@ -30,7 +32,16 @@ class BaseTestCaseDbTest extends BaseTestCase /** * @dataProvider mysqlDb */ - public function testMySqlCreateTablePart1($resource) + public function testWhetherMySqlAdapterWorks($resource) + { + $this->setupDbProvider($resource); + $this->dbAdapterSqlLoadTable($resource); + } + + /** + * @dataProvider mysqlDb + */ + public function testWhetherCreatingTablesWithMySqlAdapterWorks($resource) { $this->setupDbProvider($resource); $adapter = $resource->getConnection(); @@ -42,47 +53,20 @@ class BaseTestCaseDbTest extends BaseTestCase /** * @dataProvider mysqlDb + * @depends testWhetherCreatingTablesWithMySqlAdapterWorks */ - public function testMySqlCreateTablePart2($resource) + public function testWhetherSetupDbProviderCleansUpMySqlAdapter($resource) { $this->setupDbProvider($resource); + $tables = $resource->getConnection()->listTables(); $this->assertCount(0, $tables); } - private function dbAdapterSqlLoadTable($resource) - { - $this->setupDbProvider($resource); - - $sqlContent = array(); - $sqlContent[] = 'CREATE TABLE dummyData(value VARCHAR(50) NOT NULL PRIMARY KEY);'; - for ($i=0; $i<20; $i++) { - $sqlContent[] = 'INSERT INTO dummyData VALUES(\'' . uniqid(). '\');'; - } - - $tempFile = tempnam(sys_get_temp_dir(), 'icinga2-web-test-load-sql'); - file_put_contents($tempFile, implode(chr(10), $sqlContent)); - - $this->loadSql($resource, $tempFile); - - $count = (int) $resource->getConnection()->fetchOne('SELECT COUNT(*) as cntX from dummyData;'); - $this->assertSame(20, $count); - - $this->assertTrue(unlink($tempFile)); - } - - /** - * @dataProvider mysqlDb - */ - public function testMySqlLoadTable($resource) - { - $this->dbAdapterSqlLoadTable($resource); - } - /** * @dataProvider pgsqlDb */ - public function testPgSqlProviderAnnotation($resource) + public function testWhetherPgSqlProviderAnnotationSetsUpZendDbAdapter($resource) { $this->setupDbProvider($resource); $this->assertInstanceOf('Zend_Db_Adapter_Pdo_Pgsql', $resource->getConnection()); @@ -91,7 +75,16 @@ class BaseTestCaseDbTest extends BaseTestCase /** * @dataProvider pgsqlDb */ - public function testPgSqlCreateTablePart1($resource) + public function testWhetherPgSqlAdapterWorks($resource) + { + $this->setupDbProvider($resource); + $this->dbAdapterSqlLoadTable($resource); + } + + /** + * @dataProvider pgsqlDb + */ + public function testWhetherCreatingTablesWithPgSqlAdapterWorks($resource) { $this->setupDbProvider($resource); $adapter = $resource->getConnection(); @@ -103,56 +96,92 @@ class BaseTestCaseDbTest extends BaseTestCase /** * @dataProvider pgsqlDb + * @depends testWhetherCreatingTablesWithPgSqlAdapterWorks */ - public function testPgSqlCreateTablePart2($resource) + public function testWhetherSetupDbProviderCleansUpPgSqlAdapter($resource) { $this->setupDbProvider($resource); + $tables = $resource->getConnection()->listTables(); $this->assertCount(0, $tables); } /** - * @dataProvider pgsqlDb + * @dataProvider oracleDb */ - public function testPgSqlLoadTable($resource) + public function testWhetherOciProviderAnnotationSetsUpZendDbAdapter($resource) { + $this->setupDbProvider($resource); + $this->assertInstanceOf('Zend_Db_Adapter_Pdo_Oci', $resource->getConnection()); + } + + /** + * @dataProvider oracleDb + */ + public function testWhetherOciAdapterWorks($resource) + { + $this->setupDbProvider($resource); $this->dbAdapterSqlLoadTable($resource); } /** - * @dataProvider mysqlDb + * @dataProvider oracleDb */ - public function testNotExistSqlDumpFile($resource) + public function testWhetherCreatingTablesWithOciAdapterWorks($resource) { $this->setupDbProvider($resource); + $adapter = $resource->getConnection(); + $adapter->exec('CREATE TABLE test(uid INT NOT NULL PRIMARY KEY);'); - $this->setExpectedException( - 'RuntimeException', - 'Sql file not found: /does/not/exist1238837 (test=testNotExistSqlDumpFile with data set #0)' - ); - - $this->loadSql($resource, '/does/not/exist1238837'); + $tables = $adapter->listTables(); + $this->assertCount(1, $tables); } /** - * @dataProvider mysqlDb + * @dataProvider oracleDb + * @depends testWhetherCreatingTablesWithOciAdapterWorks */ - public function testDumpFileIsEmpty($resource) + public function testWhetherSetupDbProviderCleansUpOciAdapter($resource) { $this->setupDbProvider($resource); + + $tables = $resource->getConnection()->listTables(); + $this->assertCount(0, $tables); + } + + /** + * @expectedException RuntimeException + */ + public function testWhetherLoadSqlThrowsErrorWhenFileMissing() + { + $this->loadSql(Mockery::mock('Icinga\Data\Db\Connection'), 'not_existing'); + } + + /** + * @expectedException RuntimeException + */ + public function testWhetherLoadSqlThrowsErrorWhenFileEmpty() + { $this->emptySqlDumpFile = tempnam(sys_get_temp_dir(), 'icinga2-web-db-test-empty'); - $this->assertFileExists($this->emptySqlDumpFile); + $this->loadSql(Mockery::mock('Icinga\Data\Db\Connection'), $this->emptySqlDumpFile); + } - $expectedMessage = 'Sql file is empty: ' - . $this->emptySqlDumpFile - . ' (test=testDumpFileIsEmpty with data set #0)'; + protected function dbAdapterSqlLoadTable($resource) + { + $sqlContent = array(); + $sqlContent[] = 'CREATE TABLE dummyData(value VARCHAR(50) NOT NULL PRIMARY KEY);'; + for ($i=0; $i<20; $i++) { + $sqlContent[] = 'INSERT INTO dummyData VALUES(\'' . uniqid(). '\');'; + } - $this->setExpectedException( - 'RuntimeException', - $expectedMessage - ); + $tempFile = tempnam(sys_get_temp_dir(), 'icinga2-web-test-load-sql'); + file_put_contents($tempFile, implode(PHP_EOL, $sqlContent)); - $this->loadSql($resource, $this->emptySqlDumpFile); + $this->loadSql($resource, $tempFile); + $count = (int) $resource->getConnection()->fetchOne('SELECT COUNT(*) as cntX from dummyData;'); + $this->assertSame(20, $count); + + $this->assertTrue(unlink($tempFile)); } } From 54a5e996bb18cd752d520bcb788a1820a0470558 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 14:31:57 +0200 Subject: [PATCH 18/29] Add test for Icinga/Logger/Writer/StreamWriter refs #6011 --- library/Icinga/Logger/LogWriter.php | 2 +- library/Icinga/Logger/Logger.php | 4 +- library/Icinga/Logger/Writer/StreamWriter.php | 11 +++- library/Icinga/Logger/Writer/SyslogWriter.php | 4 +- .../library/Icinga/Application/LoggerTest.php | 53 ------------------- .../Icinga/Logger/Writer/StreamWriterTest.php | 44 +++++++++++++++ 6 files changed, 58 insertions(+), 60 deletions(-) delete mode 100644 test/php/library/Icinga/Application/LoggerTest.php create mode 100644 test/php/library/Icinga/Logger/Writer/StreamWriterTest.php diff --git a/library/Icinga/Logger/LogWriter.php b/library/Icinga/Logger/LogWriter.php index 4c1524ca1..3222e50be 100644 --- a/library/Icinga/Logger/LogWriter.php +++ b/library/Icinga/Logger/LogWriter.php @@ -4,7 +4,7 @@ namespace Icinga\Logger; -use \Zend_Config; +use Zend_Config; /** * Abstract class for writers that write messages to a log diff --git a/library/Icinga/Logger/Logger.php b/library/Icinga/Logger/Logger.php index bdeadd0dc..31b0452d2 100644 --- a/library/Icinga/Logger/Logger.php +++ b/library/Icinga/Logger/Logger.php @@ -4,8 +4,8 @@ namespace Icinga\Logger; -use \Exception; -use \Zend_Config; +use Exception; +use Zend_Config; use Icinga\Exception\ConfigurationError; /** diff --git a/library/Icinga/Logger/Writer/StreamWriter.php b/library/Icinga/Logger/Writer/StreamWriter.php index f0336cda0..981d01d03 100644 --- a/library/Icinga/Logger/Writer/StreamWriter.php +++ b/library/Icinga/Logger/Writer/StreamWriter.php @@ -4,7 +4,8 @@ namespace Icinga\Logger\Writer; -use \Zend_Config; +use Exception; +use Zend_Config; use Icinga\Logger\Logger; use Icinga\Logger\LogWriter; use Icinga\Application\Config; @@ -87,11 +88,17 @@ class StreamWriter extends LogWriter * Write a message to the stream * * @param string $text The message to write + * + * @throws Exception In case write acess to the stream failed */ protected function write($text) { $fd = fopen($this->stream, 'a'); - fwrite($fd, $text . PHP_EOL); + + if ($fd === false || fwrite($fd, $text . PHP_EOL) === false) { + throw new Exception('Failed to write to log file "' . $this->stream . '"'); + } + fclose($fd); } } diff --git a/library/Icinga/Logger/Writer/SyslogWriter.php b/library/Icinga/Logger/Writer/SyslogWriter.php index e78971304..fea20b592 100644 --- a/library/Icinga/Logger/Writer/SyslogWriter.php +++ b/library/Icinga/Logger/Writer/SyslogWriter.php @@ -4,8 +4,8 @@ namespace Icinga\Logger\Writer; -use \Exception; -use \Zend_Config; +use Exception; +use Zend_Config; use Icinga\Logger\Logger; use Icinga\Logger\LogWriter; use Icinga\Exception\ConfigurationError; diff --git a/test/php/library/Icinga/Application/LoggerTest.php b/test/php/library/Icinga/Application/LoggerTest.php deleted file mode 100644 index 02bab4ef1..000000000 --- a/test/php/library/Icinga/Application/LoggerTest.php +++ /dev/null @@ -1,53 +0,0 @@ - true, - 'level' => Logger::$ERROR, - 'type' => 'stream', - 'target' => $target - ) - ) - ); - $this->assertFileExists($target, 'Logger did not create the log file'); - unlink($target); - } - - /** - * @depends testLogfileCreation - */ - public function testLoggingErrorMessages() - { - $target = tempnam(sys_get_temp_dir(), 'log'); - unlink($target); - $logger = new Logger( - new Zend_Config( - array( - 'enable' => true, - 'level' => Logger::$ERROR, - 'type' => 'stream', - 'target' => $target - ) - ) - ); - $logger->log('This is a test error', Logger::$ERROR); - $log = file_get_contents($target); - unlink($target); - $this->assertContains('This is a test error', $log, 'Log does not contain the error "This is a test error"'); - } -} diff --git a/test/php/library/Icinga/Logger/Writer/StreamWriterTest.php b/test/php/library/Icinga/Logger/Writer/StreamWriterTest.php new file mode 100644 index 000000000..12febf1f7 --- /dev/null +++ b/test/php/library/Icinga/Logger/Writer/StreamWriterTest.php @@ -0,0 +1,44 @@ +target = tempnam(sys_get_temp_dir(), 'log'); + } + + public function tearDown() + { + parent::tearDown(); + + unlink($this->target); + } + + public function testWhetherStreamWriterCreatesMissingFiles() + { + new StreamWriter(new Zend_Config(array('target' => $this->target))); + $this->assertFileExists($this->target, 'StreamWriter does not create missing files on initialization'); + } + + /** + * @depends testWhetherStreamWriterCreatesMissingFiles + */ + public function testWhetherStreamWriterWritesMessages() + { + $writer = new StreamWriter(new Zend_Config(array('target' => $this->target))); + $writer->log(Logger::$ERROR, 'This is a test error'); + $log = file_get_contents($this->target); + $this->assertContains('This is a test error', $log, 'StreamWriter does not write log messages'); + } +} From fe44f8ed7c354976ca10eeb0af9473f7393fa1e0 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Apr 2014 16:19:09 +0200 Subject: [PATCH 19/29] Remove empty file library/Icinga/File/Csv/Query.php --- library/Icinga/File/Csv/Query.php | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 library/Icinga/File/Csv/Query.php diff --git a/library/Icinga/File/Csv/Query.php b/library/Icinga/File/Csv/Query.php deleted file mode 100644 index e69de29bb..000000000 From e8ed7c7166f5130330b0fe1ddaba4a98300265f1 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 23 Apr 2014 13:48:43 +0200 Subject: [PATCH 20/29] Rewrite test for Icinga\Config\PreservingIniWriter refs #6011 --- library/Icinga/Config/PreservingIniWriter.php | 94 +- .../Icinga/Config/PreservingIniWriterTest.php | 880 ++++++++++++------ 2 files changed, 609 insertions(+), 365 deletions(-) diff --git a/library/Icinga/Config/PreservingIniWriter.php b/library/Icinga/Config/PreservingIniWriter.php index d8dc8cf6d..d2456cdd2 100644 --- a/library/Icinga/Config/PreservingIniWriter.php +++ b/library/Icinga/Config/PreservingIniWriter.php @@ -29,14 +29,13 @@ namespace Icinga\Config; -use \Zend_Config; -use \Zend_Config_Ini; -use \Zend_Config_Writer_FileAbstract; -use \Icinga\Config\IniEditor; +use Zend_Config; +use Zend_Config_Ini; +use Zend_Config_Writer_FileAbstract; +use Icinga\Config\IniEditor; /** - * A ini file adapter that respects the file structure and the comments of already - * existing ini files + * A ini file adapter that respects the file structure and the comments of already existing ini files */ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract { @@ -45,7 +44,7 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract * * @var array */ - private $options; + protected $options; /** * Create a new PreservingIniWriter @@ -67,7 +66,7 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract /** * Render the Zend_Config into a config file string * - * @return string + * @return string */ public function render() { @@ -86,12 +85,12 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract /** * Create a property diff and apply the changes to the editor * - * @param Zend_Config $oldconfig The config representing the state before the change - * @param Zend_Config $newconfig The config representing the state after the change - * @param IniEditor $editor The editor that should be used to edit the old config file - * @param array $parents The parent keys that should be respected when editing the config + * @param Zend_Config $oldconfig The config representing the state before the change + * @param Zend_Config $newconfig The config representing the state after the change + * @param IniEditor $editor The editor that should be used to edit the old config file + * @param array $parents The parent keys that should be respected when editing the config */ - private function diffConfigs( + protected function diffConfigs( Zend_Config $oldconfig, Zend_Config $newconfig, IniEditor $editor, @@ -102,13 +101,10 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract } /** - * Update the order of the sections in the ini file to match - * the order of the new config + * Update the order of the sections in the ini file to match the order of the new config */ - private function updateSectionOrder( - Zend_Config $newconfig, - IniEditor $editor - ) { + protected function updateSectionOrder(Zend_Config $newconfig, IniEditor $editor) + { $order = array(); foreach ($newconfig as $key => $value) { if ($value instanceof Zend_Config) { @@ -126,48 +122,35 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract * @param IniEditor $editor The editor that should be used to edit the old config file * @param array $parents The parent keys that should be respected when editing the config */ - private function diffPropertyUpdates( + protected function diffPropertyUpdates( Zend_Config $oldconfig, Zend_Config $newconfig, IniEditor $editor, array $parents = array() ) { - /* - * The current section. This value is null when processing - * the section-less root element - */ + // The current section. This value is null when processing the section-less root element $section = empty($parents) ? null : $parents[0]; - /* - * Iterate over all properties in the new configuration file and search for changes - */ + // Iterate over all properties in the new configuration file and search for changes foreach ($newconfig as $key => $value) { $oldvalue = $oldconfig->get($key); $nextParents = array_merge($parents, array($key)); - $keyIdentifier = empty($parents) ? - array($key) : array_slice($nextParents, 1, null, true); + $keyIdentifier = empty($parents) ? array($key) : array_slice($nextParents, 1, null, true); if ($value instanceof Zend_Config) { - /* - * The value is a nested Zend_Config, handle it recursively - */ - if (!isset($section)) { - /* - * Update the section declaration - */ + // The value is a nested Zend_Config, handle it recursively + if ($section === null) { + // Update the section declaration $extends = $newconfig->getExtends(); - $extend = array_key_exists($key, $extends) ? - $extends[$key] : null; + $extend = array_key_exists($key, $extends) ? $extends[$key] : null; $editor->setSection($key, $extend); } - if (!isset($oldvalue)) { + if ($oldvalue === null) { $oldvalue = new Zend_Config(array()); } $this->diffConfigs($oldvalue, $value, $editor, $nextParents); } else { - /* - * The value is a plain value, use the editor to set it - */ + // The value is a plain value, use the editor to set it if (is_numeric($key)) { $editor->setArrayElement($keyIdentifier, $value, $section); } else { @@ -185,41 +168,30 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract * @param IniEditor $editor The editor that should be used to edit the old config file * @param array $parents The parent keys that should be respected when editing the config */ - private function diffPropertyDeletions( + protected function diffPropertyDeletions( Zend_Config $oldconfig, Zend_Config $newconfig, IniEditor $editor, array $parents = array() ) { - /* - * The current section. This value is null when processing - * the section-less root element - */ + // The current section. This value is null when processing the section-less root element $section = empty($parents) ? null : $parents[0]; - /* - * Iterate over all properties in the old configuration file and search for - * deleted properties - */ + // Iterate over all properties in the old configuration file and search for deleted properties foreach ($oldconfig as $key => $value) { $nextParents = array_merge($parents, array($key)); $newvalue = $newconfig->get($key); - $keyIdentifier = empty($parents) ? - array($key) : array_slice($nextParents, 1, null, true); + $keyIdentifier = empty($parents) ? array($key) : array_slice($nextParents, 1, null, true); - if (!isset($newvalue)) { + if ($newvalue === null) { if ($value instanceof Zend_Config) { - /* - * The deleted value is a nested Zend_Config, handle it recursively - */ + // The deleted value is a nested Zend_Config, handle it recursively $this->diffConfigs($value, new Zend_Config(array()), $editor, $nextParents); - if (!isset($section)) { + if ($section === null) { $editor->removeSection($key); } } else { - /* - * The deleted value is a plain value, use the editor to delete it - */ + // The deleted value is a plain value, use the editor to delete it if (is_numeric($key)) { $editor->resetArrayElement($keyIdentifier, $section); } else { diff --git a/test/php/library/Icinga/Config/PreservingIniWriterTest.php b/test/php/library/Icinga/Config/PreservingIniWriterTest.php index 88adbad29..db83c9b03 100644 --- a/test/php/library/Icinga/Config/PreservingIniWriterTest.php +++ b/test/php/library/Icinga/Config/PreservingIniWriterTest.php @@ -4,362 +4,634 @@ namespace Tests\Icinga\Config; -use \Zend_Config; +use Zend_Config; +use Zend_Config_Ini; use Icinga\Test\BaseTestCase; use Icinga\Config\PreservingIniWriter; class PreservingIniWriterTest extends BaseTestCase { - private $tmpfiles = array(); + protected $tempFile; - /** - * Set up the test fixture - */ public function setUp() { parent::setUp(); - $ini = -' -trailing1="wert" -arr[]="0" -arr[]="1" -arr[]="2" -arr[]="3" - -Trailing2= - -;1 -;2 -;3 -[section] -property = "some value" ; Some " ; comment" -property2 = "some ;value" ; Some comment with " quotes " -property3.nest1.nest2 = "value" ; ; - -[parent] -;4 -;5 -;6 -;7 -list[]="zero" -list[]="one" - -;8 -;9 -many.many.nests="value" -propOne="value1" -propTwo="2" -propThree= -propFour="true" - -Prop5="true" - -[child : parent] -PropOne="overwritten" -;10 -'; - $this->writeToTmp('orig', $ini); - - $emptyIni = " "; - $this->writeToTmp('empty', $emptyIni); - - $editedIni = -';1 -;2 -;3 -;4 -;5 -trailing1="1" - -[parent] -;6 -;7 -;8 -;9 -;10 -propOne="value1" - -[different] -prop1="1" -prop2="2" - -[nested : different] -prop2="5" -'; - $this->writeToTmp('edited', $editedIni); + $this->tempFile = tempnam(sys_get_temp_dir(), 'icinga-ini-writer-test'); } - /** - * Write a string to a temporary file - * - * @param string $name The name of the temporary file - * @param string $content The content - */ - private function writeToTmp($name, $content) - { - $this->tmpfiles[$name] = - tempnam(dirname(__FILE__) . '/temp', $name); - $file = fopen($this->tmpfiles[$name], 'w'); - fwrite($file, $content); - fflush($file); - fclose($file); - } - - /** - * Tear down the test fixture - */ public function tearDown() { parent::tearDown(); - foreach ($this->tmpfiles as $filename) { - unlink($filename); - } + + unlink($this->tempFile); + } + + public function testWhetherSimplePropertiesAreInsertedInEmptyFiles() + { + $target = $this->writeConfigToTemporaryFile(''); + $config = new Zend_Config(array('key' => 'value')); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals('value', $newConfig->get('key'), 'PreservingIniWriter does not insert in empty files'); + } + + public function testWhetherSimplePropertiesAreInsertedInExistingFiles() + { + $target = $this->writeConfigToTemporaryFile('key1 = "1"'); + $config = new Zend_Config(array('key2' => '2')); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals('2', $newConfig->get('key2'), 'PreservingIniWriter does not insert in existing files'); } /** - * Test if the IniWriter works correctly when writing the changes back to - * the same ini file + * @depends testWhetherSimplePropertiesAreInsertedInExistingFiles */ - public function testPropertyChangeSameConfig() + public function testWhetherSimplePropertiesAreUpdated() { - $this->changeConfigAndWriteToFile('orig'); - $config = new \Zend_Config_Ini( - $this->tmpfiles['orig'], null, array('allowModifications' => true) + $target = $this->writeConfigToTemporaryFile('key = "value"'); + $config = new Zend_Config(array('key' => 'eulav')); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals('eulav', $newConfig->get('key'), 'PreservingIniWriter does not update simple properties'); + } + + /** + * @depends testWhetherSimplePropertiesAreInsertedInExistingFiles + */ + public function testWhetherSimplePropertiesAreDeleted() + { + $target = $this->writeConfigToTemporaryFile('key = "value"'); + $config = new Zend_Config(array()); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertNull($newConfig->get('key'), 'PreservingIniWriter does not delete simple properties'); + } + + public function testWhetherNestedPropertiesAreInserted() + { + $target = $this->writeConfigToTemporaryFile(''); + $config = new Zend_Config(array('a' => array('b' => 'c'))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('a'), + 'PreservingIniWriter does not insert nested properties' ); - $this->checkConfigProperties($config); - $this->checkConfigComments($this->tmpfiles['orig']); - } - - /** - * Test if the IniWriter works correctly when writing to an empty file - */ - public function testPropertyChangeEmptyConfig() - { - $this->changeConfigAndWriteToFile('empty'); - $config = new \Zend_Config_Ini( - $this->tmpfiles['empty'], null, array('allowModifications' => true) + $this->assertEquals( + 'c', + $newConfig->get('a')->get('b'), + 'PreservingIniWriter does not insert nested properties' ); - $this->checkConfigProperties($config); } /** - * Test if the IniWriter works correctly when writing to a file with changes + * @depends testWhetherNestedPropertiesAreInserted */ - public function testPropertyChangeEditedConfig() + public function testWhetherNestedPropertiesAreUpdated() { - $original = $this->changeConfigAndWriteToFile('edited'); - $config = new \Zend_Config_Ini( - $this->tmpfiles['edited'], null, array('allowModifications' => true) + $target = $this->writeConfigToTemporaryFile('a.b = "c"'); + $config = new Zend_Config(array('a' => array('b' => 'cc'))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('a'), + 'PreservingIniWriter does not update nested properties' + ); + $this->assertEquals( + 'cc', + $newConfig->get('a')->get('b'), + 'PreservingIniWriter does not update nested properties' ); - $this->checkConfigProperties($config); - $this->checkConfigComments($this->tmpfiles['edited']); } /** - * Test if the order of sections is correctly changed in the config. + * @depends testWhetherNestedPropertiesAreInserted */ - public function testSectionOrderChange() + public function testWhetherNestedPropertiesAreDeleted() { - $original = ' -;1 + $target = $this->writeConfigToTemporaryFile('a.b = "c"'); + $config = new Zend_Config(array()); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); -[section2] -;3 + $newConfig = new Zend_Config_Ini($target); + $this->assertNull( + $newConfig->get('a'), + 'PreservingIniWriter does not delete nested properties' + ); + } -;4 -[section3] -;5 + public function testWhetherSimpleSectionPropertiesAreInserted() + { + $target = $this->writeConfigToTemporaryFile(''); + $config = new Zend_Config(array('section' => array('key' => 'value'))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); -;2 -[section1] -property = "something" ; comment + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('section'), + 'PreservingIniWriter does not insert sections' + ); + $this->assertEquals( + 'value', + $newConfig->get('section')->get('key'), + 'PreservingIniWriter does not insert simple section properties' + ); + } - '; - $this->writeToTmp('section-order',$original); + /** + * @depends testWhetherSimpleSectionPropertiesAreInserted + */ + public function testWhetherSimpleSectionPropertiesAreUpdated() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[section] +key = "value" +EOD + ); + $config = new Zend_Config(array('section' => array('key' => 'eulav'))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals( + 'eulav', + $newConfig->get('section')->get('key'), + 'PreservingIniWriter does not update simple section properties' + ); + } + + /** + * @depends testWhetherSimpleSectionPropertiesAreInserted + */ + public function testWhetherSimpleSectionPropertiesAreDeleted() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[section] +key = "value" +EOD + ); + $config = new Zend_Config(array('section' => array())); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertNull( + $newConfig->get('section')->get('key'), + 'PreservingIniWriter does not delete simple section properties' + ); + } + + public function testWhetherNestedSectionPropertiesAreInserted() + { + $target = $this->writeConfigToTemporaryFile(''); + $config = new Zend_Config(array('section' => array('a' => array('b' => 'c')))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('section'), + 'PreservingIniWriter does not insert sections' + ); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('section')->get('a'), + 'PreservingIniWriter does not insert nested section properties' + ); + $this->assertEquals( + 'c', + $newConfig->get('section')->get('a')->get('b'), + 'PreservingIniWriter does not insert nested section properties' + ); + } + + /** + * @depends testWhetherNestedSectionPropertiesAreInserted + */ + public function testWhetherNestedSectionPropertiesAreUpdated() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[section] +a.b = "c" +EOD + ); + $config = new Zend_Config(array('section' => array('a' => array('b' => 'cc')))); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals( + 'cc', + $newConfig->get('section')->get('a')->get('b'), + 'PreservingIniWriter does not update nested section properties' + ); + } + + /** + * @depends testWhetherNestedSectionPropertiesAreInserted + */ + public function testWhetherNestedSectionPropertiesAreDeleted() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[section] +a.b = "c" +EOD + ); + $config = new Zend_Config(array('section' => array())); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertNull( + $newConfig->get('section')->get('a'), + 'PreservingIniWriter does not delete nested section properties' + ); + } + + public function testWhetherSimplePropertiesOfExtendingSectionsAreInserted() + { + $target = $this->writeConfigToTemporaryFile(''); $config = new Zend_Config( array( - 'section1' => array( - 'property' => 'something' - ), - 'section2' => array(), - 'section3' => array() + 'foo' => array('key1' => '1'), + 'bar' => array('key2' => '2') ) ); - $writer = new PreservingIniWriter( - array('config' => $config, 'filename' => $this->tmpfiles['section-order']) - ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); $writer->write(); - $changed = new \Zend_Config_Ini( - $this->tmpfiles['section-order'], - null, - array('allowModifications' => true) - ); - $this->assertEquals($config->section1->property, $changed->section1->property); - /* - * IniWriter should move the sections, so that comments - * are now in the right order - */ - $this->checkConfigComments( - $this->tmpfiles['section-order'], - 5, - 'Sections re-ordered correctly' + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('foo'), + 'PreservingIniWriter does not insert extended sections' + ); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('bar'), + 'PreservingIniWriter does not insert extending sections' + ); + $this->assertEquals( + '2', + $newConfig->get('bar')->get('key2'), + 'PreservingIniWriter does not insert simple properties into extending sections' + ); + $this->assertEquals( + '1', + $newConfig->get('bar')->get('key1'), + 'PreservingIniWriter does not properly define extending sections' ); } /** - * Change the test config, write the changes to the temporary - * file $tmpFile and save the path to the file in the array tmpfiles - * - * @param string $tmpFile The name that should be given to the temporary file + * @depends testWhetherSimplePropertiesOfExtendingSectionsAreInserted */ - private function changeConfigAndWriteToFile($tmpFile) + public function testWhetherSimplePropertiesOfExtendingSectionsAreUpdated() { - $config = $this->createTestConfig(); - $this->alterConfig($config); - $writer = new PreservingIniWriter( - array('config' => $config,'filename' => $this->tmpfiles[$tmpFile]) + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[foo] +key1 = "1" + +[bar : foo] +key2 = "2" +EOD ); + $config = new Zend_Config( + array( + 'foo' => array('key1' => '1'), + 'bar' => array('key2' => '22') + ) + ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); $writer->write(); - return $config; - } - /** - * Check if all comments are present - * - * @param String $file The file to check - * @param Number $count The amount of comments that should be present - * @param String $assertion The assertion message that will be displayed on errors - */ - private function checkConfigComments($file,$count = 10,$assertion = 'Comment unchanged') - { - $i = 0; - foreach (explode("\n",file_get_contents($file)) as $line) { - if (preg_match('/^;/',$line)) { - $i++; - $this->assertEquals( - $i,intval(substr($line,1)), - $assertion - ); - } - } - $this->assertEquals($count, $i, 'All comments exist'); - } - - /** - * Test if all configuration properties are set correctly - * - * @param mixed $config The configuration to check - */ - private function checkConfigProperties($config) - { - $this->assertEquals('val', $config->Trailing2, - 'Section-less property updated.'); - - $this->assertNull($config->trailing1, - 'Section-less property deleted.'); - - $this->assertEquals('value', $config->new, - 'Section-less property created.'); - - $this->assertEquals('0', $config->arr->{0}, - 'Value persisted in array'); - - $this->assertEquals('update', $config->arr->{2}, - 'Value changed in array'); - - $this->assertEquals('arrvalue', $config->arr->{4}, - 'Value added to array'); - - $this->assertEquals('', $config->parent->propOne, - 'Section property deleted.'); - - $this->assertEquals("2", $config->parent->propTwo, - 'Section property numerical unchanged.'); - - $this->assertEquals('update', $config->parent->propThree, - 'Section property updated.'); - - $this->assertEquals("true", $config->parent->propFour, - 'Section property boolean unchanged.'); - - $this->assertEquals("1", $config->parent->new, - 'Section property numerical created.'); - - $this->assertNull($config->parent->list->{0}, - 'Section array deleted'); - - $this->assertEquals('new', $config->parent->list->{1}, - 'Section array changed.'); - - $this->assertEquals('changed', $config->parent->many->many->nests, - 'Change strongly nested value.'); - - $this->assertEquals('new', $config->parent->many->many->new, - 'Ccreate strongy nested value.'); - - $this->assertEquals('overwritten', $config->child->PropOne, - 'Overridden inherited property unchanged.'); - - $this->assertEquals('somethingNew', $config->child->propTwo, - 'Inherited property changed.'); - - $this->assertEquals('test', $config->child->create, - 'Non-inherited property created.'); - - $this->assertInstanceOf('Zend_Config', $config->newsection, - 'New section created.'); - - $extends = $config->getExtends(); - $this->assertEquals('child', $extends['newsection'], - 'New inheritance created.'); - } - - /** - * Change the content of a Zend_Config for testing purposes - * - * @param Zend_Config $config The configuration that should be changed - */ - private function alterConfig(\Zend_Config $config) - { - $config->Trailing2 = 'val'; - unset($config->trailing1); - $config->new = 'value'; - $config->arr->{2} = "update"; - $config->arr->{4} = "arrvalue"; - - $config->section->property = "updated"; - unset($config->section->property3); - $config->section->property4 = "created"; - - $config->parent->propOne = null; - $config->parent->propThree = 'update'; - $config->parent->new = 1; - unset($config->parent->list->{0}); - $config->parent->list->{1} = 'new'; - - $config->parent->many->many->nests = "changed"; - $config->parent->many->many->new = "new"; - - $config->child->propTwo = 'somethingNew'; - $config->child->create = 'test'; - - $config->newsection = array(); - $config->newsection->p1 = "prop"; - $config->newsection->P2 = "prop"; - $config->setExtend('newsection', 'child'); - } - - /** - * Create the the configuration that will be used for the tests. - */ - private function createTestConfig() - { - return new \Zend_Config_Ini( - $this->tmpfiles['orig'], - null, - array('allowModifications' => true) + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals( + '22', + $newConfig->get('bar')->get('key2'), + 'PreservingIniWriter does not update simple properties of extending sections' ); } + + /** + * @depends testWhetherSimplePropertiesOfExtendingSectionsAreInserted + */ + public function testWhetherSimplePropertiesOfExtendingSectionsAreDeleted() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[foo] +key1 = "1" + +[bar : foo] +key2 = "2" +EOD + ); + $config = new Zend_Config( + array( + 'foo' => array('key1' => '1'), + 'bar' => array() + ) + ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertNull( + $newConfig->get('bar')->get('key2'), + 'PreservingIniWriter does not delete simple properties of extending sections' + ); + } + + public function testWhetherNestedPropertiesOfExtendingSectionsAreInserted() + { + $target = $this->writeConfigToTemporaryFile(''); + $config = new Zend_Config( + array( + 'foo' => array('a' => array('b' => 'c')), + 'bar' => array('d' => array('e' => 'f')) + ) + ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('foo'), + 'PreservingIniWriter does not insert extended sections' + ); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('bar'), + 'PreservingIniWriter does not insert extending sections' + ); + $this->assertInstanceOf( + '\Zend_Config', + $newConfig->get('bar')->get('d'), + 'PreservingIniWriter does not insert nested properties into extending sections' + ); + $this->assertEquals( + 'f', + $newConfig->get('bar')->get('d')->get('e'), + 'PreservingIniWriter does not insert nested properties into extending sections' + ); + $this->assertEquals( + 'c', + $newConfig->get('bar')->get('a')->get('b'), + 'PreservingIniWriter does not properly define extending sections with nested properties' + ); + } + + /** + * @depends testWhetherNestedPropertiesOfExtendingSectionsAreInserted + */ + public function testWhetherNestedPropertiesOfExtendingSectionsAreUpdated() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[foo] +a.b = "c" + +[bar : foo] +d.e = "f" +EOD + ); + $config = new Zend_Config( + array( + 'foo' => array('a' => array('b' => 'c')), + 'bar' => array('d' => array('e' => 'ff')) + ) + ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertEquals( + 'ff', + $newConfig->get('bar')->get('d')->get('e'), + 'PreservingIniWriter does not update nested properties of extending sections' + ); + } + + /** + * @depends testWhetherNestedPropertiesOfExtendingSectionsAreInserted + */ + public function testWhetherNestedPropertiesOfExtendingSectionsAreDeleted() + { + $target = $this->writeConfigToTemporaryFile(<<<'EOD' +[foo] +a.b = "c" + +[bar : foo] +d.e = "f" +EOD + ); + $config = new Zend_Config( + array( + 'foo' => array('a' => array('b' => 'c')), + 'bar' => array() + ) + ); + $config->setExtend('bar', 'foo'); + $writer = new PreservingIniWriter(array('config' => $config, 'filename' => $target)); + $writer->write(); + + $newConfig = new Zend_Config_Ini($target); + $this->assertNull( + $newConfig->get('bar')->get('d'), + 'PreservingIniWriter does not delete nested properties of extending sections' + ); + } + + public function testWhetherSectionAndPropertyOrderIsPreserved() + { + $config = <<<'EOD' +[one] +key1 = "1" +key2 = "2" + + +[two] +a.b = "c" +d.e = "f" + + +[three] +key = "value" +foo.bar = "raboof" +EOD; + $target = $this->writeConfigToTemporaryFile($config); + $writer = new PreservingIniWriter( + array( + 'config' => new Zend_Config( + array( + 'three' => array( + 'foo' => array( + 'bar' => 'raboof' + ), + 'key' => 'value' + ), + 'two' => array( + 'd' => array( + 'e' => 'f' + ), + 'a' => array( + 'b' => 'c' + ) + ), + 'one' => array( + 'key2' => '2', + 'key1' => '1' + ) + ) + ), + 'filename' => $target + ) + ); + + $this->assertEquals( + $config, + $writer->render(), + 'PreservingIniWriter does not preserve section and/or property order' + ); + } + + public function testWhetherCommentsOnEmptyLinesArePreserved() + { + $config = <<<'EOD' +; some interesting comment +key = "value" +; another interesting comment +; boring comment +EOD; + $target = $this->writeConfigToTemporaryFile($config); + $writer = new PreservingIniWriter( + array('config' => new Zend_Config(array('key' => 'value')), 'filename' => $target) + ); + + $this->assertEquals( + $config, + $writer->render(), + 'PreservingIniWriter does not preserve comments on empty lines' + ); + } + + public function testWhetherCommentsOnPropertyLinesArePreserved() + { + $config = <<<'EOD' +foo = 1337 ; I know what a " and a ' is +bar = 7331 ; I; tend; to; overact; !1!1!!11!111! ; +key = "value" ; some comment for a small sized property +xxl = "very loooooooooooooooooooooong" ; my value is very lo... +EOD; + $target = $this->writeConfigToTemporaryFile($config); + $writer = new PreservingIniWriter( + array( + 'config' => new Zend_Config( + array( + 'foo' => 1337, + 'bar' => 7331, + 'key' => 'value', + 'xxl' => 'very loooooooooooooooooooooong' + ) + ), + 'filename' => $target + ) + ); + + $this->assertEquals( + $config, + $writer->render(), + 'PreservingIniWriter does not preserve comments on property lines' + ); + } + + public function testWhetherCommentsOnEmptySectionLinesArePreserved() + { + $config = <<<'EOD' +[section] +; some interesting comment, in a section +key = "value" +EOD; + $target = $this->writeConfigToTemporaryFile($config); + $writer = new PreservingIniWriter( + array('config' => new Zend_Config(array('section' => array('key' => 'value'))), 'filename' => $target) + ); + + $this->assertEquals( + $config, + $writer->render(), + 'PreservingIniWriter does not preserve comments on empty section lines' + ); + } + + public function testWhetherCommentsOnSectionPropertyLinesArePreserved() + { + $config = <<<'EOD' +[section] +foo = 1337 ; I know what a " and a ' is +bar = 7331 ; I; tend; to; overact; !1!1!!11!111! ; +key = "value" ; some comment for a small sized property +xxl = "very loooooooooooooooooooooong" ; my value is very lo... +EOD; + $target = $this->writeConfigToTemporaryFile($config); + $writer = new PreservingIniWriter( + array( + 'config' => new Zend_Config( + array( + 'section' => array( + 'foo' => 1337, + 'bar' => 7331, + 'key' => 'value', + 'xxl' => 'very loooooooooooooooooooooong' + ) + ) + ), + 'filename' => $target + ) + ); + + $this->assertEquals( + $config, + $writer->render(), + 'PreservingIniWriter does not preserve comments on property lines' + ); + } + + /** + * Write a INI-configuration string to a temporary file and return it's path + * + * @param string $config The config string to write + * + * @return string The path to the temporary file + */ + protected function writeConfigToTemporaryFile($config) + { + file_put_contents($this->tempFile, $config); + return $this->tempFile; + } } From 87863d32122e6c7a6ccbe4e6596ce2ac888dc49d Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 23 Apr 2014 14:40:39 +0200 Subject: [PATCH 21/29] Add test for Icinga\File\Csv refs #6011 --- library/Icinga/File/Csv.php | 4 +++ test/php/library/Icinga/File/CsvTest.php | 39 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 test/php/library/Icinga/File/CsvTest.php diff --git a/library/Icinga/File/Csv.php b/library/Icinga/File/Csv.php index 7fb4b25ee..4873fcf97 100644 --- a/library/Icinga/File/Csv.php +++ b/library/Icinga/File/Csv.php @@ -1,4 +1,6 @@ array( + array('col1' => 'val1', 'col2' => 'val2', 'col3' => 'val3', 'col4' => 'val4'), + array('col1' => 'val5', 'col2' => 'val6', 'col3' => 'val7', 'col4' => 'val8') + ) + ) + ); + $csv = Csv::fromQuery($queryMock); + + $this->assertEquals( + join( + "\r\n", + array( + 'col1,col2,col3,col4', + '"val1","val2","val3","val4"', + '"val5","val6","val7","val8"' + ) + ) . "\r\n", + (string) $csv, + 'Csv does not render valid/correct csv structured data' + ); + } +} From f20d459000ad359559fbe491c46e0cffc962700f Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 23 Apr 2014 15:44:26 +0200 Subject: [PATCH 22/29] Rename test/php/library/Icinga/Application/Config to ConfigTest refs 6011 --- test/php/library/Icinga/Application/ConfigTest.php | 2 +- .../Icinga/Application/{Config => ConfigTest}/files/config.ini | 0 .../Icinga/Application/{Config => ConfigTest}/files/extra.ini | 0 .../{Config => ConfigTest}/files/modules/amodule/config.ini | 0 .../{Config => ConfigTest}/files/modules/amodule/extra.ini | 0 5 files changed, 1 insertion(+), 1 deletion(-) rename test/php/library/Icinga/Application/{Config => ConfigTest}/files/config.ini (100%) rename test/php/library/Icinga/Application/{Config => ConfigTest}/files/extra.ini (100%) rename test/php/library/Icinga/Application/{Config => ConfigTest}/files/modules/amodule/config.ini (100%) rename test/php/library/Icinga/Application/{Config => ConfigTest}/files/modules/amodule/extra.ini (100%) diff --git a/test/php/library/Icinga/Application/ConfigTest.php b/test/php/library/Icinga/Application/ConfigTest.php index 3724c9566..d5dd97e56 100644 --- a/test/php/library/Icinga/Application/ConfigTest.php +++ b/test/php/library/Icinga/Application/ConfigTest.php @@ -16,7 +16,7 @@ class ConfigTest extends BaseTestCase { parent::setUp(); $this->configDir = IcingaConfig::$configDir; - IcingaConfig::$configDir = dirname(__FILE__) . '/Config/files'; + IcingaConfig::$configDir = dirname(__FILE__) . '/ConfigTest/files'; } /** diff --git a/test/php/library/Icinga/Application/Config/files/config.ini b/test/php/library/Icinga/Application/ConfigTest/files/config.ini similarity index 100% rename from test/php/library/Icinga/Application/Config/files/config.ini rename to test/php/library/Icinga/Application/ConfigTest/files/config.ini diff --git a/test/php/library/Icinga/Application/Config/files/extra.ini b/test/php/library/Icinga/Application/ConfigTest/files/extra.ini similarity index 100% rename from test/php/library/Icinga/Application/Config/files/extra.ini rename to test/php/library/Icinga/Application/ConfigTest/files/extra.ini diff --git a/test/php/library/Icinga/Application/Config/files/modules/amodule/config.ini b/test/php/library/Icinga/Application/ConfigTest/files/modules/amodule/config.ini similarity index 100% rename from test/php/library/Icinga/Application/Config/files/modules/amodule/config.ini rename to test/php/library/Icinga/Application/ConfigTest/files/modules/amodule/config.ini diff --git a/test/php/library/Icinga/Application/Config/files/modules/amodule/extra.ini b/test/php/library/Icinga/Application/ConfigTest/files/modules/amodule/extra.ini similarity index 100% rename from test/php/library/Icinga/Application/Config/files/modules/amodule/extra.ini rename to test/php/library/Icinga/Application/ConfigTest/files/modules/amodule/extra.ini From 2b15d35decde9c97da6e6c46ac53dec74c9cc33e Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 24 Apr 2014 10:13:47 +0200 Subject: [PATCH 23/29] Rewrite test for Icinga\Web\Form refs #6011 --- library/Icinga/Web/Form.php | 259 ++++++++++------------- test/php/library/Icinga/Web/FormTest.php | 120 ++++------- 2 files changed, 147 insertions(+), 232 deletions(-) diff --git a/library/Icinga/Web/Form.php b/library/Icinga/Web/Form.php index 999dda8de..7cd3ad746 100644 --- a/library/Icinga/Web/Form.php +++ b/library/Icinga/Web/Form.php @@ -29,18 +29,18 @@ namespace Icinga\Web; -use \Zend_Controller_Request_Abstract; -use \Zend_Form; -use \Zend_Config; -use \Zend_Form_Element_Submit; -use \Zend_Form_Element_Reset; -use \Zend_View_Interface; -use \Icinga\Web\Form\Element\Note; -use \Icinga\Exception\ProgrammingError; -use \Icinga\Web\Form\Decorator\HelpText; -use \Icinga\Web\Form\Decorator\BootstrapForm; -use \Icinga\Web\Form\InvalidCSRFTokenException; -use \Icinga\Application\Config as IcingaConfig; +use Zend_Controller_Request_Abstract; +use Zend_Form; +use Zend_Config; +use Zend_Form_Element_Submit; +use Zend_Form_Element_Reset; +use Zend_View_Interface; +use Icinga\Web\Form\Element\Note; +use Icinga\Exception\ProgrammingError; +use Icinga\Web\Form\Decorator\HelpText; +use Icinga\Web\Form\Decorator\BootstrapForm; +use Icinga\Web\Form\InvalidCSRFTokenException; +use Icinga\Application\Config as IcingaConfig; /** * Base class for forms providing CSRF protection, confirmation logic and auto submission @@ -52,7 +52,7 @@ class Form extends Zend_Form * * @var Zend_Controller_Request_Abstract */ - private $request; + protected $request; /** * Main configuration @@ -61,14 +61,14 @@ class Form extends Zend_Form * * @var IcingaConfig */ - private $config; + protected $config; /** * The preference object to use instead of the one from the user (used for testing) * * @var Zend_Config */ - private $preferences; + protected $preferences; /** * Whether this form should NOT add random generated "challenge" tokens that are associated with the user's current @@ -84,21 +84,21 @@ class Form extends Zend_Form * * @var string */ - private $tokenElementName = 'CSRFToken'; + protected $tokenElementName = 'CSRFToken'; /** * Flag to indicate that form is already build * * @var bool */ - private $created = false; + protected $created = false; /** * Session id used for CSRF token generation * * @var string */ - private $sessionId; + protected $sessionId; /** * Label for submit button @@ -107,7 +107,7 @@ class Form extends Zend_Form * * @var string */ - private $submitLabel; + protected $submitLabel; /** * Label for cancel button @@ -116,7 +116,7 @@ class Form extends Zend_Form * * @var string */ - private $cancelLabel; + protected $cancelLabel; /** * Last used note-id @@ -125,21 +125,7 @@ class Form extends Zend_Form * * @var int */ - private $last_note_id = 0; - - /** - * Decorator that replaces the DtDd Zend-Form default - * - * @var Form\Decorator\BootstrapFormDecorator - */ - private $formDecorator; - - /** - * Whether to ignore users leaving the form with unsaved changes - * - * @var bool - */ - private $ignoreChangeDiscarding = false; + protected $last_note_id = 0; /** * Getter for the session ID @@ -147,26 +133,14 @@ class Form extends Zend_Form * If the ID has never been set, the ID from session_id() is returned * * @return string - * - * @see session_id() - * @see setSessionId() */ public function getSessionId() { if (!$this->sessionId) { $this->sessionId = session_id(); } - return $this->sessionId; - } - /** - * Set whether to inform a user when he is about to discard changes (false, default) or not - * - * @param boolean $bool False to not inform users when they leave modified forms, otherwise true - */ - public function setIgnoreChangeDiscarding($bool) - { - $this->ignoreChangeDiscarding = (boolean) $bool; + return $this->sessionId; } /** @@ -174,7 +148,7 @@ class Form extends Zend_Form * * This method should be used for testing purposes only * - * @param string $sessionId + * @param string $sessionId */ public function setSessionId($sessionId) { @@ -184,7 +158,7 @@ class Form extends Zend_Form /** * Return the HTML element name of the CSRF token field * - * @return string + * @return string */ public function getTokenElementName() { @@ -194,7 +168,7 @@ class Form extends Zend_Form /** * Render the form to HTML * - * @param Zend_View_Interface $view + * @param Zend_View_Interface $view * * @return string */ @@ -210,6 +184,7 @@ class Form extends Zend_Form */ protected function create() { + } /** @@ -217,12 +192,13 @@ class Form extends Zend_Form */ protected function preValidation(array $data) { + } /** * Setter for the request * - * @param Zend_Controller_Request_Abstract $request + * @param Zend_Controller_Request_Abstract $request */ public function setRequest(Zend_Controller_Request_Abstract $request) { @@ -232,7 +208,7 @@ class Form extends Zend_Form /** * Getter for the request * - * @return Zend_Controller_Request_Abstract + * @return Zend_Controller_Request_Abstract */ public function getRequest() { @@ -242,7 +218,7 @@ class Form extends Zend_Form /** * Set the configuration to be used for this form when no preferences are set yet * - * @param IcingaConfig $cfg + * @param IcingaConfig $cfg * * @return self */ @@ -257,20 +233,21 @@ class Form extends Zend_Form * * Returns the set configuration or an empty default one. * - * @return Zend_Config + * @return Zend_Config */ public function getConfiguration() { if ($this->config === null) { $this->config = new Zend_Config(array(), true); } + return $this->config; } /** * Set preferences to be used instead of the one from the user object (used for testing) * - * @param Zend_Config $prefs + * @param Zend_Config $prefs */ public function setUserPreferences($prefs) { @@ -280,13 +257,14 @@ class Form extends Zend_Form /** * Return the preferences of the user or the overwritten ones * - * @return Zend_Config + * @return Zend_Config */ public function getUserPreferences() { if ($this->preferences) { return $this->preferences; } + return $this->getRequest()->getUser()->getPreferences(); } @@ -297,7 +275,6 @@ class Form extends Zend_Form */ public function buildForm() { - if ($this->created === false) { $this->initCsrfToken(); $this->create(); @@ -314,18 +291,15 @@ class Form extends Zend_Form if (!$this->getAction() && $this->getRequest()) { $this->setAction($this->getRequest()->getRequestUri()); } - $this->addElementDecorators(); + $this->created = true; - if (!$this->ignoreChangeDiscarding) { - //$this->setAttrib('data-icinga-component', 'app/form'); - } } } /** * Setter for the cancel label * - * @param string $cancelLabel + * @param string $cancelLabel */ public function setCancelLabel($cancelLabel) { @@ -335,22 +309,23 @@ class Form extends Zend_Form /** * Add cancel button to form */ - private function addCancelButton() + protected function addCancelButton() { - $cancelLabel = new Zend_Form_Element_Reset( - array( - 'name' => 'btn_reset', - 'label' => $this->cancelLabel, - 'class' => 'btn pull-right' + $this->addElement( + new Zend_Form_Element_Reset( + array( + 'name' => 'btn_reset', + 'label' => $this->cancelLabel, + 'class' => 'btn pull-right' + ) ) ); - $this->addElement($cancelLabel); } /** * Setter for the submit label * - * @param string $submitLabel + * @param string $submitLabel */ public function setSubmitLabel($submitLabel) { @@ -360,22 +335,23 @@ class Form extends Zend_Form /** * Add submit button to form */ - private function addSubmitButton() + protected function addSubmitButton() { - $submitButton = new Zend_Form_Element_Submit( - array( - 'name' => 'btn_submit', - 'label' => $this->submitLabel, + $this->addElement( + new Zend_Form_Element_Submit( + array( + 'name' => 'btn_submit', + 'label' => $this->submitLabel + ) ) ); - $this->addElement($submitButton); } /** * Add message to form * - * @param string $message The message to be displayed - * @param int $headingType Whether it should be displayed as heading (1-6) or not (null) + * @param string $message The message to be displayed + * @param int $headingType Whether it should be displayed as heading (1-6) or not (null) */ public function addNote($message, $headingType = null) { @@ -399,17 +375,16 @@ class Form extends Zend_Form * * Enables automatic submission of this form once the user edits specific elements * - * @param array $triggerElements The element names which should auto-submit the form + * @param array $triggerElements The element names which should auto-submit the form * - * @throws ProgrammingError When an element is found which does not yet exist + * @throws ProgrammingError When an element is found which does not yet exist */ - final public function enableAutoSubmit($triggerElements) + public function enableAutoSubmit($triggerElements) { foreach ($triggerElements as $elementName) { $element = $this->getElement($elementName); if ($element !== null) { $element->setAttrib('onchange', '$(this.form).submit();'); - $element->setAttrib('data-icinga-form-autosubmit', true); } else { throw new ProgrammingError( 'You need to add the element "' . $elementName . '" to' . @@ -425,9 +400,7 @@ class Form extends Zend_Form * Ensures that the current request method is POST, that the form was manually submitted and that the data provided * in the request is valid and gets repopulated in case its invalid. * - * @return bool True when the form is submitted and valid, otherwise false - * @see Form::isValid() - * @see Form::isSubmitted() + * @return bool True when the form is submitted and valid, otherwise false */ public function isSubmittedAndValid() { @@ -457,7 +430,7 @@ class Form extends Zend_Form * Per default, this checks whether the button set with the 'setSubmitLabel' method * is being submitted. For custom submission logic, this method must be overwritten * - * @return bool True when the form is marked as submitted, otherwise false + * @return bool True when the form is marked as submitted, otherwise false */ public function isSubmitted() { @@ -466,6 +439,7 @@ class Form extends Zend_Form $checkData = $this->getRequest()->getParams(); $submitted = isset($checkData['btn_submit']); } + return $submitted; } @@ -474,13 +448,13 @@ class Form extends Zend_Form * * This method should be used for testing purposes only * - * @param bool $disabled Set true in order to disable CSRF tokens in this form (default: true), otherwise false - * - * @see tokenDisabled + * @param bool $disabled Set true in order to disable CSRF tokens in + * this form (default: true), otherwise false */ - final public function setTokenDisabled($disabled = true) + public function setTokenDisabled($disabled = true) { $this->tokenDisabled = (boolean) $disabled; + if ($disabled === true) { $this->removeElement($this->tokenElementName); } @@ -489,54 +463,47 @@ class Form extends Zend_Form /** * Add CSRF counter measure field to form */ - final public function initCsrfToken() + public function initCsrfToken() { - if ($this->tokenDisabled || $this->getElement($this->tokenElementName)) { - return; + if (!$this->tokenDisabled && $this->getElement($this->tokenElementName) === null) { + $this->addElement( + 'hidden', + $this->tokenElementName, + array( + 'value' => $this->generateCsrfTokenAsString() + ) + ); } - $this->addElement( - 'hidden', - $this->tokenElementName, - array( - 'value' => $this->generateCsrfTokenAsString(), - 'decorators' => array('ViewHelper') - ) - ); } /** * Test the submitted data for a correct CSRF token * - * @param array $checkData The POST data send by the user + * @param array $checkData The POST data send by the user * * @throws InvalidCSRFTokenException When CSRF Validation fails */ - final public function assertValidCsrfToken(array $checkData) + public function assertValidCsrfToken(array $checkData) { - if ($this->tokenDisabled) { - return; - } - - if (!isset($checkData[$this->tokenElementName]) - || !$this->hasValidCsrfToken($checkData[$this->tokenElementName]) - ) { - throw new InvalidCSRFTokenException(); + if (!$this->tokenDisabled) { + if (!isset($checkData[$this->tokenElementName]) + || !$this->hasValidCsrfToken($checkData[$this->tokenElementName]) + ) { + throw new InvalidCSRFTokenException(); + } } } /** * Check whether the form's CSRF token-field has a valid value * - * @param string $elementValue Value from the form element + * @param string $elementValue Value from the form element * * @return bool */ - private function hasValidCsrfToken($elementValue) + protected function hasValidCsrfToken($elementValue) { - if ($this->getElement($this->tokenElementName) === null) { - return false; - } - if (strpos($elementValue, '|') === false) { + if ($this->getElement($this->tokenElementName) === null || strpos($elementValue, '|') === false) { return false; } @@ -549,26 +516,12 @@ class Form extends Zend_Form return $token === hash('sha256', $this->getSessionId() . $seed); } - /** - * Add element decorators which apply to all elements - * - * Adds `HelpText` decorator - * - * @see HelpText - */ - private function addElementDecorators() - { - foreach ($this->getElements() as $element) { - $element->addDecorator(new HelpText()); - } - } - /** * Generate a new (seed, token) pair * - * @return array + * @return array */ - final public function generateCsrfToken() + public function generateCsrfToken() { $seed = mt_rand(); $hash = hash('sha256', $this->getSessionId() . $seed); @@ -579,9 +532,9 @@ class Form extends Zend_Form /** * Return the string representation of the CSRF seed/token pair * - * @return string + * @return string */ - final public function generateCsrfTokenAsString() + public function generateCsrfTokenAsString() { list ($seed, $token) = $this->generateCsrfToken($this->getSessionId()); return sprintf('%s|%s', $seed, $token); @@ -593,31 +546,29 @@ class Form extends Zend_Form * Additionally, all DtDd tags will be removed and the Bootstrap compatible * BootstrapForm decorator will be added to the elements * - * * @param string|Zend_Form_Element $element String element type, or an object of type Zend_Form_Element * @param string $name The name of the element to add if $element is a string * @param array $options The settings for the element if $element is a string * - * @return Form + * @return self * @see Zend_Form::addElement() */ public function addElement($element, $name = null, $options = null) { parent::addElement($element, $name, $options); - $el = $name ? $this->getElement($name) : $element; - - // Do not add structural elements to invisible elements - // which produces ugly views - if (strpos(strtolower(get_class($el)), 'hidden') !== false) { - $el->setDecorators(array('ViewHelper')); - return $this; - } + $el = $name !== null ? $this->getElement($name) : $element; if ($el) { - $el->removeDecorator('HtmlTag'); - $el->removeDecorator('Label'); - $el->removeDecorator('DtDdWrapper'); - $el->addDecorator(new BootstrapForm()); + if (strpos(strtolower(get_class($el)), 'hidden') !== false) { + // Do not add structural elements to invisible elements which produces ugly views + $el->setDecorators(array('ViewHelper')); + } else { + $el->removeDecorator('HtmlTag'); + $el->removeDecorator('Label'); + $el->removeDecorator('DtDdWrapper'); + $el->addDecorator(new BootstrapForm()); + $el->addDecorator(new HelpText()); + } } return $this; @@ -626,7 +577,9 @@ class Form extends Zend_Form /** * Load the default decorators * - * @return Zend_Form + * Overwrites Zend_Form::loadDefaultDecorators to avoid having the HtmlTag-Decorator added + * + * @return self */ public function loadDefaultDecorators() { @@ -637,8 +590,10 @@ class Form extends Zend_Form $decorators = $this->getDecorators(); if (empty($decorators)) { $this->addDecorator('FormElements') - ->addDecorator('Form'); + //->addDecorator('HtmlTag', array('tag' => 'dl', 'class' => 'zend_form')) + ->addDecorator('Form'); } + return $this; } } diff --git a/test/php/library/Icinga/Web/FormTest.php b/test/php/library/Icinga/Web/FormTest.php index d73dd0ef3..4e6ec5d92 100644 --- a/test/php/library/Icinga/Web/FormTest.php +++ b/test/php/library/Icinga/Web/FormTest.php @@ -7,95 +7,55 @@ namespace Tests\Icinga\Web; use Icinga\Web\Form; use Icinga\Test\BaseTestCase; -/** - * Dummy extension class as Icinga\Web\Form is an abstract one - */ -class TestForm extends Form -{ - public function create() - { - } -} - -/** - * Tests for the Icinga\Web\Form class (Base class for all other forms) - */ class FormTest extends BaseTestCase { - /** - * Tests whether the cancel label will be added to the form - */ - function testCancelLabel() + public function testWhetherAddElementDoesNotAddSpecificDecorators() { - $form = new TestForm(); - $form->setCancelLabel('Cancel'); - $form->buildForm(); - $this->assertCount(2, $form->getElements(), 'Asserting that the cancel label is present'); + $form = new Form(); + $form->addElement('text', 'someText'); + $element = $form->getElement('someText'); + + $this->assertFalse( + $element->getDecorator('HtmlTag'), + 'Form::addElement does not remove the HtmlTag-Decorator' + ); + $this->assertFalse( + $element->getDecorator('Label'), + 'Form::addElement does not remove the Label-Decorator' + ); + $this->assertFalse( + $element->getDecorator('DtDdWrapper'), + 'Form::addElement does not remove the DtDdWrapper-Decorator' + ); } - /** - * Tests whether the submit button will be added to the form - */ - function testSubmitButton() + public function testWhetherAddElementDoesNotAddAnyOptionalDecoratorsToHiddenElements() { - $form = new TestForm(); - $form->setSubmitLabel('Submit'); - $form->buildForm(); - $this->assertCount(2, $form->getElements(), 'Asserting that the submit button is present'); + $form = new Form(); + $form->addElement('hidden', 'somethingHidden'); + $element = $form->getElement('somethingHidden'); + + $this->assertCount( + 1, + $element->getDecorators(), + 'Form::addElement adds more decorators than necessary to hidden elements' + ); + $this->assertInstanceOf( + '\Zend_Form_Decorator_ViewHelper', + $element->getDecorator('ViewHelper'), + 'Form::addElement does not add the ViewHelper-Decorator to hidden elements' + ); } - /** - * Tests whether automatic form submission will be enabled for a single field - */ - function testEnableAutoSubmitSingle() + public function testWhetherLoadDefaultDecoratorsDoesNotAddTheHtmlTagDecorator() { - $form = new TestForm(); - $form->addElement('checkbox', 'example1', array()); - $form->enableAutoSubmit(array('example1')); - $this->assertArrayHasKey('data-icinga-form-autosubmit', $form->getElement('example1')->getAttribs(), - 'Asserting that auto-submit got enabled for one element'); - } + $form = new Form(); + $form->loadDefaultDecorators(); - /** - * Tests whether automatic form submission will be enabled for multiple fields - */ - function testEnableAutoSubmitMultiple() - { - $form = new TestForm(); - $form->addElement('checkbox', 'example1', array()); - $form->addElement('checkbox', 'example2', array()); - $form->enableAutoSubmit(array('example1', 'example2')); - $this->assertArrayHasKey('data-icinga-form-autosubmit', $form->getElement('example1')->getAttribs(), - 'Asserting that auto-submit got enabled for multiple elements'); - $this->assertArrayHasKey('data-icinga-form-autosubmit', $form->getElement('example2')->getAttribs(), - 'Asserting that auto-submit got enabled for multiple elements'); - } - - /** - * Tests whether automatic form submission can only be enabled for existing elements - * - * @expectedException Icinga\Exception\ProgrammingError - */ - function testEnableAutoSubmitExisting() - { - $form = new TestForm(); - $form->enableAutoSubmit(array('not_existing')); - } - - /** - * Tests whether a form will be detected as properly submitted - */ - function testFormSubmission() - { - $form = new TestForm(); - $form->setTokenDisabled(); - $form->setSubmitLabel('foo'); - $request = $this->getRequest(); - $form->setRequest($request->setMethod('GET')); - $this->assertFalse($form->isSubmittedAndValid(), - 'Asserting that it is not possible to submit a form not using POST'); - $request->setMethod('POST')->setPost(array('btn_submit' => 'foo')); - $this->assertTrue($form->isSubmittedAndValid(), - 'Asserting that it is possible to detect a form as submitted'); + $this->assertArrayNotHasKey( + 'HtmlTag', + $form->getDecorators(), + 'Form::loadDefaultDecorators adds the HtmlTag-Decorator' + ); } } From 290fe9eeb5a9a2b4355a222d355f96e533e8f8df Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 24 Apr 2014 14:40:49 +0200 Subject: [PATCH 24/29] Rewrite test for Icinga\Web\Hook refs #6011 --- library/Icinga/Web/Hook.php | 117 +++----- .../Configuration/ConfigurationTabBuilder.php | 2 +- library/Icinga/Web/Widget/FilterBox.php | 1 - .../controllers/ListController.php | 2 +- .../application/views/helpers/CommandForm.php | 1 - .../library/Monitoring/Environment.php | 4 +- test/php/library/Icinga/Web/HookTest.php | 250 +++++++++++------- 7 files changed, 205 insertions(+), 172 deletions(-) diff --git a/library/Icinga/Web/Hook.php b/library/Icinga/Web/Hook.php index 915597f86..6f3ffc2f0 100644 --- a/library/Icinga/Web/Hook.php +++ b/library/Icinga/Web/Hook.php @@ -29,9 +29,9 @@ namespace Icinga\Web; +use Exception; use Icinga\Logger\Logger; use Icinga\Exception\ProgrammingError; -use stdClass; /** * Icinga Web Hook registry @@ -79,9 +79,9 @@ class Hook /** * Whether someone registered itself for the given hook name * - * @param string $name One of the predefined hook names + * @param string $name One of the predefined hook names * - * @return bool + * @return bool */ public static function has($name) { @@ -89,72 +89,29 @@ class Hook } /** - * Get the first registered instance for the given hook name + * Create or return an instance of a given hook * - * TODO: Multiple instances are not handled yet * TODO: Should return some kind of a hook interface * - * @param string $name One of the predefined hook names - * - * @return mixed - * @throws ProgrammingError - */ - public static function get($name) - { - if (! self::has($name)) { - return null; - } - if (! array_key_exists($name, self::$instances)) { - $class = self::$hooks[$name]; - try { - $obj = new $class(); - } catch (\Exception $e) { - // TODO: Persist unloading for "some time" or "current session" - Logger::debug( - 'Hook "%s" (%s) failed, will be unloaded: %s', - $name, - $class, - $e->getMessage() - ); - unset(self::$hooks[$name]); - return null; - } - $base_class = 'Icinga\\Web\\Hook\\' . ucfirst($name); - if (! $obj instanceof $base_class) { - throw new ProgrammingError( - sprintf( - '%s is not an instance of %s', - get_class($obj), - $base_class - ) - ); - } - self::$instances[$name] = $obj; - - } - return self::$instances[$name]; - } - - /** - * Create or return an instance of the hook - * - * @param string $name - * @param string $key - * @return stdClass + * @param string $name One of the predefined hook names + * @param string $key The identifier of a specific subtype * + * @return mixed */ public static function createInstance($name, $key) { if (!self::has($name, $key)) { return null; } + if (isset(self::$instances[$name][$key])) { return self::$instances[$name][$key]; } + $class = self::$hooks[$name][$key]; try { $instance = new $class(); - } catch (\Exception $e) { + } catch (Exception $e) { Logger::debug( 'Hook "%s" (%s) (%s) failed, will be unloaded: %s', $name, @@ -162,9 +119,11 @@ class Hook $class, $e->getMessage() ); + // TODO: Persist unloading for "some time" or "current session" unset(self::$hooks[$name][$key]); return null; } + self::assertValidHook($instance, $name); self::$instances[$name][$key] = $instance; return $instance; @@ -173,11 +132,12 @@ class Hook /** * Test for a valid class name * - * @param stdClass $instance - * @param string $name - * @throws ProgrammingError + * @param mixed $instance + * @param string $name + * + * @throws ProgrammingError */ - private static function assertValidHook(&$instance, $name) + private static function assertValidHook($instance, $name) { $base_class = self::$BASE_NS . ucfirst($name); if (!$instance instanceof $base_class) { @@ -194,43 +154,45 @@ class Hook /** * Return all instances of a specific name * - * @param string $name + * @param string $name One of the predefined hook names * - * @return array + * @return array */ public static function all($name) { if (!self::has($name)) { return array(); } + foreach (self::$hooks[$name] as $key => $hook) { if (self::createInstance($name, $key) === null) { return array(); } } + return self::$instances[$name]; } /** * Get the first hook * - * @param string $name + * @param string $name One of the predefined hook names * - * @return stdClass + * @return null|mixed */ public static function first($name) { - return self::createInstance($name, key(self::$hooks[$name])); + if (self::has($name)) { + return self::createInstance($name, key(self::$hooks[$name])); + } } /** * Register your hook * - * @param string $name One of the predefined hook names - * @param string $key - * @param string $class Your class name, must inherit one of the classes - * in the Icinga/Web/Hook folder + * Alias for Hook::registerClass() * + * @see Hook::registerClass() */ public static function register($name, $key, $class) { @@ -240,12 +202,17 @@ class Hook /** * Register a class * - * @param string $name - * @param string $key - * @param string $class + * @param string $name One of the predefined hook names + * @param string $key The identifier of a specific subtype + * @param string $class Your class name, must inherit one of the + * classes in the Icinga/Web/Hook folder */ public static function registerClass($name, $key, $class) { + if (!class_exists($class)) { + throw new ProgrammingError('"' . $class . '" is not an existing class'); + } + if (!isset(self::$hooks[$name])) { self::$hooks[$name] = array(); } @@ -256,23 +223,23 @@ class Hook /** * Register an object * - * @param string $name - * @param string $key - * @param object $object + * @param string $name One of the predefined hook names + * @param string $key The identifier of a specific subtype + * @param object $object The instantiated hook to register * - * @throws ProgrammingError + * @throws ProgrammingError */ public static function registerObject($name, $key, $object) { if (!is_object($object)) { - throw new ProgrammingError('object is not an instantiated class'); + throw new ProgrammingError('"' . $object . '" is not an instantiated class'); } if (!isset(self::$instances[$name])) { self::$instances[$name] = array(); } - self::$instances[$name][$key] =& $object; + self::$instances[$name][$key] = $object; self::registerClass($name, $key, get_class($object)); } } diff --git a/library/Icinga/Web/Hook/Configuration/ConfigurationTabBuilder.php b/library/Icinga/Web/Hook/Configuration/ConfigurationTabBuilder.php index ae772f68e..dad7a37f0 100644 --- a/library/Icinga/Web/Hook/Configuration/ConfigurationTabBuilder.php +++ b/library/Icinga/Web/Hook/Configuration/ConfigurationTabBuilder.php @@ -90,7 +90,7 @@ class ConfigurationTabBuilder { /** @var ConfigurationTab $configTab */ $configTab = null; - foreach (Hook::get(self::HOOK_NAMESPACE) as $configTab) { + foreach (Hook::all(self::HOOK_NAMESPACE) as $configTab) { if (!$configTab instanceof ConfigurationTabInterface) { throw new ProgrammingError('tab not instance of ConfigTabInterface'); } diff --git a/library/Icinga/Web/Widget/FilterBox.php b/library/Icinga/Web/Widget/FilterBox.php index 3059197d1..4395129ff 100644 --- a/library/Icinga/Web/Widget/FilterBox.php +++ b/library/Icinga/Web/Widget/FilterBox.php @@ -110,7 +110,6 @@ EOT; $query = $form->getElement('query')->setDecorators(array('ViewHelper')); $badges = new FilterBadgeRenderer($this->initialFilter); - $form->setIgnoreChangeDiscarding(true); return '
' . $badges->render($view) . '
' . $form; $html = str_replace('{{FORM}}', $form->render($view), self::$TPL); $html = '
' . $html . '
'; diff --git a/modules/monitoring/application/controllers/ListController.php b/modules/monitoring/application/controllers/ListController.php index 5e26d3654..8005e8713 100644 --- a/modules/monitoring/application/controllers/ListController.php +++ b/modules/monitoring/application/controllers/ListController.php @@ -74,7 +74,7 @@ class Monitoring_ListController extends Controller public function init() { $this->backend = Backend::createBackend($this->_getParam('backend')); - $this->view->grapher = Hook::get('grapher'); + $this->view->grapher = Hook::first('grapher'); $this->createTabs(); $this->view->activeRowHref = $this->getParam('detail'); $this->view->compact = ($this->_request->getParam('view') === 'compact'); diff --git a/modules/monitoring/application/views/helpers/CommandForm.php b/modules/monitoring/application/views/helpers/CommandForm.php index d3124019d..7ebc788c6 100644 --- a/modules/monitoring/application/views/helpers/CommandForm.php +++ b/modules/monitoring/application/views/helpers/CommandForm.php @@ -56,7 +56,6 @@ class Zend_View_Helper_CommandForm extends Zend_View_Helper_Abstract { $form = new Form(); $form->setAttrib('class', 'inline'); - $form->setIgnoreChangeDiscarding(true); $form->setRequest(Zend_Controller_Front::getInstance()->getRequest()); // Filter work only from get parts. Put important diff --git a/modules/monitoring/library/Monitoring/Environment.php b/modules/monitoring/library/Monitoring/Environment.php index a1e41d732..0213118a2 100644 --- a/modules/monitoring/library/Monitoring/Environment.php +++ b/modules/monitoring/library/Monitoring/Environment.php @@ -37,12 +37,12 @@ class Environment public static function grapher($env = null) { - return Hook::get('grapher', null, self::config($env, 'grapher')); + return Hook::createInstance('grapher', null, self::config($env, 'grapher')); } public static function configBackend($env = null) { - return Hook::get( + return Hook::createInstance( 'configBackend', null, self::config($env, 'configBackend') diff --git a/test/php/library/Icinga/Web/HookTest.php b/test/php/library/Icinga/Web/HookTest.php index 9c14f1f62..9e82ad90d 100644 --- a/test/php/library/Icinga/Web/HookTest.php +++ b/test/php/library/Icinga/Web/HookTest.php @@ -4,27 +4,16 @@ namespace Tests\Icinga\Web; -use \Mockery; +use Mockery; +use Exception; use Icinga\Web\Hook; use Icinga\Test\BaseTestCase; -class Base -{ -} - -class TestHookImplementation extends Base -{ -} - -class TestBadHookImplementation -{ -} - class ErrorProneHookImplementation { public function __construct() { - throw new \Exception("HOOK ERROR"); + throw new Exception(); } } @@ -42,103 +31,182 @@ class HookTest extends BaseTestCase Hook::clean(); } - public function testHas() + public function testWhetherHasReturnsTrueIfGivenAKnownHook() { - $this->assertFalse(Hook::has("a")); - $this->assertFalse(Hook::has("a","b")); + Hook::registerClass('TestHook', __FUNCTION__, get_class(Mockery::mock(Hook::$BASE_NS . 'TestHook'))); - Hook::registerClass("a","b","c"); - $this->assertTrue(Hook::has("a")); - $this->assertTrue(Hook::has("a","b")); + $this->assertTrue(Hook::has('TestHook'), 'Hook::has does not return true if given a known hook'); } - public function testCreateInstance() + public function testWhetherHasReturnsFalseIfGivenAnUnknownHook() { - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - Hook::registerClass("Base","b","Tests\\Icinga\\Web\\TestHookImplementation"); - $this->assertInstanceOf("Tests\\Icinga\\Web\\TestHookImplementation",Hook::createInstance("Base","b")); - Hook::clean(); + $this->assertFalse(Hook::has('not_existing'), 'Hook::has does not return false if given an unknown hook'); } - public function testCreateInvalidInstance1() + public function testWhetherHooksCanBeRegisteredWithRegisterClass() { - $this->setExpectedException('\Icinga\Exception\ProgrammingError'); - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - Hook::registerClass("Base","b","Tests\\Icinga\\Web\\TestBadHookImplementation"); - Hook::createInstance("Base","b"); - Hook::clean(); + Hook::registerClass('TestHook', __FUNCTION__, get_class(Mockery::mock(Hook::$BASE_NS . 'TestHook'))); + + $this->assertTrue(Hook::has('TestHook'), 'Hook::registerClass does not properly register a given hook'); } - public function testCreateInvalidInstance2() + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterClass + */ + public function testWhetherMultipleHooksOfTheSameTypeCanBeRegisteredWithRegisterClass() { - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - $test = Hook::createInstance("Base","NOTEXIST"); - $this->assertNull($test); - } + $firstHook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + $secondHook = Mockery::mock('overload:' . get_class($firstHook)); - public function testCreateInvalidInstance3() - { - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - Hook::register("Base","ErrorProne","Tests\\Icinga\\Web\\ErrorProneHookImplementation"); - $test = Hook::createInstance("Base","ErrorProne"); - $this->assertNull($test); - } - - public function testAll() - { - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - Hook::registerClass("Base","a","Tests\\Icinga\\Web\\TestHookImplementation"); - Hook::registerClass("Base","b","Tests\\Icinga\\Web\\TestHookImplementation"); - Hook::registerClass("Base","c","Tests\\Icinga\\Web\\TestHookImplementation"); - $this->assertCount(3,Hook::all("Base")); - foreach(Hook::all("Base") as $instance) { - $this->assertInstanceOf("Tests\\Icinga\\Web\\TestHookImplementation",$instance); - } - } - - public function testFirst() - { - Hook::$BASE_NS = "Tests\\Icinga\\Web\\"; - Hook::registerClass("Base","a","Tests\\Icinga\\Web\\TestHookImplementation"); - Hook::registerClass("Base","b","Tests\\Icinga\\Web\\TestHookImplementation"); - Hook::registerClass("Base","c","Tests\\Icinga\\Web\\TestHookImplementation"); - - $this->assertInstanceOf("Tests\\Icinga\\Web\\TestHookImplementation",Hook::first("Base")); - } - - public function testRegisterObject() - { - $o1 = Mockery::mock('Some\\Name\\Space\\ObjectHook'); - $o1->test = '$123123'; - $o2 = Mockery::mock('Some\\Name\\Space\\ObjectHook'); - $o2->test = '#456456'; - - Hook::registerObject('Test', 'o1', $o1); - Hook::registerObject('Test', 'o2', $o2); - - $this->assertInstanceOf('Some\\Name\\Space\\ObjectHook', Hook::createInstance('Test', 'o1')); - $this->assertInstanceOf('Some\\Name\\Space\\ObjectHook', Hook::createInstance('Test', 'o2')); - - $string = ""; - foreach (Hook::all('Test') as $hook) { - $string .= $hook->test; - } - $this->assertEquals('$123123#456456', $string); + Hook::registerClass('TestHook', 'one', get_class($firstHook)); + Hook::registerClass('TestHook', 'two', get_class($secondHook)); + $this->assertInstanceOf( + get_class($secondHook), + Hook::createInstance('TestHook', 'two'), + 'Hook::registerClass is not able to register different hooks of the same type' + ); } /** * @expectedException Icinga\Exception\ProgrammingError - * @expectedExceptionMessage object is not an instantiated class */ - public function testErrorObjectRegistration() + public function testWhetherOnlyClassesCanBeRegisteredAsHookWithRegisterClass() { - Hook::registerObject('Test', 'e1', 'STRING'); + Hook::registerClass('TestHook', __FUNCTION__, 'nope'); } - public function testGetZeroHooks() + public function testWhetherHooksCanBeRegisteredWithRegisterObject() { - $nh = Hook::all('DOES_NOT_EXIST'); - $this->assertInternalType('array', $nh); - $this->assertCount(0, $nh); + Hook::registerObject('TestHook', __FUNCTION__, Mockery::mock(Hook::$BASE_NS . 'TestHook')); + + $this->assertTrue(Hook::has('TestHook'), 'Hook::registerObject does not properly register a given hook'); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterObject + */ + public function testWhetherMultipleHooksOfTheSameTypeCanBeRegisteredWithRegisterObject() + { + $firstHook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + $secondHook = Mockery::mock('overload:' . get_class($firstHook)); + + Hook::registerObject('TestHook', 'one', $firstHook); + Hook::registerObject('TestHook', 'two', $secondHook); + $this->assertInstanceOf( + get_class($secondHook), + Hook::createInstance('TestHook', 'two'), + 'Hook::registerObject is not able to register different hooks of the same type' + ); + } + + /** + * @expectedException Icinga\Exception\ProgrammingError + */ + public function testWhetherOnlyObjectsCanBeRegisteredAsHookWithRegisterObject() + { + Hook::registerObject('TestHook', __FUNCTION__, 'nope'); + } + + public function testWhetherCreateInstanceReturnsNullIfGivenAnUnknownHookName() + { + $this->assertNull( + Hook::createInstance('not_existing', __FUNCTION__), + 'Hook::createInstance does not return null if given an unknown hook' + ); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterClass + */ + public function testWhetherCreateInstanceInitializesHooksInheritingFromAPredefinedAbstractHook() + { + $baseHook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + Hook::registerClass( + 'TestHook', + __FUNCTION__, + get_class(Mockery::mock('overload:' . get_class($baseHook))) + ); + + $this->assertInstanceOf( + get_class($baseHook), + Hook::createInstance('TestHook', __FUNCTION__), + 'Hook::createInstance does not initialize hooks inheriting from a predefined abstract hook' + ); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterClass + */ + public function testWhetherCreateInstanceDoesNotInitializeMultipleHooksForASpecificIdentifier() + { + Hook::registerClass('TestHook', __FUNCTION__, get_class(Mockery::mock(Hook::$BASE_NS . 'TestHook'))); + $secondHook = Hook::createInstance('TestHook', __FUNCTION__); + $thirdHook = Hook::createInstance('TestHook', __FUNCTION__); + + $this->assertSame( + $secondHook, + $thirdHook, + 'Hook::createInstance initializes multiple hooks for a specific identifier' + ); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterClass + */ + public function testWhetherCreateInstanceReturnsNullIfHookCannotBeInitialized() + { + Hook::registerClass('TestHook', __FUNCTION__, 'Tests\Icinga\Web\ErrorProneHookImplementation'); + + $this->assertNull(Hook::createInstance('TestHook', __FUNCTION__)); + } + + /** + * @expectedException Icinga\Exception\ProgrammingError + * @depends testWhetherHooksCanBeRegisteredWithRegisterClass + */ + public function testWhetherCreateInstanceThrowsAnErrorIfGivenAHookNotInheritingFromAPredefinedAbstractHook() + { + Mockery::mock(Hook::$BASE_NS . 'TestHook'); + Hook::registerClass('TestHook', __FUNCTION__, get_class(Mockery::mock('TestHook'))); + + Hook::createInstance('TestHook', __FUNCTION__); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterObject + */ + public function testWhetherAllReturnsAllRegisteredHooks() + { + $hook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + Hook::registerObject('TestHook', 'one', $hook); + Hook::registerObject('TestHook', 'two', $hook); + Hook::registerObject('TestHook', 'three', $hook); + + $this->assertCount(3, Hook::all('TestHook'), 'Hook::all does not return all registered hooks'); + } + + public function testWhetherAllReturnsNothingIfGivenAnUnknownHookName() + { + $this->assertEmpty( + Hook::all('not_existing'), + 'Hook::all does not return an empty array if given an unknown hook' + ); + } + + /** + * @depends testWhetherHooksCanBeRegisteredWithRegisterObject + */ + public function testWhetherFirstReturnsTheFirstRegisteredHook() + { + $firstHook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + $secondHook = Mockery::mock(Hook::$BASE_NS . 'TestHook'); + Hook::registerObject('TestHook', 'first', $firstHook); + Hook::registerObject('TestHook', 'second', $secondHook); + + $this->assertSame( + $firstHook, + Hook::first('TestHook'), + 'Hook::first does not return the first registered hook' + ); } } From c5c375e72d00e8021a6c4e3530ad3441c7ebd065 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 24 Apr 2014 16:14:27 +0200 Subject: [PATCH 25/29] Add test for Icinga\Web\MenuItem refs #6011 --- library/Icinga/Web/Menu.php | 50 ++++++++---- library/Icinga/Web/MenuItem.php | 86 +++++++++++--------- test/php/library/Icinga/Web/MenuItemTest.php | 29 +++++++ 3 files changed, 112 insertions(+), 53 deletions(-) create mode 100644 test/php/library/Icinga/Web/MenuItemTest.php diff --git a/library/Icinga/Web/Menu.php b/library/Icinga/Web/Menu.php index 28a6b3a6b..a92679dfa 100644 --- a/library/Icinga/Web/Menu.php +++ b/library/Icinga/Web/Menu.php @@ -29,9 +29,9 @@ namespace Icinga\Web; +use Icinga\Logger\Logger; use Icinga\Application\Config; use Icinga\Application\Icinga; -use Icinga\Logger\Logger; use Icinga\Exception\NotReadableError; class Menu extends MenuItem @@ -39,34 +39,49 @@ class Menu extends MenuItem /** * Create menu from the application's menu config file plus the config files from all enabled modules * - * @return Menu + * @return self */ public static function fromConfig() { $menu = new static('menu'); - $manager = Icinga::app()->getModuleManager(); + $manager = Icinga::app()->getModuleManager(); + try { $menuConfigs = array(Config::app('menu')); } catch (NotReadableError $e) { Logger::error($e); $menuConfigs = array(); } - try { - foreach ($manager->listEnabledModules() as $moduleName) { - $moduleMenuConfig = Config::module($moduleName, 'menu'); - if ($moduleMenuConfig) { - $menuConfigs[] = $moduleMenuConfig; - } - } + try { + $modules = $manager->listEnabledModules(); } catch (NotReadableError $e) { Logger::error($e); + $modules = array(); } + + foreach ($modules as $moduleName) { + try { + $moduleMenuConfig = Config::module($moduleName, 'menu'); + } catch (NotReadableError $e) { + Logger::error($e); + $moduleMenuConfig = array(); + } + + if (!empty($moduleMenuConfig)) { + $menuConfigs[] = $moduleMenuConfig; + } + } + return $menu->loadMenuItems($menu->flattenConfigs($menuConfigs)); } /** - * Flatten configs into a key-value array + * Flatten configs + * + * @param array $configs An two dimensional array of menu configurations + * + * @return array The flattened config, as key-value array */ public function flattenConfigs(array $configs) { @@ -79,18 +94,23 @@ class Menu extends MenuItem $flattened[$section] = $itemConfig; } } - ksort($flattened); + return $flattened; } /** - * Load menu items from a key-value array + * Load menu items + * + * @param array $items The items to load, as key-value array + * + * @return self */ - public function loadMenuItems(array $flattened) + public function loadMenuItems(array $items) { - foreach ($flattened as $id => $itemConfig) { + foreach ($items as $id => $itemConfig) { $this->addChild($id, $itemConfig); } + return $this; } } diff --git a/library/Icinga/Web/MenuItem.php b/library/Icinga/Web/MenuItem.php index 16fd30548..6f5394ca6 100644 --- a/library/Icinga/Web/MenuItem.php +++ b/library/Icinga/Web/MenuItem.php @@ -13,7 +13,7 @@ class MenuItem * * @type string */ - private $id; + protected $id; /** * Item title @@ -22,7 +22,7 @@ class MenuItem * * @type string */ - private $title; + protected $title; /** * Item priority @@ -31,44 +31,48 @@ class MenuItem * * @type int */ - private $priority = 100; + protected $priority = 100; /** * Item url * * @type string */ - private $url; + protected $url; /** * Item icon path * * @type string */ - private $icon; + protected $icon; /** * Item icon class * * @type string */ - private $iconClass; + protected $iconClass; /** * Item's children * * @type array */ - private $children = array(); - - private $attribs = array(); + protected $children = array(); + /** + * HTML anchor tag attributes + * + * @var array + */ + protected $attribs = array(); /** * Create a new MenuItem * - * @param int $id - * @param object $config + * @param int $id + * @param object $config */ public function __construct($id, $config = null) { @@ -81,7 +85,7 @@ class MenuItem /** * Setter for id * - * @param string $id + * @param string $id * * @return self */ @@ -104,7 +108,7 @@ class MenuItem /** * Setter for title * - * @param string $title + * @param string $title * * @return self */ @@ -114,11 +118,10 @@ class MenuItem return $this; } - /** * Getter for title * - * @return string + * @return string */ public function getTitle() { @@ -128,7 +131,7 @@ class MenuItem /** * Setter for priority * - * @param int $priority + * @param int $priority * * @return self */ @@ -141,7 +144,7 @@ class MenuItem /** * Getter for priority * - * @return int + * @return int */ public function getPriority() { @@ -151,7 +154,7 @@ class MenuItem /** * Setter for URL * - * @param string $url + * @param string $url * * @return self */ @@ -164,7 +167,7 @@ class MenuItem /** * Getter for URL * - * @return string + * @return string */ public function getUrl() { @@ -174,7 +177,7 @@ class MenuItem /** * Setter for icon path * - * @param string $path + * @param string $path * * @return self */ @@ -187,7 +190,7 @@ class MenuItem /** * Getter for icon path * - * @return string + * @return string */ public function getIcon() { @@ -197,7 +200,7 @@ class MenuItem /** * Setter for icon class * - * @param string $iconClass + * @param string $iconClass * * @return self */ @@ -210,7 +213,7 @@ class MenuItem /** * Getter for icon class * - * @return string + * @return string */ public function getIconClass() { @@ -220,7 +223,7 @@ class MenuItem /** * Set the configuration for the item * - * @param object $config + * @param Zend_Config $config */ public function setConfig($config) { @@ -249,18 +252,21 @@ class MenuItem } else { // Submenu item list($parentId, $id) = explode('.', $id, 2); + if ($this->hasChild($parentId)) { $parent = $this->getChild($parentId); } else { $parent = $this->addChild($parentId); } + $menuItem = $parent->addChild($id, $itemConfig); } + return $menuItem; } /** - * Check whether the item has children + * Check whether the item has any children * * @return bool */ @@ -270,10 +276,11 @@ class MenuItem } /** - * Get children sorted + * Get sorted children * * @return array - * @see cmpChildren() + * + * @see MenuItem::cmpChildren() */ public function getChildren() { @@ -282,11 +289,11 @@ class MenuItem } /** - * Whether a given child id exists + * Return whether a given child id exists * * @param string $id * - * @return self|$default + * @return bool */ public function hasChild($id) { @@ -300,20 +307,22 @@ class MenuItem * @param mixed $default * * @return MenuItem + * * @throws ProgrammingError */ public function getChild($id) { - if ($this->hasChild($id)) { - return $this->children[$id]; + if (!$this->hasChild($id)) { + throw new ProgrammingError(sprintf('Trying to get invalid Menu child "%s"', $id)); } - throw new ProgrammingError(sprintf('Trying to get invalid Menu child "%s"', $id)); + + return $this->children[$id]; } /** - * Set HTML a tag attributes + * Set HTML anchor tag attributes * - * @param array $attribs + * @param array $attribs * * @return self */ @@ -324,9 +333,9 @@ class MenuItem } /** - * Get HTML a tag attributes + * Get HTML anchor tag attributes * - * @return array + * @return array */ public function getAttribs() { @@ -336,8 +345,8 @@ class MenuItem /** * Compare children based on priority and title * - * @param MenuItem $a - * @param MenuItem $b + * @param MenuItem $a + * @param MenuItem $b * * @return int */ @@ -346,6 +355,7 @@ class MenuItem if ($a->priority === $b->priority) { return ($a->getTitle() > $b->getTitle()) ? 1 : -1; } + return ($a->priority > $b->priority) ? 1 : -1; } } diff --git a/test/php/library/Icinga/Web/MenuItemTest.php b/test/php/library/Icinga/Web/MenuItemTest.php new file mode 100644 index 000000000..f8234ab5e --- /dev/null +++ b/test/php/library/Icinga/Web/MenuItemTest.php @@ -0,0 +1,29 @@ +addChild(5, new Zend_Config(array('title' => 'ccc5'))); + $item->addChild(0, new Zend_Config(array('title' => 'aaa'))); + $item->addChild(3, new Zend_Config(array('title' => 'ccc'))); + $item->addChild(2, new Zend_Config(array('title' => 'bbb'))); + $item->addChild(4, new Zend_Config(array('title' => 'ccc2'))); + $item->addChild(1, new Zend_Config(array('title' => 'bb'))); + + $this->assertEquals( + array('aaa', 'bb', 'bbb', 'ccc', 'ccc2', 'ccc5'), + array_map(function ($it) { return $it->getTitle(); }, $item->getChildren()), + 'MenuItem::getChildren does not return its elements in natural order' + ); + } +} From 729c33c3cf963fde7129c368666ae1acdda8e05a Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 24 Apr 2014 16:32:48 +0200 Subject: [PATCH 26/29] Add helpful documentation to Icinga\Web\ViewStream --- library/Icinga/Web/ViewStream.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/Icinga/Web/ViewStream.php b/library/Icinga/Web/ViewStream.php index c44e67fa5..715316201 100644 --- a/library/Icinga/Web/ViewStream.php +++ b/library/Icinga/Web/ViewStream.php @@ -29,8 +29,11 @@ namespace Icinga\Web; -use Zend_View_Stream as ZfViewStream; +use Zend_View_Stream; -class ViewStream extends ZfViewStream +/** + * Is used in Icinga\Web\View to imitate PHP's short_open_tag + */ +class ViewStream extends Zend_View_Stream { } From 07da92ee4293ca7165cba6d07d808694eee037da Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 25 Apr 2014 14:25:13 +0200 Subject: [PATCH 27/29] Rewrite test for Icinga\Web\Url and fix some bugs refs #6011 --- library/Icinga/Test/BaseTestCase.php | 21 +- library/Icinga/Web/Url.php | 209 +++++----- test/php/library/Icinga/Web/UrlTest.php | 512 ++++++++++++------------ 3 files changed, 394 insertions(+), 348 deletions(-) diff --git a/library/Icinga/Test/BaseTestCase.php b/library/Icinga/Test/BaseTestCase.php index d88245ccd..0bf13212e 100644 --- a/library/Icinga/Test/BaseTestCase.php +++ b/library/Icinga/Test/BaseTestCase.php @@ -25,6 +25,7 @@ namespace Icinga\Test { use RuntimeException; use Mockery; use Zend_Config; + use Zend_Controller_Request_Abstract; use Zend_Test_PHPUnit_ControllerTestCase; use Icinga\Application\Icinga; use Icinga\Util\DateTimeFactory; @@ -142,23 +143,25 @@ namespace Icinga\Test { public function setUp() { parent::setUp(); - $this->setupIcingaMock(); + + $requestMock = Mockery::mock('Icinga\Web\Request'); + $requestMock->shouldReceive('getPathInfo')->andReturn('') + ->shouldReceive('getBaseUrl')->andReturn('/') + ->shouldReceive('getQuery')->andReturn(array()); + $this->setupIcingaMock($requestMock); } /** * Setup mock object for the application's bootstrap + * + * @param Zend_Controller_Request_Abstract $request The request to be returned by + * Icinga::app()->getFrontController()->getRequest() */ - protected function setupIcingaMock() + protected function setupIcingaMock(Zend_Controller_Request_Abstract $request) { $bootstrapMock = Mockery::mock('Icinga\Application\ApplicationBootstrap')->shouldDeferMissing(); $bootstrapMock->shouldReceive('getFrontController->getRequest')->andReturnUsing( - function () { - return Mockery::mock('Request') - ->shouldReceive('getPathInfo')->andReturn('') - ->shouldReceive('getBaseUrl')->andReturn('/') - ->shouldReceive('getQuery')->andReturn(array()) - ->getMock(); - } + function () use ($request) { return $request; } )->shouldReceive('getApplicationDir')->andReturn(self::$appDir); Icinga::setApp($bootstrapMock, true); diff --git a/library/Icinga/Web/Url.php b/library/Icinga/Web/Url.php index 2a9e21f3a..8bfc86aa4 100644 --- a/library/Icinga/Web/Url.php +++ b/library/Icinga/Web/Url.php @@ -30,16 +30,16 @@ namespace Icinga\Web; use Icinga\Application\Icinga; +use Icinga\Exception\ProgrammingError; /** - * Url class that provides convenient access to parameters, allows to modify query parameters and - * returns Urls reflecting all changes made to the url and to the parameters. + * Url class that provides convenient access to parameters, allows to modify query parameters and + * returns Urls reflecting all changes made to the url and to the parameters. * - * Direct instantiation is prohibited and should be done either with @see Url::fromRequest() or - * @see Url::fromUrlString() - * - * Currently, protocol, host and port are ignored and will be implemented when required + * Direct instantiation is prohibited and should be done either with @see Url::fromRequest() or + * @see Url::fromUrlString() * + * Currently, protocol, host and port are ignored and will be implemented when required */ class Url { @@ -48,28 +48,28 @@ class Url * * @var array */ - private $params = array(); + protected $params = array(); /** * An array to map aliases to valid parameters * * @var array */ - private $aliases = array(); + protected $aliases = array(); /** * The site anchor after the '#' * * @var string */ - private $anchor = ''; + protected $anchor = ''; /** * The relative path of this Url, without query parameters * * @var string */ - private $path = ''; + protected $path = ''; /** * The baseUrl that will be appended to @see Url::$path in order to @@ -77,10 +77,11 @@ class Url * * @var string */ - private $baseUrl = '/'; + protected $baseUrl = '/'; - private function __construct() + protected function __construct() { + } /** @@ -89,11 +90,10 @@ class Url * If $params are given, those will be added to the request's parameters * and overwrite any existing parameters * - * @param string $url The string representation of the Url to parse - * @param array $params Parameters that should additionally be considered for the Url - * @param Zend_Request $request A request to use instead of the default one + * @param array $params Parameters that should additionally be considered for the url + * @param Zend_Request $request A request to use instead of the default one * - * @return Url + * @return Url */ public static function fromRequest(array $params = array(), $request = null) { @@ -111,9 +111,9 @@ class Url /** * Return a request object that should be used for determining the URL * - * @return Zend_Abstract_Request + * @return Zend_Abstract_Request */ - private static function getRequest() + protected static function getRequest() { return Icinga::app()->getFrontController()->getRequest(); } @@ -124,38 +124,45 @@ class Url * If $params are given, those will be added to the urls parameters * and overwrite any existing parameters * - * @param string $url The string representation of the Url to parse - * @param array $params An array of parameters that should additionally be considered for the Url - * @param Zend_Request $request A request to use instead of the default one + * @param string $url The string representation of the url to parse + * @param array $params An array of parameters that should additionally be considered for the url + * @param Zend_Request $request A request to use instead of the default one * - * @return Url + * @return Url */ public static function fromPath($url, array $params = array(), $request = null) { - $urlObject = new Url(); if ($request === null) { $request = self::getRequest(); } + + if (!is_string($url)) { + throw new ProgrammingError(sprintf('url "%s" is not a string', $url)); + } + + $urlObject = new Url(); $urlObject->setBaseUrl($request->getBaseUrl()); - /* - * Fetch fragment manually and remove it from the url, to 'help' the parse_url() function - * parsing the url properly. Otherwise calling the function with a fragment, but without a - * query will cause unpredictable behaviour. - */ - $fragment = self::getUrlFragment($url); + // Fetch fragment manually and remove it from the url, to 'help' the parse_url() function + // parsing the url properly. Otherwise calling the function with a fragment, but without a + // query will cause unpredictable behaviour. $url = self::stripUrlFragment($url); $urlParts = parse_url($url); - if (isset($urlParts["path"])) { - $urlObject->setPath($urlParts["path"]); + if (strpos($urlParts["path"], $request->getBaseUrl()) === 0) { + $urlObject->setPath(substr($urlParts["path"], strlen($request->getBaseUrl()))); + } else { + $urlObject->setPath($urlParts["path"]); + } } if (isset($urlParts["query"])) { $urlParams = array(); parse_str($urlParts["query"], $urlParams); $params = array_merge($urlParams, $params); } + + $fragment = self::getUrlFragment($url); if ($fragment !== '') { $urlObject->setAnchor($fragment); } @@ -167,11 +174,11 @@ class Url /** * Get the fragment of a given url * - * @param $url The url containing the fragment. + * @param string $url The url containing the fragment. * - * @return string The fragment without the '#' + * @return string The fragment without the '#' */ - private static function getUrlFragment($url) + protected static function getUrlFragment($url) { $url = parse_url($url); if (isset($url['fragment'])) { @@ -184,11 +191,11 @@ class Url /** * Remove the fragment-part of a given url * - * @param $url string The url to strip from its fragment + * @param string $url The url to strip from its fragment * - * @return string The url without the fragment. + * @return string The url without the fragment */ - private static function stripUrlFragment($url) + protected static function stripUrlFragment($url) { return preg_replace('/#.*$/', '', $url); } @@ -209,9 +216,9 @@ class Url /** * Return the parameter for the given alias * - * @param string $alias The alias to translate + * @param string $alias The alias to translate * - * @return string The parameter name + * @return string The parameter name */ public function translateAlias($alias) { @@ -219,26 +226,28 @@ class Url } /** - * Overwrite the baseUrl. + * Overwrite the baseUrl * * If an empty Url is given '/' is used as the base * - * @param string $baseUrl The url path to use as the Url Base - * @return $this + * @param string $baseUrl The url path to use as the Url Base + * + * @return self */ public function setBaseUrl($baseUrl) { - if (trim($baseUrl) == '') { + if (($baseUrl = rtrim($baseUrl, '/ ')) === '') { $baseUrl = '/'; } + $this->baseUrl = $baseUrl; return $this; } /** - * Return the baseUrl set for this Url + * Return the baseUrl set for this url * - * @return string + * @return string */ public function getBaseUrl() { @@ -248,19 +257,22 @@ class Url /** * Set the relative path of this url, without query parameters * - * @param string $path The path to set + * @param string $path The path to set + * + * @return self */ public function setPath($path) { - $this->path = $path; + $this->path = ltrim($path, '/'); + return $this; } /** - * Return the relative path of this Url, without query parameters + * Return the relative path of this url, without query parameters * * If you want the relative path with query parameters use getRelativeUrl * - * @return string + * @return string */ public function getPath() { @@ -270,36 +282,38 @@ class Url /** * Return the relative url with query parameters as a string * - * @return string + * @return string */ public function getRelativeUrl() { if (empty($this->params)) { - return ltrim($this->path, '/') . $this->anchor; + return $this->path . $this->anchor; } + $params = array(); foreach ($this->params as $param => $value) { $params[$this->translateAlias($param)] = $value; } - return ltrim($this->path, '/') . '?' . http_build_query($params, '', '&') . $this->anchor; + + return $this->path . '?' . http_build_query($params, '', '&') . $this->anchor; } /** * Return the absolute url with query parameters as a string * - * @return string + * @return string */ public function getAbsoluteUrl() { - $url = $this->getRelativeUrl(); - return preg_replace('/\/{2,}/', '/', '/'.$this->baseUrl.'/'.$url); + return $this->baseUrl . ($this->baseUrl !== '/' ? '/' : '') . $this->getRelativeUrl(); } /** * Add a set of parameters to the query part if the keys don't exist yet * - * @param array $params The parameters to add - * @return self + * @param array $params The parameters to add + * + * @return self */ public function addParams(array $params) { @@ -310,8 +324,9 @@ class Url /** * Set and overwrite the given params if one if the same key already exists * - * @param array $params The parameters to set - * @return self + * @param array $params The parameters to set + * + * @return self */ public function overwriteParams(array $params) { @@ -322,8 +337,9 @@ class Url /** * Overwrite the parameters used in the query part * - * @param array $params The new parameters to use for the query part - * @return $this + * @param array $params The new parameters to use for the query part + * + * @return self */ public function setParams(array $params) { @@ -334,7 +350,7 @@ class Url /** * Return all parameters that will be used in the query part * - * @return array An associative key => value array containing all parameters + * @return array An associative key => value array containing all parameters */ public function getParams() { @@ -342,48 +358,52 @@ class Url } /** - * Return true if the Urls' query parameter with $key exists, otherwise false + * Return true if a urls' query parameter exists, otherwise false * - * @param $key A key to check for existing - * @return bool + * @param string $param The url parameter name to check + * + * @return bool */ - public function hasParam($key) + public function hasParam($param) { - return array_key_exists($key, $this->params); + return array_key_exists($param, $this->params); } /** - * Return the Url's query parameter with the name $key if exists, otherwise $default + * Return a url's query parameter if it exists, otherwise $default * - * @param $key A query parameter name to return if existing - * @param mixed $default A value to return when the parameter doesn't exist - * @return mixed + * @param string $param A query parameter name to return if existing + * @param mixed $default A value to return when the parameter doesn't exist + * + * @return mixed */ - public function getParam($key, $default = null) + public function getParam($param, $default = null) { - if ($this->hasParam($key)) { - return $this->params[$key]; + if ($this->hasParam($param)) { + return $this->params[$param]; } + return $default; } /** - * Set a single parameter $key, overwriting existing ones with the same key + * Set a single parameter, overwriting any existing one with the same name * - * @param string $key A string representing the key of the parameter - * @param array|string $value An array or string to set as the parameter value - * @return $this + * @param string $param The query parameter name + * @param array|string $value An array or string to set as the parameter value + * + * @return self */ - public function setParam($key, $value) + public function setParam($param, $value) { - $this->params[$key] = $value; + $this->params[$param] = $value; return $this; } /** * Set the url anchor-part * - * @param $anchor The site's anchor string without the '#' + * @param string $anchor The site's anchor string without the '#' * * @return self */ @@ -396,9 +416,9 @@ class Url /** * Remove provided key (if string) or keys (if array of string) from the query parameter array * - * @param string|array $keyOrArrayOfKeys An array of strings or a string representing the key(s) + * @param string|array $keyOrArrayOfKeys An array of strings or a string representing the key(s) * of the parameters to be removed - * @return $this + * @return self */ public function remove($keyOrArrayOfKeys) { @@ -407,45 +427,51 @@ class Url } else { $this->removeKey($keyOrArrayOfKeys); } + return $this; } /** * Remove all parameters with the parameter names in the $keys array * - * @param array $keys An array of strings containing parameter names to remove - * @return $this + * @param array $keys An array of strings containing parameter names to remove + * + * @return self */ public function removeKeys(array $keys) { foreach ($keys as $key) { $this->removeKey($key); } + return $this; } /** * Remove a single parameter with the provided parameter name $key * - * @param string $key The key to remove from the Url - * @return $this + * @param string $key The key to remove from the url + * + * @return self */ public function removeKey($key) { if (isset($this->params[$key])) { unset($this->params[$key]); } + return $this; } /** * Return a copy of this url without the parameter given * - * The argument can either a single query parameter name or an array of parameter names to + * The argument can be either a single query parameter name or an array of parameter names to * remove from the query list * - * @param string|array $keyOrArrayOfKeys A single string or an array containing parameter names - * @return Url + * @param string|array $keyOrArrayOfKeys A single string or an array containing parameter names + * + * @return Url */ public function getUrlWithout($keyOrArrayOfKeys) { @@ -456,7 +482,8 @@ class Url /** * Alias for @see Url::getAbsoluteUrl() - * @return mixed + * + * @return string */ public function __toString() { diff --git a/test/php/library/Icinga/Web/UrlTest.php b/test/php/library/Icinga/Web/UrlTest.php index 091737981..21a893b62 100644 --- a/test/php/library/Icinga/Web/UrlTest.php +++ b/test/php/library/Icinga/Web/UrlTest.php @@ -4,324 +4,340 @@ namespace Tests\Icinga\Web; -use \Mockery; +use Mockery; use Icinga\Web\Url; use Icinga\Test\BaseTestCase; -/** - * Tests for the Icinga\Web\Url class that provides convenient access to Url manipulation method - */ class UrlTest extends BaseTestCase { - /** - * Tests whether a simple Url without query parameters and baseUrl is correctly parsed and returns correct Urls - */ - function testFromStringWithoutQuery() + public function testWhetherFromRequestWorksWithoutARequest() { - $url = Url::fromPath('http://myHost/my/test/url.html'); - $this->assertEquals( - '/my/test/url.html', - $url->getPath(), - 'Assert the parsed url path to be equal to the input path' - ); - $this->assertEquals( - $url->getPath(), - '/' . $url->getRelativeUrl(), - 'Assert the path of an url without query to be equal the relative path' - ); - } - - /** - * Tests whether a simple Url without query parameters and with baseUrl is correctly parsed and returns correct Urls - */ - function testFromUrlWithBasePath() - { - $url = Url::fromPath('my/test/url.html'); - $url->setBaseUrl('the/path/to'); - $this->assertEquals( - '/the/path/to/my/test/url.html', - $url->getAbsoluteUrl(), - 'Assert the url path to be the base path with the relative path' - ); - } - - /** - * Tests whether query parameters in Urls are correctly recognized and decoded - */ - function testFromUrlWithKeyValueQuery() - { - $url = Url::fromPath('/my/test/url.html?param1=%25arg1¶m2=arg2'); - $this->assertEquals( - '/my/test/url.html', - $url->getPath(), - 'Assert the parsed url path to be equal to the input path' - ); - $this->assertEquals( - array( - 'param1' => '%arg1', - 'param2' => 'arg2' - ), - $url->getParams(), - 'Assert single key=value Url parameters to be correctly parsed and recognized' - ); - } - - /** - * Tests whether unnamed query parameters in Urls are correctly recognized and decoded - */ - function testFromUrlWithArrayInQuery() - { - $url = Url::fromPath('/my/test/url.html?param[]=%25val1¶m[]=%40val2'); - $this->assertEquals( - array( - 'param' => array('%val1', '@val2') - ), - $url->getParams(), - 'Assert arrays in param[] = value syntax to be correctly recognized and parsed as arrays' - ); - } - - /** - * Tests whether named query parameters in Urls are correctly recognized and decoded - */ - function testFromUrlWithAssociativeArrayInQuery() - { - $url = Url::fromPath('/my/test/url.html?param[value]=%25val1¶m[value2]=%40val2'); - $this->assertEquals( - array( - 'param' => array( - 'value' => '%val1', - 'value2' => '@val2' - ) - ), - $url->getParams(), - 'Assert arrays in param[] = value syntax to be correctly recognized and parsed as arrays' - ); - } - - /** - * Tests whether simple query parameters can be correctly added on an existing query and ends up in correct Urls - */ - function testAddQueryParameterToUrlWithoutQuery() - { - $url = Url::fromPath( - '/my/test/url.html', - array( - 'param1' => 'val1', - 'param2' => 'val2' - ) - ); - $url->setBaseUrl('path/to'); - $this->assertEquals( - '/path/to/my/test/url.html?param1=val1&param2=val2', - $url->getAbsoluteUrl(), - 'Assert additional parameters to be correctly added to the Url' - ); - } - - /** - * Test whether parameters are correctly added to existing query parameters - * and existing ones are correctly overwritten if they have the same key - */ - function testOverwritePartialQuery() - { - $url = Url::fromPath( - '/my/test/url.html?param1=oldval1', - array( - 'param1' => 'val1', - 'param2' => 'val2' - ) - ); - $url->setBaseUrl('path/to'); - $this->assertEquals( - '/path/to/my/test/url.html?param1=val1&param2=val2', - $url->getAbsoluteUrl(), - 'Assert additional parameters to be correctly added to the Url and overwriting existing parameters' - ); - } - - /** - * Test whether array parameters are correctly added to an existing Url and end up in correct Urls - */ - function testSetQueryWithArrayParameter() - { - $url = Url::fromPath( - '/my/test/url.html', - array( - 'flatarray' => array('val1', 'val2'), - 'param' => array('value1'=>'val1', 'value2' => 'val2') - ) - ); - $url->setBaseUrl('path/to'); - $this->assertEquals( - '/path/to/my/test/url.html?flatarray'.urlencode('[0]').'=val1&'. - 'flatarray'.urlencode('[1]').'=val2&'. - 'param'.urlencode('[value1]').'=val1&'. - 'param'.urlencode('[value2]').'=val2', - $url->getAbsoluteUrl(), - 'Assert array parameters to be correctly encoded and added to the Url' - ); - } - - /** - * Test whether Urls from the request are correctly parsed when no query is given - */ - function testUrlFromRequestWithoutQuery() - { - $request = Mockery::mock('RequestWithoutQuery'); + $request = Mockery::mock('Icinga\Web\Request'); $request->shouldReceive('getPathInfo')->andReturn('my/test/url.html') - ->shouldReceive('getBaseUrl')->andReturn('path/to') + ->shouldReceive('getBaseUrl')->andReturn('/path/to') + ->shouldReceive('getQuery')->andReturn(array('param1' => 'value1', 'param2' => 'value2')); + $this->setupIcingaMock($request); + + $url = Url::fromRequest(); + $this->assertEquals( + '/path/to/my/test/url.html?param1=value1&param2=value2', + $url->getAbsoluteUrl(), + 'Url::fromRequest does not reassemble the correct url from the global request' + ); + } + + public function testWhetherFromRequestWorksWithARequest() + { + $request = Mockery::mock('Icinga\Web\Request'); + $request->shouldReceive('getPathInfo')->andReturn('my/test/url.html') + ->shouldReceive('getBaseUrl')->andReturn('/path/to') ->shouldReceive('getQuery')->andReturn(array()); $url = Url::fromRequest(array(), $request); $this->assertEquals( '/path/to/my/test/url.html', $url->getAbsoluteUrl(), - 'Asserting absolute path resembling the requests path appended by the baseUrl' + 'Url::fromRequest does not reassemble the correct url from a given request' + ); + } + + public function testWhetherFromRequestAcceptsAdditionalParameters() + { + $request = Mockery::mock('Icinga\Web\Request'); + $request->shouldReceive('getPathInfo')->andReturn('') + ->shouldReceive('getBaseUrl')->andReturn('/') + ->shouldReceive('getQuery')->andReturn(array('key1' => 'val1')); + + $url = Url::fromRequest(array('key1' => 'newval1', 'key2' => 'val2'), $request); + $this->assertEquals( + 'val2', + $url->getParam('key2', 'wrongval'), + 'Url::fromRequest does not accept additional parameters' + ); + $this->assertEquals( + 'newval1', + $url->getParam('key1', 'wrongval1'), + 'Url::fromRequest does not overwrite existing parameters with additional ones' ); } /** - * Test whether Urls from the request are correctly parsed when a query is given + * @expectedException Icinga\Exception\ProgrammingError */ - function testUrlFromRequestWithQuery() + public function testWhetherFromPathProperlyHandlesInvalidUrls() { - $request = Mockery::mock('RequestWithoutQuery'); - $request->shouldReceive('getPathInfo')->andReturn('my/test/url.html') - ->shouldReceive('getBaseUrl')->andReturn('path/to') - ->shouldReceive('getQuery')->andReturn(array( - 'param1' => 'value1', - 'param2' => array('key1' => 'value1', 'key2' => 'value2') - ) + Url::fromPath(null); + } + + public function testWhetherFromPathAcceptsAdditionalParameters() + { + $url = Url::fromPath('/my/test/url.html', array('key' => 'value')); + + $this->assertEquals( + 'value', + $url->getParam('key', 'wrongvalue'), + 'Url::fromPath does not accept additional parameters' + ); + } + + public function testWhetherFromPathProperlyParsesUrlsWithoutQuery() + { + $url = Url::fromPath('/my/test/url.html'); + + $this->assertEquals( + '/', + $url->getBaseUrl(), + 'Url::fromPath does not recognize the correct base url' + ); + $this->assertEquals( + 'my/test/url.html', + $url->getPath(), + 'Url::fromPath does not recognize the correct url path' + ); + } + + /** + * @depends testWhetherFromPathProperlyParsesUrlsWithoutQuery + */ + public function testWhetherFromPathProperlyRecognizesTheBaseUrl() + { + $url = Url::fromPath( + '/path/to/my/test/url.html', + array(), + Mockery::mock(array('getBaseUrl' => '/path/to')) ); - $url = Url::fromRequest(array(), $request); $this->assertEquals( - '/path/to/my/test/url.html?param1=value1&'. - 'param2'.urlencode('[key1]').'=value1&'. - 'param2'.urlencode('[key2]').'=value2', + '/path/to/my/test/url.html', $url->getAbsoluteUrl(), - 'Asserting absolute path resembling the requests path appended by the baseUrl' + 'Url::fromPath does not properly differentiate between the base url and its path' ); } /** - * Test the @see Url::getParam($name, $default) function + * @depends testWhetherFromPathProperlyRecognizesTheBaseUrl */ - function testGetParameterByName() + public function testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters() + { + $url = Url::fromPath('/my/test/url.html?param1=%25arg1¶m2=arg+2' + . '¶m3[]=1¶m3[]=2¶m3[]=3¶m4[key1]=val1¶m4[key2]=val2'); + + $this->assertEquals( + '%arg1', + $url->getParam('param1', 'wrongval'), + 'Url::fromPath does not properly decode escaped characters in query parameter values' + ); + $this->assertEquals( + 'arg 2', + $url->getParam('param2', 'wrongval'), + 'Url::fromPath does not properly decode aliases characters in query parameter values' + ); + $this->assertEquals( + array('1', '2', '3'), + $url->getParam('param3'), + 'Url::fromPath does not properly reassemble query parameter values as sequenced values' + ); + $this->assertEquals( + array('key1' => 'val1', 'key2' => 'val2'), + $url->getParam('param4'), + 'Url::fromPath does not properly reassemble query parameters as associative arrays' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherTranslateAliasTranslatesKnownAliases() + { + $url = Url::fromPath('/my/test/url.html'); + $url->setAliases(array('foo' => 'bar')); + + $this->assertEquals( + 'bar', + $url->translateAlias('foo'), + 'Url::translateAlias does not translate a known alias' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherTranslateAliasDoesNotTranslateUnknownAliases() + { + $url = Url::fromPath('/my/test/url.html'); + $url->setAliases(array('foo' => 'bar')); + + $this->assertEquals( + 'fo', + $url->translateAlias('fo'), + 'Url::translateAlias does translate an unknown alias' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherGetAbsoluteUrlReturnsTheAbsoluteUrl() { $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2'); + $this->assertEquals( - "val", - $url->getParam("param", "wrongval"), - "Asserting a parameter can be fetched via getParam()" - ); - $this->assertEquals( - "val2", - $url->getParam("param2", "wrongval2"), - "Asserting a parameter can be fetched via getParam()" - ); - $this->assertEquals( - "nonexisting", - $url->getParam("param3", "nonexisting"), - "Asserting a non existing parameter returning the default value when fetched via getParam()" + '/my/test/url.html?param=val&param2=val2', + $url->getAbsoluteUrl(), + 'Url::getAbsoluteUrl does not return the absolute url' ); } /** - * Test the Url::remove function with a single key passed + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters */ - function testRemoveSingleParameter() + public function testWhetherGetRelativeUrlReturnsTheRelativeUrl() { $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2'); - $url->remove("param"); + $this->assertEquals( - "val2", - $url->getParam("param2", "wrongval2"), - "Asserting other parameters (param2) not being affected by remove" - ); - $this->assertEquals( - "rightval", - $url->getParam("param", "rightval"), - "Asserting a parameter (param) can be removed via remove" + 'my/test/url.html?param=val&param2=val2', + $url->getRelativeUrl(), + 'Url::getRelativeUrl does not return the relative url' ); } /** - * Test the Url::remove function with an array of keys passed + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters */ - function testRemoveMultipleParameters() + public function testWhetherGetParamReturnsTheCorrectParameter() { - $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); - $url->remove(array("param", "param2")); + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2'); + $this->assertEquals( - "val3", - $url->getParam("param3", "wrongval"), - "Asserting other parameters (param3) not being affected by remove" + 'val', + $url->getParam('param', 'wrongval'), + 'Url::getParam does not return the correct value for an existing parameter' ); $this->assertEquals( - "rightval", - $url->getParam("param", "rightval"), - "Asserting a parameter (param) can be removed via remove in a batch" + 'val2', + $url->getParam('param2', 'wrongval2'), + 'Url::getParam does not return the correct value for an existing parameter' ); $this->assertEquals( - "rightval", - $url->getParam("param2", "rightval"), - "Asserting a parameter (param2) can be removed via remove in a batch" + 'nonexisting', + $url->getParam('param3', 'nonexisting'), + 'Url::getParam does not return the default value for a non existing parameter' ); } /** - * Test the Url::without call and whether it returns a copy instead of working on the called object + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters */ - function testWithoutCall() + public function testWhetherRemoveRemovesAGivenSingleParameter() { - $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); - $url2 = $url->getUrlWithout(array("param")); - $this->assertNotEquals( - $url, - $url2, - "Asserting without creating a copy of the url" - ); - $this->assertTrue( - $url->hasParam("param"), - "Asserting the original Url not being affected when calling 'without'" - ); - $this->assertFalse( - $url2->hasParam("param"), - "Asserting the returned Url being without the passed parameter when calling 'without'" - ); - } + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2'); + $url->remove('param'); - function testAddParamAfterCreation() - { - $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); - $url->addParams(array( - "param4" => "val4", - "param3" => "newval3" - )); $this->assertEquals( - "val4", - $url->getParam("param4", "wrongval"), - "Asserting that a parameter can be added with addParam" + 'val2', + $url->getParam('param2', 'wrongval2'), + 'Url::remove removes not only the given parameter' ); $this->assertEquals( - "val3", - $url->getParam("param3", "wrongval"), - "Asserting that addParam doesn't overwrite existing parameters" + 'rightval', + $url->getParam('param', 'rightval'), + 'Url::remove does not remove the given parameter' ); } /** - * Test whether toString is the same as getAbsoluteUrl + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters */ - function testToString() + public function testWhetherRemoveRemovesAGivenSetOfParameters() { $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); + $url->remove(array('param', 'param2')); + + $this->assertEquals( + 'val3', + $url->getParam('param3', 'wrongval'), + 'Url::remove removes not only the given parameters' + ); + $this->assertEquals( + 'rightval', + $url->getParam('param', 'rightval'), + 'Url::remove does not remove all given parameters' + ); + $this->assertEquals( + 'rightval', + $url->getParam('param2', 'rightval'), + 'Url::remove does not remove all given parameters' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherGetUrlWithoutReturnsACopyOfTheUrlWithoutAGivenSetOfParameters() + { + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); + $url2 = $url->getUrlWithout(array('param', 'param2')); + + $this->assertNotSame($url, $url2, 'Url::getUrlWithout does not return a new copy of the url'); + $this->assertEquals( + array('param3' => 'val3'), + $url2->getParams(), + 'Url::getUrlWithout does not remove a given set of parameters from the url' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherAddParamsDoesNotOverwriteExistingParameters() + { + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); + $url->addParams(array('param4' => 'val4', 'param3' => 'newval3')); + + $this->assertEquals( + 'val4', + $url->getParam('param4', 'wrongval'), + 'Url::addParams does not add new parameters' + ); + $this->assertEquals( + 'val3', + $url->getParam('param3', 'wrongval'), + 'Url::addParams overwrites existing parameters' + ); + } + + /** + * @depends testWhetherFromPathProperlyRecognizesAndDecodesQueryParameters + */ + public function testWhetherOverwriteParamsOverwritesExistingParameters() + { + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); + $url->overwriteParams(array('param4' => 'val4', 'param3' => 'newval3')); + + $this->assertEquals( + 'val4', + $url->getParam('param4', 'wrongval'), + 'Url::addParams does not add new parameters' + ); + $this->assertEquals( + 'newval3', + $url->getParam('param3', 'wrongval'), + 'Url::addParams does not overwrite existing parameters' + ); + } + + /** + * @depends testWhetherGetAbsoluteUrlReturnsTheAbsoluteUrl + */ + public function testWhetherToStringConversionReturnsTheAbsoluteUrl() + { + $url = Url::fromPath('/my/test/url.html?param=val¶m2=val2¶m3=val3'); + $this->assertEquals( $url->getAbsoluteUrl(), (string) $url, - "Asserting whether toString returns the absolute Url" + 'Converting a url to string does not return the absolute url' ); } } From 1df56cbfb9743c7d2669dbc3543904d235e13849 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 25 Apr 2014 16:39:54 +0200 Subject: [PATCH 28/29] Fix errors in tests refs #6011 --- .../forms/Config/Authentication/DbBackendFormTest.php | 6 ++++++ .../Config/Authentication/LdapBackendFormTest.php | 6 ++++++ .../forms/Config/Resource/ResourceFormTest.php | 10 ++++++++++ test/php/library/Icinga/Application/ConfigTest.php | 10 +++++----- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/test/php/application/forms/Config/Authentication/DbBackendFormTest.php b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php index 3e515c97a..b5553a896 100644 --- a/test/php/application/forms/Config/Authentication/DbBackendFormTest.php +++ b/test/php/application/forms/Config/Authentication/DbBackendFormTest.php @@ -4,6 +4,10 @@ namespace Tests\Icinga\Form\Config\Authentication; +// Necessary as some of these tests disable phpunit's preservation +// of the global state (e.g. autoloaders are in the global state) +require_once realpath(dirname(__FILE__) . '/../../../../bootstrap.php'); + use Mockery; use Icinga\Test\BaseTestCase; use Icinga\Form\Config\Authentication\DbBackendForm; @@ -18,6 +22,7 @@ class DbBackendFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testValidBackendIsValid() { @@ -40,6 +45,7 @@ class DbBackendFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testInvalidBackendIsNotValid() { diff --git a/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php index 1da715f1a..a3a0cfcaa 100644 --- a/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php +++ b/test/php/application/forms/Config/Authentication/LdapBackendFormTest.php @@ -4,6 +4,10 @@ namespace Tests\Icinga\Form\Config\Authentication; +// Necessary as some of these tests disable phpunit's preservation +// of the global state (e.g. autoloaders are in the global state) +require_once realpath(dirname(__FILE__) . '/../../../../bootstrap.php'); + use Mockery; use Icinga\Test\BaseTestCase; use Icinga\Form\Config\Authentication\LdapBackendForm; @@ -18,6 +22,7 @@ class LdapBackendFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testValidBackendIsValid() { @@ -40,6 +45,7 @@ class LdapBackendFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testInvalidBackendIsNotValid() { diff --git a/test/php/application/forms/Config/Resource/ResourceFormTest.php b/test/php/application/forms/Config/Resource/ResourceFormTest.php index cbaa55502..e0b5b2999 100644 --- a/test/php/application/forms/Config/Resource/ResourceFormTest.php +++ b/test/php/application/forms/Config/Resource/ResourceFormTest.php @@ -4,6 +4,10 @@ namespace Tests\Icinga\Form\Config\Resource; +// Necessary as some of these tests disable phpunit's preservation +// of the global state (e.g. autoloaders are in the global state) +require_once realpath(dirname(__FILE__) . '/../../../../bootstrap.php'); + use Mockery; use Zend_Config; use Icinga\Test\BaseTestCase; @@ -64,6 +68,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testValidDbResourceIsValid() { @@ -80,6 +85,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testInvalidDbResourceIsNotValid() { @@ -96,6 +102,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testValidLdapResourceIsValid() { @@ -112,6 +119,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testInvalidLdapResourceIsNotValid() { @@ -128,6 +136,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testValidLivestatusResourceIsValid() { @@ -145,6 +154,7 @@ class ResourceFormTest extends BaseTestCase /** * @runInSeparateProcess + * @preserveGlobalState disabled */ public function testInvalidLivestatusResourceIsNotValid() { diff --git a/test/php/library/Icinga/Application/ConfigTest.php b/test/php/library/Icinga/Application/ConfigTest.php index d5dd97e56..bc39afecd 100644 --- a/test/php/library/Icinga/Application/ConfigTest.php +++ b/test/php/library/Icinga/Application/ConfigTest.php @@ -30,7 +30,7 @@ class ConfigTest extends BaseTestCase public function testAppConfig() { - $config = IcingaConfig::app(); + $config = IcingaConfig::app('config', true); $this->assertEquals(1, $config->logging->enable, 'Unexpected value retrieved from config file'); // Test non-existent property where null is the default value $this->assertEquals( @@ -70,28 +70,28 @@ class ConfigTest extends BaseTestCase $config->backend->db->toArray() ); // Test singleton - $this->assertEquals($config, IcingaConfig::app()); + $this->assertEquals($config, IcingaConfig::app('config')); $this->assertEquals(array('logging', 'backend'), $config->keys()); $this->assertEquals(array('enable'), $config->keys('logging')); } public function testAppExtraConfig() { - $extraConfig = IcingaConfig::app('extra'); + $extraConfig = IcingaConfig::app('extra', true); $this->assertEquals(1, $extraConfig->meta->version); $this->assertEquals($extraConfig, IcingaConfig::app('extra')); } public function testModuleConfig() { - $moduleConfig = IcingaConfig::module('amodule'); + $moduleConfig = IcingaConfig::module('amodule', 'config', true); $this->assertEquals(1, $moduleConfig->menu->get('breadcrumb')); $this->assertEquals($moduleConfig, IcingaConfig::module('amodule')); } public function testModuleExtraConfig() { - $moduleExtraConfig = IcingaConfig::module('amodule', 'extra'); + $moduleExtraConfig = IcingaConfig::module('amodule', 'extra', true); $this->assertEquals( 'inetOrgPerson', $moduleExtraConfig->ldap->user->get('ldap_object_class') From 98ca15d1fb52f2e0d4f5d4e8d6437aa7d986f721 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Mon, 28 Apr 2014 14:03:52 +0200 Subject: [PATCH 29/29] Mark less important or non-testable code as skipped for code coverage refs #6011 --- .../clicommands/AutocompleteCommand.php | 2 ++ application/clicommands/HelpCommand.php | 2 ++ application/clicommands/ModuleCommand.php | 2 ++ application/clicommands/WebCommand.php | 2 ++ .../controllers/AuthenticationController.php | 2 ++ application/controllers/ConfigController.php | 2 ++ .../controllers/DashboardController.php | 4 +-- application/controllers/ErrorController.php | 4 +-- application/controllers/FilterController.php | 4 +-- application/controllers/IndexController.php | 4 +-- application/controllers/LayoutController.php | 4 +-- application/controllers/ListController.php | 4 +-- .../controllers/PreferenceController.php | 4 +-- application/controllers/SearchController.php | 8 ++--- application/controllers/StaticController.php | 4 +-- .../forms/Authentication/LoginForm.php | 2 ++ .../forms/Config/ConfirmRemovalForm.php | 2 ++ application/forms/Config/GeneralForm.php | 2 ++ application/forms/Config/LoggingForm.php | 2 ++ application/forms/Dashboard/AddUrlForm.php | 9 ++---- application/forms/Preference/GeneralForm.php | 2 ++ application/views/helpers/FormDateTime.php | 6 ++-- application/views/helpers/FormNumber.php | 6 ++-- .../views/helpers/FormTriStateCheckbox.php | 4 +-- .../Application/ApplicationBootstrap.php | 2 ++ library/Icinga/Application/Benchmark.php | 2 ++ library/Icinga/Application/Cli.php | 2 ++ library/Icinga/Application/EmbeddedWeb.php | 2 ++ library/Icinga/Application/LegacyWeb.php | 2 ++ library/Icinga/Application/Platform.php | 2 ++ library/Icinga/Application/Web.php | 2 ++ library/Icinga/File/Pdf.php | 2 ++ library/Icinga/Logger/Writer/SyslogWriter.php | 2 ++ library/Icinga/User/Message.php | 4 ++- library/Icinga/Util/File.php | 4 ++- .../Web/Controller/ActionController.php | 2 ++ .../Web/Controller/BaseConfigController.php | 4 ++- .../Controller/BasePreferenceController.php | 2 ++ .../Web/Controller/ControllerTabCollector.php | 10 +++---- .../Web/Form/Decorator/BootstrapForm.php | 2 ++ .../Web/Form/Decorator/ConditionalHidden.php | 2 ++ .../Icinga/Web/Form/Decorator/HelpText.php | 4 ++- library/Icinga/Web/Hook/Grapher.php | 2 ++ library/Icinga/Web/Hook/Ticket.php | 2 ++ library/Icinga/Web/JavaScript.php | 2 ++ library/Icinga/Web/LessCompiler.php | 2 ++ library/Icinga/Web/Request.php | 2 ++ library/Icinga/Web/Session.php | 2 ++ library/Icinga/Web/StyleSheet.php | 2 ++ library/Icinga/Web/View.php | 2 ++ library/Icinga/Web/Widget.php | 3 +- library/Icinga/Web/Widget/AbstractWidget.php | 5 ++-- library/Icinga/Web/Widget/AlertMessageBox.php | 8 +++-- .../Web/Widget/Chart/HistoryColorGrid.php | 2 ++ library/Icinga/Web/Widget/Chart/InlinePie.php | 3 +- library/Icinga/Web/Widget/Dashboard.php | 1 - .../Icinga/Web/Widget/FilterBadgeRenderer.php | 2 ++ library/Icinga/Web/Widget/FilterBox.php | 2 ++ library/Icinga/Web/Widget/SortBox.php | 5 ++-- .../Web/Widget/Tabextension/BasketAction.php | 6 ++-- .../Widget/Tabextension/DashboardAction.php | 8 ++--- .../Web/Widget/Tabextension/OutputFormat.php | 30 ++++++++----------- .../Web/Widget/Tabextension/Tabextension.php | 2 +- test/php/phpunit.xml | 14 ++++++--- 64 files changed, 159 insertions(+), 87 deletions(-) diff --git a/application/clicommands/AutocompleteCommand.php b/application/clicommands/AutocompleteCommand.php index b41b59fd8..4171b27c1 100644 --- a/application/clicommands/AutocompleteCommand.php +++ b/application/clicommands/AutocompleteCommand.php @@ -1,4 +1,5 @@ fail("Not implemented yet"); } } +// @codeCoverageIgnoreEnd diff --git a/application/clicommands/WebCommand.php b/application/clicommands/WebCommand.php index 4ad8112cc..12d140f43 100644 --- a/application/clicommands/WebCommand.php +++ b/application/clicommands/WebCommand.php @@ -1,4 +1,5 @@ view->dashboard = $dashboard; } } -// @codingStandardsIgnoreEnd +// @codeCoverageIgnoreEnd diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index d2fc5c5ef..343a40b7f 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -1,5 +1,5 @@ view->request = $error->request; } } -// @codingStandardsIgnoreEnd +// @codeCoverageIgnoreEnd diff --git a/application/controllers/FilterController.php b/application/controllers/FilterController.php index 72f0a2ad0..74ade2492 100644 --- a/application/controllers/FilterController.php +++ b/application/controllers/FilterController.php @@ -1,4 +1,5 @@ renderScript('parts/topbar.phtml'); } } -// @codingStandardsIgnoreEnd +// @codeCoverageIgnoreEnd diff --git a/application/controllers/ListController.php b/application/controllers/ListController.php index a2c1e5138..46a370df6 100644 --- a/application/controllers/ListController.php +++ b/application/controllers/ListController.php @@ -1,5 +1,5 @@ view->form = $form; } } -// @codingStandardsIgnoreEnd +// @codeCoverageIgnoreEnd diff --git a/application/controllers/SearchController.php b/application/controllers/SearchController.php index d0a76a737..5760803da 100644 --- a/application/controllers/SearchController.php +++ b/application/controllers/SearchController.php @@ -1,10 +1,5 @@ - * @license http://www.icinga.org/license/gpl2 GPL, version 2 - */ +// @codeCoverageIgnoreStart use Icinga\Web\Controller\ActionController; use Icinga\Application\Icinga; @@ -50,3 +45,4 @@ class SearchController extends ActionController $this->view->tabs = $dashboard->getTabs(); } } +// @codeCoverageIgnoreEnd diff --git a/application/controllers/StaticController.php b/application/controllers/StaticController.php index 1b85618f6..17089510a 100644 --- a/application/controllers/StaticController.php +++ b/application/controllers/StaticController.php @@ -1,5 +1,5 @@ printStack(); } } -// @codingStandardsIgnoreEnd +// @codeCoverageIgnoreEnd diff --git a/application/forms/Authentication/LoginForm.php b/application/forms/Authentication/LoginForm.php index b4e79cbe8..97ca3ea2c 100644 --- a/application/forms/Authentication/LoginForm.php +++ b/application/forms/Authentication/LoginForm.php @@ -1,4 +1,5 @@ setSubmitLabel("Add To Dashboard"); - } } +// @codeCoverageIgnoreEnd diff --git a/application/forms/Preference/GeneralForm.php b/application/forms/Preference/GeneralForm.php index ca2b6699f..512b54689 100644 --- a/application/forms/Preference/GeneralForm.php +++ b/application/forms/Preference/GeneralForm.php @@ -1,4 +1,5 @@ getClosingBracket(); } } - -// @codingStandardsIgnoreStop +// @codeCoverageIgnoreEnd diff --git a/application/views/helpers/FormTriStateCheckbox.php b/application/views/helpers/FormTriStateCheckbox.php index c41664437..5db0532fc 100644 --- a/application/views/helpers/FormTriStateCheckbox.php +++ b/application/views/helpers/FormTriStateCheckbox.php @@ -1,6 +1,5 @@ '; } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Application/ApplicationBootstrap.php b/library/Icinga/Application/ApplicationBootstrap.php index df8d6a0e1..c0cb83841 100644 --- a/library/Icinga/Application/ApplicationBootstrap.php +++ b/library/Icinga/Application/ApplicationBootstrap.php @@ -1,4 +1,5 @@ loadEnabledModules(); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Application/LegacyWeb.php b/library/Icinga/Application/LegacyWeb.php index bf1ff3a45..acce4ad47 100644 --- a/library/Icinga/Application/LegacyWeb.php +++ b/library/Icinga/Application/LegacyWeb.php @@ -1,4 +1,5 @@ legacyBasedir; } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Application/Platform.php b/library/Icinga/Application/Platform.php index 80bb6896d..dfc98819d 100644 --- a/library/Icinga/Application/Platform.php +++ b/library/Icinga/Application/Platform.php @@ -1,4 +1,5 @@ level; } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Util/File.php b/library/Icinga/Util/File.php index 47b0deb6c..c46025ebc 100644 --- a/library/Icinga/Util/File.php +++ b/library/Icinga/Util/File.php @@ -1,4 +1,5 @@ view->tabs = ControllerTabCollector::collectControllerTabs('ConfigController'); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Controller/BasePreferenceController.php b/library/Icinga/Web/Controller/BasePreferenceController.php index e1c330413..5365ff40f 100644 --- a/library/Icinga/Web/Controller/BasePreferenceController.php +++ b/library/Icinga/Web/Controller/BasePreferenceController.php @@ -1,4 +1,5 @@ save($currentPreferences); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Controller/ControllerTabCollector.php b/library/Icinga/Web/Controller/ControllerTabCollector.php index dded99830..a29e5d57a 100644 --- a/library/Icinga/Web/Controller/ControllerTabCollector.php +++ b/library/Icinga/Web/Controller/ControllerTabCollector.php @@ -1,4 +1,5 @@ '; } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Form/Decorator/ConditionalHidden.php b/library/Icinga/Web/Form/Decorator/ConditionalHidden.php index 305054a88..3964dc7e6 100644 --- a/library/Icinga/Web/Form/Decorator/ConditionalHidden.php +++ b/library/Icinga/Web/Form/Decorator/ConditionalHidden.php @@ -1,4 +1,5 @@ user; } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Session.php b/library/Icinga/Web/Session.php index ef7887d61..220c6b4f7 100644 --- a/library/Icinga/Web/Session.php +++ b/library/Icinga/Web/Session.php @@ -1,4 +1,5 @@ renderVertical($grid); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Widget/Chart/InlinePie.php b/library/Icinga/Web/Widget/Chart/InlinePie.php index d38163aa6..c2ff99314 100644 --- a/library/Icinga/Web/Widget/Chart/InlinePie.php +++ b/library/Icinga/Web/Widget/Chart/InlinePie.php @@ -1,4 +1,5 @@ nodeToBadge(Tree::normalizeTree($this->tree->root)); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Widget/FilterBox.php b/library/Icinga/Web/Widget/FilterBox.php index 4395129ff..1c986ded2 100644 --- a/library/Icinga/Web/Widget/FilterBox.php +++ b/library/Icinga/Web/Widget/FilterBox.php @@ -1,4 +1,5 @@ render($view), $html); } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Widget/SortBox.php b/library/Icinga/Web/Widget/SortBox.php index a3ee99a8f..97e528784 100644 --- a/library/Icinga/Web/Widget/SortBox.php +++ b/library/Icinga/Web/Widget/SortBox.php @@ -1,4 +1,5 @@ not show. - * */ public function __construct(array $disabled = array()) { - foreach ($this->supportedTypes as $type => $values) { - if (in_array($type, $disabled)) { - continue; + foreach ($this->supportedTypes as $type => $tabConfig) { + if (!in_array($type, $disabled)) { + $tabConfig['url'] = Url::fromRequest(); + $tabConfig['tagParams'] = array( + 'target' => '_blank' + ); + $this->tabs[] = new Tab($tabConfig); } - if (!isset($this->supportedTypes[$type])) { - Logger::error('Tried to add an unsupported output type: %s', $type); - continue; - } - $tabConfig = $this->supportedTypes[$type]; - $tabConfig['url'] = Url::fromRequest(); - $tabConfig['tagParams'] = array( - 'target' => '_blank' - ); - $this->tabs[] = new Tab($tabConfig); } } @@ -132,3 +125,4 @@ class OutputFormat implements Tabextension } } } +// @codeCoverageIgnoreEnd diff --git a/library/Icinga/Web/Widget/Tabextension/Tabextension.php b/library/Icinga/Web/Widget/Tabextension/Tabextension.php index 2cf2c7b45..5d51379ef 100644 --- a/library/Icinga/Web/Widget/Tabextension/Tabextension.php +++ b/library/Icinga/Web/Widget/Tabextension/Tabextension.php @@ -29,7 +29,7 @@ namespace Icinga\Web\Widget\Tabextension; -use \Icinga\Web\Widget\Tabs; +use Icinga\Web\Widget\Tabs; /** * Tabextension interface that allows to extend a tabbar with reusable components diff --git a/test/php/phpunit.xml b/test/php/phpunit.xml index b5ab5a395..b36e81fe8 100644 --- a/test/php/phpunit.xml +++ b/test/php/phpunit.xml @@ -23,10 +23,16 @@ - - /usr/share/php - /usr/lib - + + ../../application + ../../library/Icinga + ../../modules + + ../../library/Icinga/Application/functions.php + ../../library/Icinga/Application/webrouter.php + ../../library/Icinga/Web/View/helpers + +