about/index.php: Add message if database connection fail #5356

JolienTrog · 2025-04-23T14:17:09Z

nilmerg · 2025-04-24T13:44:19Z

Seems like this was already in part addressed by #5236, which hid the stacktrace completely. You're now re-introducing it, but with a different message, even though I said to not show a stacktrace anymore!

But anyway, let's start from scratch here. Please do the following:

Drop the option config_resource (Section global) from /etc/icingaweb2/config.ini
This will simulate the case that no configuration database has been configured
Note down the cases where migrations are checked
Utilize PHPStorm's Find Usages for this
Visit the routes that will trigger a check and see how they cope with a missing/invalid resource
We know already that the about page will suppress the error, but others might not

Then, depending on the individual case, please propose a solution how it can be improved.
In case of the about page, a message at the location where pending migrations are usually shown for example. (Above loaded libraries)

application/controllers/MigrationsController.php

application/controllers/MyDevicesController.php

application/forms/PreferenceForm.php

application/forms/Config/General/ApplicationConfigForm.php

application/views/scripts/about/index.phtml

- Add error handling for database connection failures - Suppress stacktrace and add error messages for user and admin - Improve user experience with clear error messages

Co-authored-by: Ravi Kumar Kempapura Srinivasa <[email protected]>

raviks789 · 2025-10-01T08:22:08Z

application/controllers/ErrorController.php

-                            ]);
-
-                            return;
+                        $hasPending = false;


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.

application/forms/PreferenceForm.php

raviks789 · 2025-10-01T08:46:37Z

application/views/scripts/about/index.phtml

+            <table >
+                <tr>
+                    <th>
+                  <?php  if (!isset($config->config_resource)) : ?>


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.

raviks789 · 2025-10-01T09:28:16Z

application/controllers/MyDevicesController.php

+            $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
+                )
+            );


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.

raviks789 · 2025-10-01T09:33:52Z

application/forms/Config/General/ApplicationConfigForm.php

+                    'No Configuration Database selected.'
+                    . 'Please set the field to establish a valid database connection.'


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.'

raviks789 · 2025-10-01T09:43:40Z

application/forms/PreferenceForm.php

+                    'No Configuration Database selected.'
+                    . 'To establish a valid database connection set the Configuration Database field.'


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.

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?

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 :)

@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.

raviks789 · 2025-10-01T09:44:07Z

application/views/scripts/about/index.phtml

+        </div>
+        <?php $config = Config::app()->getSection('global') ?>
+        <div class="pending-migrations clearfix">
+            <?php if($mm->getPendingMigrations() || (!isset($config->config_resource))) : ?>


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.

raviks789 · 2025-10-01T10:03:05Z

application/views/scripts/about/index.phtml

+                      <?=  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>


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.

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

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.

your suggestion:

What do you think?

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.

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.

Al2Klimov · 2025-10-13T11:40:15Z

application/controllers/MigrationsController.php

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


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

Al2Klimov · 2025-10-13T11:41:04Z

application/controllers/MigrationsController.php

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


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

Al2Klimov · 2025-10-13T11:45:00Z

application/controllers/MigrationsController.php

+                    )
+                )
+            );
+            return;


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

JolienTrog · 2025-10-13T12:11:53Z

@nilmerg :
I talked to @flourish86 and he suggested adding a notification similar to the health check and integrating it into the footer. Compared to a regular warning message, this notification is persistent and doesn’t disappear after a short time. That’s why I didn’t use the regular warning notification, but instead displayed the message at the top of the page. However, this approach is not consistent with the overall UI/UX.
The disadvantage would be that the existing development would basically have to be completely reworked. So, aside from the learning effect for me, a lot of development work would be wasted.

What do you think? Should I keep it like this or use the HealthHook instead?

Workaround version:

HealthHook verson:

cla-bot bot added the cla/signed label Apr 23, 2025

JolienTrog requested a review from nilmerg April 23, 2025 14:17

JolienTrog self-assigned this Apr 23, 2025

JolienTrog added the bug Something isn't working label Apr 23, 2025

JolienTrog force-pushed the fix/stacktrace-for-nonexisting-database-connection branch from 6b82f16 to 15b0148 Compare June 5, 2025 08:28

nilmerg requested a review from raviks789 June 12, 2025 09:24

raviks789 requested changes Jun 13, 2025

View reviewed changes

JolienTrog added 2 commits August 8, 2025 12:25

about/index.php: Add message if database connection fail

603c2b3

Fix stacktrace for missing database connection

fe03c03

- Add error handling for database connection failures - Suppress stacktrace and add error messages for user and admin - Improve user experience with clear error messages

JolienTrog force-pushed the fix/stacktrace-for-nonexisting-database-connection branch from 219a4a0 to 038bc99 Compare August 8, 2025 10:42

fix: resolve review comments

56793c8

Co-authored-by: Ravi Kumar Kempapura Srinivasa <[email protected]>

JolienTrog force-pushed the fix/stacktrace-for-nonexisting-database-connection branch from 883278e to 56793c8 Compare August 8, 2025 11:49

JolienTrog requested a review from raviks789 August 8, 2025 13:02

raviks789 requested changes Oct 1, 2025

View reviewed changes

Al2Klimov requested changes Oct 13, 2025

View reviewed changes

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

		'No Configuration Database selected.'
		. 'Please set the field to establish a valid database connection.'

		'No Configuration Database selected.'
		. 'To establish a valid database connection set the Configuration Database field.'

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

about/index.php: Add message if database connection fail #5356

Are you sure you want to change the base?

about/index.php: Add message if database connection fail #5356

Uh oh!

Conversation

JolienTrog commented Apr 23, 2025

Uh oh!

nilmerg commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JolienTrog Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JolienTrog commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JolienTrog Oct 6, 2025 •

edited

Loading