Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions application/controllers/ErrorController.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Icinga\Application\Hook\DbMigrationHook;
use Icinga\Application\MigrationManager;
use Icinga\Exception\IcingaException;
use Throwable;
use Zend_Controller_Plugin_ErrorHandler;
use Icinga\Application\Icinga;
use Icinga\Application\Logger;
Expand Down Expand Up @@ -96,17 +97,23 @@ public function errorAction()
$mm = MigrationManager::instance();
$action = $this->getRequest()->getActionName();
$controller = $this->getRequest()->getControllerName();
if ($action !== 'hint' && $controller !== 'migrations' && $mm->hasMigrations($moduleName)) {
// The view renderer from IPL web doesn't render the HTML content set in the respective
// controller if the error_handler request param is set, as it doesn't support error
// rendering. Since this error handler isn't caused by the migrations controller, we can
// safely unset this.
$this->setParam('error_handler', null);
$this->forward('hint', 'migrations', 'default', [
DbMigrationHook::MIGRATION_PARAM => $moduleName
]);

return;
$hasPending = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you need this variable $hasPending.

I don't see this variable used anywhere. In the try block, you're assigning it a value, but it's not used afterward. You could consider removing it.

try {
$hasPending = $mm->hasPendingMigrations();
if ($action !== 'hint' && $controller !== 'migrations' && $mm->hasMigrations($moduleName)) {
// The view renderer from IPL web doesn't render the HTML content set in the respective
// controller if the error_handler request param is set, as it doesn't support error
// rendering. Since this error handler isn't caused by the migrations controller, we can
// safely unset this.
$this->setParam('error_handler', null);
$this->forward('hint', 'migrations', 'default', [
DbMigrationHook::MIGRATION_PARAM => $moduleName
]);

return;
}
} catch (Throwable $e) {
//suppress
}

$this->getResponse()->setHttpResponseCode(500);
Expand Down
49 changes: 38 additions & 11 deletions application/controllers/MigrationsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
use ipl\Html\Text;
use ipl\Web\Compat\CompatController;
use ipl\Web\Widget\ActionLink;
use ipl\Web\Widget\Icon;
use ipl\Web\Widget\Link;
use Throwable;

class MigrationsController extends CompatController
{
Expand Down Expand Up @@ -55,24 +58,48 @@ public function indexAction(): void

$migrateListForm = new MigrationForm();
$migrateListForm->setAttribute('id', $this->getRequest()->protectId('migration-form'));
$migrateListForm->setRenderDatabaseUserChange(! $mm->validateDatabasePrivileges());
try {
$migrateListForm->setRenderDatabaseUserChange(! $mm->validateDatabasePrivileges());

if ($canApply && $mm->hasPendingMigrations()) {
$migrateAllButton = new SubmitButtonElement(sprintf('migrate-%s', DbMigrationHook::ALL_MIGRATIONS), [
if ($canApply && $mm->hasPendingMigrations()) {
$migrateAllButton = new SubmitButtonElement(sprintf('migrate-%s', DbMigrationHook::ALL_MIGRATIONS), [
'form' => $migrateListForm->getAttribute('id')->getValue(),
'label' => $this->translate('Migrate All'),
'title' => $this->translate('Migrate all pending migrations')
]);
]);

// Is the first button, so will be cloned and that the visible
// button is outside the form doesn't matter for Web's JS
$migrateListForm->registerElement($migrateAllButton);
// Is the first button, so will be cloned and that the visible
// button is outside the form doesn't matter for Web's JS
$migrateListForm->registerElement($migrateAllButton);

// Make sure it looks familiar, even if not inside a form
$migrateAllButton->setWrapper(new HtmlElement('div', Attributes::create(['class' => 'icinga-controls'])));
// Make sure it looks familiar, even if not inside a form
$migrateAllButton->setWrapper(
new HtmlElement('div', Attributes::create(['class' => 'icinga-controls']))
);

$this->controls->getAttributes()->add('class', 'default-layout');
$this->addControl($migrateAllButton);
$this->controls->getAttributes()->add('class', 'default-layout');
$this->addControl($migrateAllButton);
}
} catch (Throwable $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you need to try { such a large code fragment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given your specific error message, re-consider your caught type. (Throwable)

$this->addContent(
new HtmlElement(
'div',
new Attributes(['class' => 'db-connection-warning']),
new Icon('warning'),
new HtmlElement('ul', null),
new HtmlElement(
'p',
null,
new Text($this->translate(
'No Configuration Database selected. '
. 'To establish a valid database connection set the Configuration Database field.'
)),
new HtmlElement('ul', null),
new Link($this->translate('Configuration Database'), 'config/general')
)
)
);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the framework has no configuration DB to migrate:

  • Modules (the code below) may have pending migrations – but you're blocking them as well
  • I'd even guess(!) no configuration DB = no need to migrate framework stuff = fine

}

$this->handleFormatRequest($mm->toArray());
Expand Down
49 changes: 42 additions & 7 deletions application/controllers/MyDevicesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@
namespace Icinga\Controllers;

use Icinga\Common\Database;
use Icinga\Web\Notification;
use Icinga\Web\RememberMe;
use Icinga\Web\RememberMeUserDevicesList;
use ipl\Html\Attributes;
use ipl\Html\HtmlElement;
use ipl\Html\Text;
use ipl\Web\Compat\CompatController;
use ipl\Web\Widget\Icon;
use ipl\Web\Widget\Link;
use Throwable;

/**
* MyDevicesController
Expand Down Expand Up @@ -49,6 +54,42 @@ public function init()

public function indexAction()
{
try {
$this->getDb();
} catch (Throwable $e) {
$hasConfigPermission = $this->hasPermission('config/*');
$configLink = new HtmlDocument();
if ($hasConfigPermission) {
$warningMessage = $this->translate(
'No Configuration Database selected.'
. 'To establish a valid database connection set the Configuration Database field.'
);

$configLink = new Link($this->translate('Configuration Database'), 'config/general');
} else {
$warningMessage = $this->translate(
'No Configuration Database selected.'
. 'You don`t have permission to change this setting. Please contact an administrator.'
);
}

$this->addContent(
new HtmlElement(
'div',
new Attributes(['class' => 'db-connection-warning']),
new Icon('warning'),
new HtmlElement(
'p',
null,
Text::create($warningMessage),
),
$configLink
)
);
Comment on lines +60 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$hasConfigPermission = $this->hasPermission('config/*');
$configLink = new HtmlDocument();
if ($hasConfigPermission) {
$warningMessage = $this->translate(
'No Configuration Database selected.'
. 'To establish a valid database connection set the Configuration Database field.'
);
$configLink = new Link($this->translate('Configuration Database'), 'config/general');
} else {
$warningMessage = $this->translate(
'No Configuration Database selected.'
. 'You don`t have permission to change this setting. Please contact an administrator.'
);
}
$this->addContent(
new HtmlElement(
'div',
new Attributes(['class' => 'db-connection-warning']),
new Icon('warning'),
new HtmlElement(
'p',
null,
Text::create($warningMessage),
),
$configLink
)
);
$hasConfigPermission = $this->hasPermission('config/*');
if ($hasConfigPermission) {
$configLink = new Link($this->translate('Configuration Database'), 'config/general');
$warningMessage = sprintf($this->translate(
'No configuration database is selected. '
. 'To establish a valid database connection set the %s field.'
), $configLink);
} else {
$warningMessage = Text::create(
$this->translate(
'No configuration database is selected. '
. 'You don`t have permission to change this setting. Please contact an administrator.'
)
);
}
$this->addContent(
new HtmlElement(
'div',
new Attributes(['class' => 'db-connection-warning']),
new Icon('warning'),
new HtmlElement(
'p',
null,
HtmlString::create($warningMessage),
)
)
);

I have changed the message and also included the link within the message, which made more sense to me. Also, add ipl\Html\HtmlString to the usages at the top of the file.


return;
}

$name = $this->auth->getUser()->getUsername();

$data = (new RememberMeUserDevicesList())
Expand All @@ -57,12 +98,6 @@ public function indexAction()
->setUrl('my-devices/delete');

$this->addContent($data);

if (! $this->hasDb()) {
Notification::warning(
$this->translate("Users can't stay logged in without a database configuration backend")
);
}
}

public function deleteAction()
Expand Down
13 changes: 13 additions & 0 deletions application/forms/Config/General/ApplicationConfigForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@

namespace Icinga\Forms\Config\General;

use Icinga\Application\Config;
use Icinga\Application\Icinga;
use Icinga\Data\ResourceFactory;
use Icinga\Web\Form;
use ipl\Html\Text;

/**
* Configuration form for general application options
Expand Down Expand Up @@ -100,6 +102,17 @@ public function createElements(array $formData)
)
);

$config = Config::app()->getSection('global');
if (! isset($config->config_resource)) {
$this->warning(
$this->translate(
'No Configuration Database selected.'
. 'Please set the field to establish a valid database connection.'
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'No Configuration Database selected.'
. 'Please set the field to establish a valid database connection.'
'The configuration database has not been selected. '
. 'Please set the required field to establish a valid database connection.'

),
false
);
}

return $this;
}
}
14 changes: 12 additions & 2 deletions application/forms/PreferenceForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Icinga\Web\Session;
use Icinga\Web\StyleSheet;
use ipl\Html\HtmlElement;
use ipl\Html\Text;
use ipl\I18n\GettextTranslator;
use ipl\I18n\Locale;
use ipl\I18n\StaticTranslator;
Expand Down Expand Up @@ -185,7 +186,16 @@ public function createElements(array $formData)
false
);
}

$config = Config::app()->getSection('global');
if (! isset($config->config_resource)) {
$this->warning(
$this->translate(
'No Configuration Database selected.'
. 'To establish a valid database connection set the Configuration Database field.'
Comment on lines +193 to +194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'No Configuration Database selected.'
. 'To establish a valid database connection set the Configuration Database field.'
'The configuration database is selected. '
. 'To establish a valid database connection set the configuration database field.'

I am curious why the link to set the configuration database was not included here like in MyDevicesController.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there’s a small mistake in the suggested text. The correct version should be: “The configuration database has not been selected.”
Also, if I remember correctly, it wasn’t possible to include a link in a Zend Form warning message, since the link would not be rendered. Perhaps you know a workaround?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there’s a small mistake in the suggested text. The correct version should be: “The configuration database has not been selected.”

You are right, use the correct version has you have mentioned.

Also, if I remember correctly, it wasn’t possible to include a link in a Zend Form warning message, since the link would not be rendered. Perhaps you know a workaround?

Good question, I will look into it and will let you know if this is possible :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JolienTrog, easiest way is to add an EmptyState widget directly in the view script application/views/scripts/account/index.phtml, just like you did in application/views/scripts/about/index.phtml and use some css styling to make it appear like a warning. This way you could also include the link.

),
false
);
}
$themeFile = StyleSheet::getThemeFile(Config::app()->get('themes', 'default'));
if (! (bool) Config::app()->get('themes', 'disabled', false)) {
$themes = Icinga::app()->getThemes();
Expand Down Expand Up @@ -439,7 +449,7 @@ public function addSubmitButton()

public function isSubmitted()
{
if (parent::isSubmitted()) {
if (($this->getElement('btn_submit') !== null ) && parent::isSubmitted()) {
return true;
}

Expand Down
38 changes: 32 additions & 6 deletions application/views/scripts/about/index.phtml
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
<?php

use Icinga\Application\Config;
use Icinga\Application\MigrationManager;
use Icinga\Web\Navigation\Renderer\BadgeNavigationItemRenderer;
use ipl\Html\HtmlElement;
use ipl\Web\Widget\EmptyState;
use ipl\Web\Widget\Icon;
use ipl\Web\Widget\StateBadge;

Expand Down Expand Up @@ -93,8 +95,6 @@ use ipl\Web\Widget\StateBadge;
]
);
?>
</div>
</div>

<?php
$mm = MigrationManager::instance();
Expand All @@ -104,10 +104,36 @@ use ipl\Web\Widget\StateBadge;
} catch (Throwable $e) {
// suppress
}
if ($hasPending): ?>
<div class="pending-migrations clearfix">
<h2><?= $this->translate('Pending Migrations') ?></h2>
<table class="name-value-table migrations">
?>
</div>
</div>
<?php $config = Config::app()->getSection('global') ?>
<div class="pending-migrations clearfix">
<?php if($mm->getPendingMigrations() || (!isset($config->config_resource))) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<?php if($mm->getPendingMigrations() || (!isset($config->config_resource))) : ?>
<?php if($mm->getPendingMigrations() || (! isset($config->config_resource))) : ?>

We usually add a space after the ! (not) operator for readability.

<h2><?= $this->translate('Pending Migrations') ?></h2>
<?php endif ?>
<table >
<tr>
<th>
<?php if (!isset($config->config_resource)) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<?php if (!isset($config->config_resource)) : ?>
<?php if (! isset($config->config_resource)) : ?>

We usually add a space after the ! (not) operator for readability.

<?= new EmptyState(
$this->translate('No Configuration Database selected. To establish a valid database connection set: ')
)
?><td>
<?= $this->qlink(
$this->translate('Configuration Database'),
'config/general/',
[],
['title' => sprintf($this->translate('Go to Application/General')), 'Configuration Database']
) ?>
</td>
Comment on lines +119 to +129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<?= new EmptyState(
$this->translate('No Configuration Database selected. To establish a valid database connection set: ')
)
?><td>
<?= $this->qlink(
$this->translate('Configuration Database'),
'config/general/',
[],
['title' => sprintf($this->translate('Go to Application/General')), 'Configuration Database']
) ?>
</td>
<?= new EmptyState(
HtmlString::create(sprintf(
$this->translate('No configuration database is selected. To establish a valid database connection set the %s'),
new Link($this->translate('Configuration Database'), 'config/general')
))
)
?>

HtmlString can render the html in the string. And it looks better to me.
You need to add ipl\Html\HtmlString and ipl\Web\Widget\Link at the top of the file.

Copy link
Contributor Author

@JolienTrog JolienTrog Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other parts of the code, qlink has always been used. Should I now use new Link just this one time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the text and the link are not displayed on the same line, that’s why I used a colon (:), because to me it looks better that way.

image

your suggestion:
image

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to check why this is happening. The issue appears to be caused by the current CSS rules, which are preventing the links from displaying on the same line. Consider updating the layout properties that are causing this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other parts of the code, qlink has always been used. Should I now use new Link just this one time?

It provides a straightforward way to include the link in the EmptyState widget. Hence, you could use it here.

<?php endif ?>
</th>
</tr>
</table>

<?php if ($hasPending): ?>
<table class="name-value-table migrations">
<?php foreach ($mm->getPendingMigrations() as $migration): ?>
<tr>
<th><?= $this->escape($migration->getName()) ?></th>
Expand Down
1 change: 1 addition & 0 deletions library/Icinga/Web/StyleSheet.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class StyleSheet
'css/icinga/health.less',
'css/icinga/php-diff.less',
'css/icinga/pending-migration.less',
'css/icinga/db-connection-warning.less',
];

/**
Expand Down
32 changes: 32 additions & 0 deletions public/css/icinga/db-connection-warning.less
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
.db-connection-warning {
border-radius: .25em;
display: flex;
align-items: center;
margin: 0 0 1em 0;
padding: 1em 1em;
background-color: fade(#ffaa44, 40%);
color: #fff;

p {
margin: 0;
align-items: center;
}

a {
margin: 0;
align-items: center;
margin-left: .5em;
color: #00c3ed;
}

i {
font-size: 2em;

opacity: .4;
align-self: flex-start;
}

ul {
list-style: none;
}
}
Loading