diff --git a/CHANGELOG.md b/CHANGELOG.md index 38d8c61..3c621a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ --- 1.4.0 --- +* issue: Fix the Fleet Dashboard returning a 500 error (`ArgumentCountError`) when it showed an estimated load. Cacti's `__()` formats a string as soon as it is given more than the text, and the text domain counts as a value, so `__('%s of %s rated', 'apcupsd')` had too few values. Every string with a placeholder now passes its values before the text domain, for example `__('%s of %s rated', $load, $capacity, 'apcupsd')` +* issue: Show the real values in the gauge min/avg/max/now summaries (they read "min apcupsd"), in the NUT error messages ("did not answer within apcupsd seconds", "reported an error: apcupsd", a UPS "named apcupsd"), and in the apcaccess timeout and Graph Automation messages, which had the same problem +* issue: Stop an unreachable NUT server crashing the poller. Building the "Unable to connect to the NUT server" message threw the same `ArgumentCountError`, so the poller died with its output discarded, without polling the remaining UPSes, recording a status or writing the `APCUPSD STATS` log line * issue: Write a WARNING to the Cacti log (APCUPSD) naming the UPS, host, port and reason whenever a NUT query fails; before, the reason was only stored in the UPS's error message and shown with `-d`, so a NUT UPS that could not be reached left nothing in the log * issue: Save the Poller ID chosen on the UPS form; the field was displayed but never stored, so every UPS was always assigned to poller 1. Changing the Poller ID of a UPS that is linked to a Cacti Device is refused with an explanation, since the device's poller has to be changed on the device itself * feature: Add support for monitoring UPSes through Network UPS Tools (NUT) as a third collector type, "NUT Based". The poller talks to `upsd` (TCP 3493 by default) over a native PHP socket implementation of the NUT protocol, so no `upsc` or other CLI tool is needed. Optional username/password authentication and STARTTLS (with certificate verification) are supported, and one NUT server can serve several UPSes through the new NUT UPS Name setting. NUT status flags are translated to the existing apcupsd vocabulary (OL to ONLINE, OB to ONBATT, and so on) so the dashboards, fleet totals and alerts keep working unchanged diff --git a/includes/dashboard.php b/includes/dashboard.php index c145131..3801929 100644 --- a/includes/dashboard.php +++ b/includes/dashboard.php @@ -558,19 +558,19 @@ function apcupsd_dashboard_bullet(string $label, ?float $current, ?float $min, ? $summary = []; if ($current !== null) { - $summary[] = sprintf(__('now %s', 'apcupsd'), apcupsd_dashboard_format_value($current, $unit)); + $summary[] = __('now %s', apcupsd_dashboard_format_value($current, $unit), 'apcupsd'); } if ($min !== null) { - $summary[] = sprintf(__('min %s', 'apcupsd'), apcupsd_dashboard_format_value($min, $unit)); + $summary[] = __('min %s', apcupsd_dashboard_format_value($min, $unit), 'apcupsd'); } if ($avg !== null) { - $summary[] = sprintf(__('avg %s', 'apcupsd'), apcupsd_dashboard_format_value($avg, $unit)); + $summary[] = __('avg %s', apcupsd_dashboard_format_value($avg, $unit), 'apcupsd'); } if ($max !== null) { - $summary[] = sprintf(__('max %s', 'apcupsd'), apcupsd_dashboard_format_value($max, $unit)); + $summary[] = __('max %s', apcupsd_dashboard_format_value($max, $unit), 'apcupsd'); } $head = '
' @@ -893,15 +893,15 @@ function apcupsd_dashboard_stats_caption(?float $min, ?float $avg, ?float $max, $parts = []; if ($min !== null) { - $parts[] = sprintf(__('min %s', 'apcupsd'), apcupsd_dashboard_format_value($min, $unit)); + $parts[] = __('min %s', apcupsd_dashboard_format_value($min, $unit), 'apcupsd'); } if ($avg !== null) { - $parts[] = sprintf(__('avg %s', 'apcupsd'), apcupsd_dashboard_format_value($avg, $unit)); + $parts[] = __('avg %s', apcupsd_dashboard_format_value($avg, $unit), 'apcupsd'); } if ($max !== null) { - $parts[] = sprintf(__('max %s', 'apcupsd'), apcupsd_dashboard_format_value($max, $unit)); + $parts[] = __('max %s', apcupsd_dashboard_format_value($max, $unit), 'apcupsd'); } return implode(' ยท ', $parts); @@ -2486,7 +2486,7 @@ function apcupsd_fleet_card_body(string $key): string { } $est_load = $est_watts / $est_capacity * 100; - $rated = sprintf(__('%s of %s rated', 'apcupsd'), apcupsd_dashboard_format_value($est_load, '%'), apcupsd_dashboard_format_value($est_capacity, ' W')); + $rated = __('%s of %s rated', apcupsd_dashboard_format_value($est_load, '%'), apcupsd_dashboard_format_value($est_capacity, ' W'), 'apcupsd'); // The range comes from each UPS's load RRD scaled by its (constant) nominal power, // like the Load gauge; without load history the rating is the caption instead. diff --git a/includes/nut.php b/includes/nut.php index 9c54f6e..50a1cb0 100644 --- a/includes/nut.php +++ b/includes/nut.php @@ -174,11 +174,11 @@ function apcupsd_nut_error(string $line, string $ups): string { case 'INVALID-USERNAME': return __('The NUT server denied access. Check the NUT username and password.', 'apcupsd'); case 'UNKNOWN-UPS': - return sprintf(__('The NUT server does not know a UPS named "%s".', 'apcupsd'), $ups); + return __('The NUT server does not know a UPS named "%s".', $ups, 'apcupsd'); case 'DATA-STALE': return __('The NUT server has stale data for this UPS: its driver is not responding.', 'apcupsd'); default: - return sprintf(__('The NUT server reported an error: %s', 'apcupsd'), $code === '' ? 'ERR' : $code); + return __('The NUT server reported an error: %s', $code === '' ? 'ERR' : $code, 'apcupsd'); } } } @@ -374,7 +374,7 @@ function apcupsd_nut_query_stream($stream, string $hostname, string $ups, string // A read that gave up because time ran out looks like a closed connection to the session. if ($result['error'] === __('The NUT server closed the connection unexpectedly.', 'apcupsd') && microtime(true) >= $deadline) { - $result['error'] = sprintf(__('The NUT server did not answer within %s seconds.', 'apcupsd'), $timeout); + $result['error'] = __('The NUT server did not answer within %s seconds.', $timeout, 'apcupsd'); } return $result; @@ -474,7 +474,7 @@ function apcupsd_nut_query(string $hostname, $port, string $ups, string $usernam $stream = @stream_socket_client('tcp://' . $target, $errno, $errstr, $timeout, STREAM_CLIENT_CONNECT, $context); if ($stream === false) { - return ['vars' => [], 'version' => '', 'error' => sprintf(__('Unable to connect to the NUT server %s: %s', 'apcupsd'), $target, $errstr)]; + return ['vars' => [], 'version' => '', 'error' => __('Unable to connect to the NUT server %s: %s', $target, $errstr, 'apcupsd')]; } $result = apcupsd_nut_query_stream($stream, $hostname, $ups, $username, $password, $tls, $timeout, $started + ($timeout * 2)); diff --git a/poller_apcupsd.php b/poller_apcupsd.php index cc8f465..2bcdd33 100644 --- a/poller_apcupsd.php +++ b/poller_apcupsd.php @@ -498,7 +498,7 @@ function collect_ups_data($ups) { })); if ($return != 0 || !cacti_sizeof($output)) { - $message = cacti_sizeof($output) ? implode(', ', $output) : sprintf(__('apcaccess failed or did not answer within %s seconds.', 'apcupsd'), $timeout); + $message = cacti_sizeof($output) ? implode(', ', $output) : __('apcaccess failed or did not answer within %s seconds.', $timeout, 'apcupsd'); db_execute_prepared('UPDATE apcupsd_ups SET status = 1, error_message = ? diff --git a/tests/Unit/I18nPlaceholderTest.php b/tests/Unit/I18nPlaceholderTest.php new file mode 100644 index 0000000..7df1bc8 --- /dev/null +++ b/tests/Unit/I18nPlaceholderTest.php @@ -0,0 +1,113 @@ + 'file:line text' for each offending call. + */ +function apcupsd_test_domain_only_placeholders(string $root): array { + $found = []; + $files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($root, FilesystemIterator::SKIP_DOTS)); + + foreach ($files as $file) { + $path = str_replace('\\', '/', $file->getPathname()); + + if (substr($path, -4) !== '.php' || preg_match('#/(tests|locales|vendor|\.git)/#', $path)) { + continue; + } + + $tokens = token_get_all((string) file_get_contents($path)); + + foreach ($tokens as $i => $token) { + if (!is_array($token) || $token[0] !== T_STRING || !in_array($token[1], ['__', '__esc'], true)) { + continue; + } + + $args = []; + $depth = 0; + $cur = ''; + + for ($j = $i + 1, $n = count($tokens); $j < $n; $j++) { + $text = is_array($tokens[$j]) ? $tokens[$j][1] : $tokens[$j]; + + if (in_array($text, ['(', '[', '{'], true) && ++$depth === 1) { + continue; + } + + if (in_array($text, [')', ']', '}'], true) && --$depth === 0) { + $args[] = trim($cur); + + break; + } + + if ($text === ',' && $depth === 1) { + $args[] = trim($cur); + $cur = ''; + + continue; + } + + $cur .= $text; + } + + if (count($args) === 2 && preg_match('/^([\'"])apcupsd\1$/', $args[1]) && preg_match('/%(?!%)[-+0-9.\']*[bcdeEfFgGosuxX]/', $args[0])) { + $found[] = substr($path, strlen(str_replace('\\', '/', $root)) + 1) . ':' . $token[2] . ' ' . $args[0]; + } + } + } + + return $found; +} + +describe('translated strings with placeholders', function () { + it('pass their values before the text domain', function () { + expect(apcupsd_test_domain_only_placeholders(dirname(__DIR__, 2)))->toBe([]); + }); + + it('is caught by the scanner when the broken form is used', function () { + $dir = sys_get_temp_dir() . '/apcupsd_i18n_' . uniqid(); + mkdir($dir); + file_put_contents($dir . '/bad.php', "toHaveCount(1); + expect($found[0])->toContain('bad.php:2'); + }); + + it('are formatted like Cacti: the text domain is a sprintf value, so too few values throw', function () { + expect(__('%s of %s rated', '45%', '900 W', 'apcupsd'))->toBe('45% of 900 W rated'); + expect(__('now %s', '12 V', 'apcupsd'))->toBe('now 12 V'); + expect(__('Plain text', 'apcupsd'))->toBe('Plain text'); + expect(fn () => __('%s of %s rated', 'apcupsd'))->toThrow(ArgumentCountError::class); + }); +}); + +describe('NUT error messages', function () { + it('name the UPS and error code they were given', function () { + require_once __DIR__ . '/../../includes/nut.php'; + + expect(apcupsd_nut_error('ERR UNKNOWN-UPS', 'ups1'))->toBe('The NUT server does not know a UPS named "ups1".'); + expect(apcupsd_nut_error('ERR SOMETHING-ELSE', 'ups1'))->toBe('The NUT server reported an error: SOMETHING-ELSE'); + expect(apcupsd_nut_error('ERR', 'ups1'))->toBe('The NUT server reported an error: ERR'); + }); +}); diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index 4ad08d5..b19e01b 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -291,14 +291,20 @@ function html_escape($string) { } if (!function_exists('__')) { - function __($text, $domain = '') { - return $text; + // Mirrors Cacti's __(): with more than the text it formats at once, and the trailing text domain is + // just one more sprintf argument. A placeholder with too few values therefore throws, as it does in Cacti. + function __($text, ...$args) { + if ($args === [] || preg_match('/%(?!%)[-+0-9.\']*[bcdeEfFgGosuxX]/', (string) $text) !== 1) { + return $text; + } + + return str_replace('%%', '%', sprintf($text, ...$args)); } } if (!function_exists('__esc')) { - function __esc($text, $domain = '') { - return htmlspecialchars((string) $text, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + function __esc($text, ...$args) { + return htmlspecialchars((string) __($text, ...$args), ENT_QUOTES | ENT_HTML5, 'UTF-8'); } } diff --git a/upses.php b/upses.php index 5d1e9d3..8c86865 100644 --- a/upses.php +++ b/upses.php @@ -627,9 +627,9 @@ function run_graph_automation(array $ups_ids): void { if ($ran === 0) { raise_message('apcupsd_automation', __('None of the selected UPSes has a Cacti Device yet, so Graph Automation was not run. A device is created on the first successful poll.', 'apcupsd'), MESSAGE_LEVEL_WARN); } elseif ($skipped > 0) { - raise_message('apcupsd_automation', sprintf(__('Graph Automation was run for %d UPS(es). %d UPS(es) have no Cacti Device yet and were skipped.', 'apcupsd'), $ran, $skipped), MESSAGE_LEVEL_WARN); + raise_message('apcupsd_automation', __('Graph Automation was run for %d UPS(es). %d UPS(es) have no Cacti Device yet and were skipped.', $ran, $skipped, 'apcupsd'), MESSAGE_LEVEL_WARN); } else { - raise_message('apcupsd_automation', sprintf(__('Graph Automation was run for %d UPS(es).', 'apcupsd'), $ran), MESSAGE_LEVEL_INFO); + raise_message('apcupsd_automation', __('Graph Automation was run for %d UPS(es).', $ran, 'apcupsd'), MESSAGE_LEVEL_INFO); } }