From 3412fdfac19fadbb6eabca4c53ee0651d6c0684d Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Sun, 17 Sep 2017 09:25:13 +0200 Subject: [PATCH 1/8] Enable PHP CodeSniffer in travis testing --- .travis.yml | 3 +-- phpcs.xml | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 phpcs.xml diff --git a/.travis.yml b/.travis.yml index cb5087ec8..cdb7055fe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -42,6 +42,5 @@ before_script: script: # also see: modules/test/application/clicommands/PhpCommand.php - # phpcs is disabled until fixed... - # - php phpcs.phar -p --standard=icingaweb2.ruleset.xml --extensions=php --encoding=utf-8 application/ library/Icinga modules/ test/ + - php phpcs.phar - php phpunit.phar -c modules/test/phpunit.xml --verbose diff --git a/phpcs.xml b/phpcs.xml new file mode 100644 index 000000000..e04ad72f8 --- /dev/null +++ b/phpcs.xml @@ -0,0 +1,23 @@ + + + Sniff our code a while + + application/ + library/Icinga + modules/ + test/ + + vendor/* + application/fonts/fontello-ifont/* + + + + + + + + + + + + From cf4c83011f906887d6b165e5b5b5ec4d5539bd50 Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Tue, 19 Sep 2017 14:39:56 +0200 Subject: [PATCH 2/8] travis: Update phpcs to 3.0.2 And install phpcbf --- test/setup_vendor.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/setup_vendor.sh b/test/setup_vendor.sh index 17ef47533..663db8000 100755 --- a/test/setup_vendor.sh +++ b/test/setup_vendor.sh @@ -4,7 +4,7 @@ set -ex ICINGAWEB_HOME=${ICINGAWEB_HOME:="$(dirname "$(readlink -f $(dirname "$0"))")"} PHP_VERSION="$(php -r 'echo phpversion();')" -PHPCS_VERSION=${PHPCS_VERSION:=2.9.1} +PHPCS_VERSION=${PHPCS_VERSION:=3.0.2} MOCKERY_VERSION=${MOCKERY_VERSION:=0.9.9} HAMCREST_VERSION=${HAMCREST_VERSION:=2.0.0} @@ -32,6 +32,12 @@ if [ ! -e "${phpcs_path}".phar ]; then https://github.com/squizlabs/PHP_CodeSniffer/releases/download/${PHPCS_VERSION}/phpcs.phar fi ln -svf "${phpcs_path}".phar phpcs.phar +phpcbf_path="vendor/phpcbf-${PHPCS_VERSION}" +if [ ! -e "${phpcbf_path}".phar ]; then + wget -O "${phpcbf_path}".phar \ + https://github.com/squizlabs/PHP_CodeSniffer/releases/download/${PHPCS_VERSION}/phpcbf.phar +fi +ln -svf "${phpcbf_path}".phar phpcbf.phar # mockery mockery_path="vendor/mockery-${MOCKERY_VERSION}" From 4bffcf1ba5c094c4362c6adb75868066149e3d7e Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Tue, 19 Sep 2017 14:43:09 +0200 Subject: [PATCH 3/8] style: Update icingaweb2.ruleset.xml So allow common style in our code base. --- icingaweb2.ruleset.xml | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/icingaweb2.ruleset.xml b/icingaweb2.ruleset.xml index 020f26c26..6e1b31e4e 100644 --- a/icingaweb2.ruleset.xml +++ b/icingaweb2.ruleset.xml @@ -3,8 +3,27 @@ The default PSR-2 standard with specifically excluded non-critical sniffs - + + + 0 + + + 0 + + + 0 + + + library/Icinga/Application/Cli.php + library/Icinga/Application/EmbeddedWeb.php + library/Icinga/Application/functions.php + library/Icinga/Application/LegacyWeb.php + library/Icinga/Application/Web.php + library/Icinga/File/Pdf.php + modules/doc/library/Doc/Renderer/DocSectionRenderer.php + modules/monitoring/library/Monitoring/Plugin.php + */test/php/* @@ -25,5 +44,6 @@ */modules/monitoring/library/Monitoring/Backend/Ido/Query/* + */modules/monitoring/library/Monitoring/Backend/Livestatus/Query/* From c3ef50978a2eead5ef1647937e36c306570f4b0a Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Tue, 19 Sep 2017 14:41:10 +0200 Subject: [PATCH 4/8] style: Fix minor styleguide issues for PHPCS --- .../Icinga/Exception/Http/BaseHttpException.php | 1 - .../Exception/Http/HttpExceptionInterface.php | 1 - library/Icinga/Util/Json.php | 2 +- library/Icinga/Web/Form.php | 3 ++- library/Icinga/Web/Widget/Chart/InlinePie.php | 7 ++++--- library/Icinga/Web/Widget/Tabs.php | 4 +++- .../Migrate/Config/UserDomainMigration.php | 1 - .../forms/Config/TransportConfigForm.php | 4 +++- .../Command/Transport/ApiCommandTransport.php | 15 ++++++++++++--- .../application/clicommands/ConfigCommand.php | 11 ++++++++--- 10 files changed, 33 insertions(+), 16 deletions(-) diff --git a/library/Icinga/Exception/Http/BaseHttpException.php b/library/Icinga/Exception/Http/BaseHttpException.php index 781fa906d..cad41c622 100644 --- a/library/Icinga/Exception/Http/BaseHttpException.php +++ b/library/Icinga/Exception/Http/BaseHttpException.php @@ -5,7 +5,6 @@ namespace Icinga\Exception\Http; use Icinga\Exception\IcingaException; - /** * Base class for HTTP exceptions */ diff --git a/library/Icinga/Exception/Http/HttpExceptionInterface.php b/library/Icinga/Exception/Http/HttpExceptionInterface.php index 559b1d27a..c5e0cc749 100644 --- a/library/Icinga/Exception/Http/HttpExceptionInterface.php +++ b/library/Icinga/Exception/Http/HttpExceptionInterface.php @@ -3,7 +3,6 @@ namespace Icinga\Exception\Http; - interface HttpExceptionInterface { /** diff --git a/library/Icinga/Util/Json.php b/library/Icinga/Util/Json.php index 0f757c745..b41da82d5 100644 --- a/library/Icinga/Util/Json.php +++ b/library/Icinga/Util/Json.php @@ -25,7 +25,7 @@ class Json { if (version_compare(phpversion(), '5.4.0', '<')) { $encoded = json_encode($value); - } else if (version_compare(phpversion(), '5.5.0', '<')) { + } elseif (version_compare(phpversion(), '5.5.0', '<')) { $encoded = json_encode($value, $options); } else { $encoded = json_encode($value, $options, $depth); diff --git a/library/Icinga/Web/Form.php b/library/Icinga/Web/Form.php index 08b66f933..9d0d5d707 100644 --- a/library/Icinga/Web/Form.php +++ b/library/Icinga/Web/Form.php @@ -772,7 +772,8 @@ class Form extends Zend_Form $action = $action->without(array_keys($this->getElements())); } - // TODO(el): Re-evalute this necessity. JavaScript could use the container's URL if there's no action set. + // TODO(el): Re-evalute this necessity. + // JavaScript could use the container'sURL if there's no action set. // We MUST set an action as JS gets confused otherwise, if // this form is being displayed in an additional column $this->setAction($action); diff --git a/library/Icinga/Web/Widget/Chart/InlinePie.php b/library/Icinga/Web/Widget/Chart/InlinePie.php index aa0046d35..a88fc8a12 100644 --- a/library/Icinga/Web/Widget/Chart/InlinePie.php +++ b/library/Icinga/Web/Widget/Chart/InlinePie.php @@ -74,14 +74,15 @@ class InlinePie extends AbstractWidget * @var string */ private $template =<<<'EOD' - - + {noscript} EOD; private $noscript =<<<'EOD' EOD; diff --git a/library/Icinga/Web/Widget/Tabs.php b/library/Icinga/Web/Widget/Tabs.php index 2bc296939..979b5ef0a 100644 --- a/library/Icinga/Web/Widget/Tabs.php +++ b/library/Icinga/Web/Widget/Tabs.php @@ -36,7 +36,9 @@ EOT; */ private $dropdownTpl = <<< 'EOT'
  • - + + +
      {TABS}
    diff --git a/modules/migrate/library/Migrate/Config/UserDomainMigration.php b/modules/migrate/library/Migrate/Config/UserDomainMigration.php index f7e52c438..d56722b4e 100644 --- a/modules/migrate/library/Migrate/Config/UserDomainMigration.php +++ b/modules/migrate/library/Migrate/Config/UserDomainMigration.php @@ -127,7 +127,6 @@ class UserDomainMigration rename($from, $to); } } - } protected function migrateNavigation() diff --git a/modules/monitoring/application/forms/Config/TransportConfigForm.php b/modules/monitoring/application/forms/Config/TransportConfigForm.php index 3c41ec800..059ea401d 100644 --- a/modules/monitoring/application/forms/Config/TransportConfigForm.php +++ b/modules/monitoring/application/forms/Config/TransportConfigForm.php @@ -369,7 +369,9 @@ class TransportConfigForm extends ConfigForm 'order' => 0, 'ignore' => true, 'label' => $this->translate('Force Changes'), - 'description' => $this->translate('Check this box to enforce changes without connectivity validation') + 'description' => $this->translate( + 'Check this box to enforce changes without connectivity validation' + ) ) ); diff --git a/modules/monitoring/library/Monitoring/Command/Transport/ApiCommandTransport.php b/modules/monitoring/library/Monitoring/Command/Transport/ApiCommandTransport.php index 6dc5fea24..b371e500f 100644 --- a/modules/monitoring/library/Monitoring/Command/Transport/ApiCommandTransport.php +++ b/modules/monitoring/library/Monitoring/Command/Transport/ApiCommandTransport.php @@ -204,7 +204,10 @@ class ApiCommandTransport implements CommandTransportInterface ->setPayload($command->getData()) ->send(); } catch (JsonDecodeException $e) { - throw new CommandTransportException('Got invalid JSON response from the Icinga 2 API: %s', $e->getMessage()); + throw new CommandTransportException( + 'Got invalid JSON response from the Icinga 2 API: %s', + $e->getMessage() + ); } if (isset($response['error'])) { @@ -256,9 +259,15 @@ class ApiCommandTransport implements CommandTransportInterface try { $response = $request->send(); } catch (CurlException $e) { - throw new CommandTransportException('Couldn\'t connect to the Icinga 2 API: %s', $e->getMessage()); + throw new CommandTransportException( + 'Couldn\'t connect to the Icinga 2 API: %s', + $e->getMessage() + ); } catch (JsonDecodeException $e) { - throw new CommandTransportException('Got invalid JSON response from the Icinga 2 API: %s', $e->getMessage()); + throw new CommandTransportException( + 'Got invalid JSON response from the Icinga 2 API: %s', + $e->getMessage() + ); } if (isset($response['error'])) { diff --git a/modules/setup/application/clicommands/ConfigCommand.php b/modules/setup/application/clicommands/ConfigCommand.php index 5f5d16c04..8e80b9368 100644 --- a/modules/setup/application/clicommands/ConfigCommand.php +++ b/modules/setup/application/clicommands/ConfigCommand.php @@ -96,7 +96,8 @@ class ConfigCommand extends Command * * --path= The URL path to Icinga Web 2 [/icingaweb2] * - * --root|--document-root= The directory from which the webserver will serve files [/path/to/icingaweb2/public] + * --root|--document-root= The directory from which the webserver will serve files + * [/path/to/icingaweb2/public] * * --config= Path to Icinga Web 2's configuration files [/etc/icingaweb2] * @@ -106,9 +107,13 @@ class ConfigCommand extends Command * * icingacli setup config webserver apache * - * icingacli setup config webserver apache --path=/icingaweb2 --document-root=/usr/share/icingaweb2/public --config=/etc/icingaweb2 + * icingacli setup config webserver apache \ + * --path=/icingaweb2 \ + * --document-root=/usr/share/icingaweb2/public \ + * --config=/etc/icingaweb2 * - * icingacli setup config webserver apache --file=/etc/apache2/conf.d/icingaweb2.conf + * icingacli setup config webserver apache \ + * --file=/etc/apache2/conf.d/icingaweb2.conf * * icingacli setup config webserver nginx */ From 5ba3ff3c4d8f6b867a8c0b4aae268faa264c971f Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Tue, 19 Sep 2017 16:09:06 +0200 Subject: [PATCH 5/8] travis: Update build matrix * Only generate locales on precise * Pin version of phpcs for PHP 5.3 --- .travis.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index cdb7055fe..34a4e5f63 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ language: php dist: trusty -sudo: required +sudo: false php: - '5.4' @@ -13,7 +13,10 @@ matrix: include: - php: '5.3' dist: precise + sudo: required env: + - PHPCS_VERSION=2.9.1 + - LOCALE_GEN=1 - ENABLE_LDAP=1 services: @@ -35,7 +38,7 @@ notifications: # also see: test/setup_vendor.sh before_script: - php -m - - sudo locale-gen en_US.UTF-8 de_DE.UTF-8 fr_FR.UTF-8 + - sh -c '[ -z $LOCALE_GEN ] || sudo locale-gen en_US.UTF-8 de_DE.UTF-8 fr_FR.UTF-8' - sh -c '[ -z $ENABLE_LDAP ] || phpenv config-add test/travis-ldap.ini' - test/travis_database.sh - test/setup_vendor.sh From 9921ebc2f81c26ae66c25a14f8f0208096e70df5 Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Mon, 9 Oct 2017 16:43:28 +0200 Subject: [PATCH 6/8] test: Align icingacli test commands with default settings We cleaned the default settings for travis, the CLI commands now use the same default. The following issue should be fixed for issues: https://github.com/Icinga/icinga-packaging/issues/50 --- .../application/clicommands/PhpCommand.php | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/modules/test/application/clicommands/PhpCommand.php b/modules/test/application/clicommands/PhpCommand.php index 1ec80de2f..8a62deffd 100644 --- a/modules/test/application/clicommands/PhpCommand.php +++ b/modules/test/application/clicommands/PhpCommand.php @@ -3,6 +3,7 @@ namespace Icinga\Module\Test\Clicommands; +use Icinga\Application\Icinga; use Icinga\Cli\Command; /** @@ -10,18 +11,6 @@ use Icinga\Cli\Command; */ class PhpCommand extends Command { - /** - * Default arguments and options for PHP_CodeSniffer - * - * @var array - */ - protected $phpcsDefaultParams = array( - '-p', - '--standard=PSR2', - '--extensions=php', - '--encoding=utf-8' - ); - /** * Run all unit-test suites * @@ -42,6 +31,11 @@ class PhpCommand extends Command * icingacli test php unit --verbose * icingacli test php unit --build * icingacli test php unit --include=*SpecialTest + * + * NOTES + * + * Default settings are defined via `modules/test/phpunit.xml` under icingaweb' + * installation directory. */ public function unitAction() { @@ -69,11 +63,12 @@ class PhpCommand extends Command $options[] = $include; } - chdir(realpath(__DIR__ . '/../..')); - $command = $this->getEnvironmentVariables() . $phpUnit . ' ' . join( - ' ', - array_merge($options, $this->params->getAllStandalone()) - ); + chdir(Icinga::app()->getBaseDir()); + $command = $this->getEnvironmentVariables() + . $phpUnit + . ' -c modules/test/phpunit.xml' + . ' ' . join(' ', array_merge($options, $this->params->getAllStandalone())); + if ($this->isVerbose) { $res = `$command`; foreach (preg_split('/\n/', $res) as $line) { @@ -114,6 +109,11 @@ class PhpCommand extends Command * icingacli test php style --build * icingacli test php style --include=path/to/your/file * icingacli test php style --exclude=*someFile* --exclude=someOtherFile* + * + * NOTES + * + * Default rules are defined via `phpcs.xml` and `icingaweb2.ruleset.xml` in icingaweb' + * installation directory. */ public function styleAction() { @@ -141,20 +141,13 @@ class PhpCommand extends Command $arguments = array_filter(array_map(function ($p) { return realpath($p); }, $include)); - if (empty($arguments)) { - $arguments = array( - realpath(__DIR__ . '/../../../../application'), - realpath(__DIR__ . '/../../../../library/Icinga') - ); - } - chdir(realpath(__DIR__ . '/../..')); + chdir(Icinga::app()->getBaseDir()); passthru( $phpcs . ' ' . join( ' ', array_merge( $options, - $this->phpcsDefaultParams, $arguments, $this->params->getAllStandalone() ) From 73a67504893239b01691000c6bdd57e6ced7c352 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 8 Nov 2017 10:07:49 +0100 Subject: [PATCH 7/8] Make phpcs and PhpStorm happy w/ intentional switch, case fall-throughs Unfortunately, PhpStorm does not regonise PSR2 fall-through comments. On the other hand, phpcs does not support the @noinspection phpdoc comment (/**). The fix is a mix of PSR2 comments and @noinspection tags in code comments. --- library/Icinga/Web/Response/JsonResponse.php | 2 ++ modules/monitoring/library/Monitoring/Plugin/Perfdata.php | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/library/Icinga/Web/Response/JsonResponse.php b/library/Icinga/Web/Response/JsonResponse.php index 744456c82..d0da9a4f3 100644 --- a/library/Icinga/Web/Response/JsonResponse.php +++ b/library/Icinga/Web/Response/JsonResponse.php @@ -176,8 +176,10 @@ class JsonResponse extends Response 'status' => $this->status ); switch ($this->status) { + /** @noinspection PhpMissingBreakStatementInspection */ case static::STATUS_ERROR: $body['message'] = $this->getErrorMessage(); + // Fallthrough case static::STATUS_FAIL: $failData = $this->getFailData(); if ($failData !== null || $this->status === static::STATUS_FAIL) { diff --git a/modules/monitoring/library/Monitoring/Plugin/Perfdata.php b/modules/monitoring/library/Monitoring/Plugin/Perfdata.php index b222bf625..6ffaf181f 100644 --- a/modules/monitoring/library/Monitoring/Plugin/Perfdata.php +++ b/modules/monitoring/library/Monitoring/Plugin/Perfdata.php @@ -290,16 +290,20 @@ class Perfdata } switch (count($parts)) { + /* @noinspection PhpMissingBreakStatementInspection */ case 5: if ($parts[4] !== '') { $this->maxValue = self::convert($parts[4], $this->unit); } + /* @noinspection PhpMissingBreakStatementInspection */ case 4: if ($parts[3] !== '') { $this->minValue = self::convert($parts[3], $this->unit); } + /* @noinspection PhpMissingBreakStatementInspection */ case 3: $this->criticalThreshold = trim($parts[2]) ? trim($parts[2]) : null; + // Fallthrough case 2: $this->warningThreshold = trim($parts[1]) ? trim($parts[1]) : null; } From f4ce2a5d8298bb4d43eb253c0bce41d43cf17c9e Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 8 Nov 2017 10:26:41 +0100 Subject: [PATCH 8/8] Fix code style issues --- .../application/clicommands/ListCommand.php | 3 ++- .../application/controllers/CommentController.php | 3 ++- .../monitoring/library/Monitoring/Controller.php | 13 +++++++++---- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/modules/monitoring/application/clicommands/ListCommand.php b/modules/monitoring/application/clicommands/ListCommand.php index 491c12894..a82724aec 100644 --- a/modules/monitoring/application/clicommands/ListCommand.php +++ b/modules/monitoring/application/clicommands/ListCommand.php @@ -148,7 +148,8 @@ class ListCommand extends Command * icingacli monitoring list --problems * icingacli monitoring list --problems --service_state_type 0 * icingacli monitoring list --host=local* --service=*disk* - * icingacli monitoring list --columns 'host,service,service_output' --format='$host$: $service$ ($service_output$)' + * icingacli monitoring list --columns 'host,service,service_output' \ + --format='$host$: $service$ ($service_output$)' */ public function statusAction() { diff --git a/modules/monitoring/application/controllers/CommentController.php b/modules/monitoring/application/controllers/CommentController.php index 4d82a0080..e47105404 100644 --- a/modules/monitoring/application/controllers/CommentController.php +++ b/modules/monitoring/application/controllers/CommentController.php @@ -68,7 +68,8 @@ class CommentController extends Controller $this->view->comment = $this->comment; if ($this->hasPermission('monitoring/command/comment/delete')) { - $listUrl = Url::fromPath('monitoring/list/comments')->setQueryString('comment_type=comment|comment_type=ack'); + $listUrl = Url::fromPath('monitoring/list/comments') + ->setQueryString('comment_type=comment|comment_type=ack'); $form = new DeleteCommentCommandForm(); $form ->populate(array( diff --git a/modules/monitoring/library/Monitoring/Controller.php b/modules/monitoring/library/Monitoring/Controller.php index 470d652de..7c41f3e5a 100644 --- a/modules/monitoring/library/Monitoring/Controller.php +++ b/modules/monitoring/library/Monitoring/Controller.php @@ -45,8 +45,7 @@ class Controller extends IcingaWebController $query->limit(); // Resets any default limit and offset } - switch ($desiredFormat) - { + switch ($desiredFormat) { case 'sql': echo '
    '
                     . htmlspecialchars(wordwrap($query->dump()))
@@ -57,7 +56,10 @@ class Controller extends IcingaWebController
                 $response
                     ->setHeader('Content-Type', 'application/json')
                     ->setHeader('Cache-Control', 'no-store')
-                    ->setHeader('Content-Disposition', 'inline; filename=' . $this->getRequest()->getActionName() . '.json')
+                    ->setHeader(
+                        'Content-Disposition',
+                        'inline; filename=' . $this->getRequest()->getActionName() . '.json'
+                    )
                     ->appendBody(Json::encode($query->getQuery()->fetchAll()))
                     ->sendResponse();
                 exit;
@@ -66,7 +68,10 @@ class Controller extends IcingaWebController
                 $response
                     ->setHeader('Content-Type', 'text/csv')
                     ->setHeader('Cache-Control', 'no-store')
-                    ->setHeader('Content-Disposition', 'attachment; filename=' . $this->getRequest()->getActionName() . '.csv')
+                    ->setHeader(
+                        'Content-Disposition',
+                        'attachment; filename=' . $this->getRequest()->getActionName() . '.csv'
+                    )
                     ->appendBody((string) Csv::fromQuery($query))
                     ->sendResponse();
                 exit;