Files
zoneminder/tests/php/test_assignment_precedence.php
T
Isaac ConnorandClaude Opus 5 9b1ae0ce14 fix: show a saved monitor action's schedule as a sentence, not as cron refs #4957
A saved time based action listed its schedule as "0 21 * * *" rather than
"Every day at 21:00". The cron parser was not at fault; cronToParts('0 21 * *
*') returns the right parts. The fault was one operator:

  $drawable = ($action->Schedule() === '') or ($schedule_parts !== null);

"or" binds looser than "=", so this parses as ($drawable = ($action->Schedule()
=== '')) or (...). The right-hand clause is evaluated and thrown away, and
$drawable gets only the left one -- false for any schedule that is actually
set, which is every saved action. So the editor took every saved schedule for
one it could not draw and fell back to the read-only cron field, which is the
branch that exists for genuinely undrawable specs like "*/5 * * * *".

Changed to ||, which binds tighter than "=" and does what the line looks like
it does.

Tests: tests/php/test_assignment_precedence.php, 21 checks. It tokenizes every
php file under web/ and reports assignments whose right-hand side has an
unbracketed "and", "or" or "xor" -- tokenized rather than grepped so that the
many safely bracketed ones, in conditions and ternaries, are not reported.
There is a self-check for each distinction it has to make, since a scan that
quietly matched nothing would pass the tree-wide check on its own. Verified it
fails when the reported line is put back, and when a new instance is added
elsewhere.

The scan found six more in the rest of the tree, which the test carries as a
list of open questions rather than as exemptions. Two are the deliberate
"$fp = fopen(...) or error_log(...)" idiom and are correct. The other four are
this same bug, and each changes behaviour to fix, so none is swept up here --
in particular filter.php:401 assigns only $filter->Id() to $canDelete, so
delete is offered for any existing filter whether or not the user may edit it,
and that is access control rather than something to guess at. Anything not on
the list fails the test, so a new one cannot be added, and the list is checked
in both directions so it shrinks as the entries are dealt with.

Ran: tests/php/test_assignment_precedence.php (21 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
2026-10-28 18:01:26 -05:00

216 lines
9.3 KiB
PHP

<?php
// Tests that no assignment in the web ui is cut short by "and" or "or".
//
// php's "and" and "or" bind *looser* than "=", so
//
// $drawable = ($schedule === '') or ($parts !== null);
//
// parses as ($drawable = ($schedule === '')) or ($parts !== null): the
// right-hand clause is evaluated for its side effects and thrown away, and
// $drawable gets only the left one. "||" and "&&" bind tighter than "=" and do
// what the line looks like it does.
//
// This is a bad one to have in a view. There is no error, no warning, and
// php -l is happy; the expression is legal and simply means something else.
// The instance that prompted this test made every saved schedule render as
// raw cron instead of "Every day at 21:00", and the symptom pointed at the
// cron parser, which was working perfectly.
//
// Tokenized rather than grepped so that an "and" or "or" safely inside
// brackets -- which is most of them in this tree, in conditions and ternaries
// -- is not reported.
//
// Run: php tests/php/test_assignment_precedence.php (from the repo root)
$root = realpath(__DIR__.'/../..');
$passed = 0;
$failed = 0;
function check($label, $got, $want) {
global $passed, $failed;
if ($got === $want) {
$passed++;
echo " ok $label\n";
} else {
$failed++;
echo " FAIL $label\n";
echo " got: ".var_export($got, true)."\n";
echo " want: ".var_export($want, true)."\n";
}
}
/**
* Assignments whose right-hand side has an unbracketed "and" / "or" / "xor".
*
* Walks forward from each "=" to the statement's end, tracking bracket depth,
* and reports a logical keyword found at depth zero. Inside brackets the
* keyword cannot reach past the assignment, so it is harmless there.
*
* @param string $src php source
* @return array one "line $var = ... or" description per find
*/
function cut_short_assignments($src) {
$tokens = @token_get_all($src);
if (!$tokens) return array();
$significant = array();
foreach ($tokens as $token) {
if (is_array($token) and
in_array($token[0], array(T_WHITESPACE, T_COMMENT, T_DOC_COMMENT), true)) continue;
$significant[] = $token;
}
$found = array();
for ($i = 0; $i < count($significant); $i++) {
// A plain assignment to a variable. Compound assignments (.=, +=) have
// their own token and are not "=", so they are not considered; neither is
// "==", for the same reason.
if ($significant[$i] !== '=') continue;
$target = isset($significant[$i - 1]) ? $significant[$i - 1] : null;
if (!is_array($target) or $target[0] !== T_VARIABLE) continue;
$depth = 0;
for ($j = $i + 1; $j < count($significant); $j++) {
$token = $significant[$j];
if ($token === '(' or $token === '[' or $token === '{') {
$depth++;
continue;
}
if ($token === ')' or $token === ']' or $token === '}') {
// A closing bracket at depth zero ends the enclosing block, so the
// statement is over.
if ($depth === 0) break;
$depth--;
continue;
}
// End of statement.
if ($depth === 0 and ($token === ';' or $token === ',')) break;
if ($depth !== 0) continue;
if (is_array($token) and
in_array($token[0], array(T_LOGICAL_AND, T_LOGICAL_OR, T_LOGICAL_XOR), true)) {
$found[] = $target[2].' '.$target[1].' = ... '.strtolower($token[1]);
break;
}
}
}
return $found;
}
echo "the scan can tell the difference\n";
// Given before the tree-wide check, because a scan that matched nothing would
// pass that check on its own.
check('an assignment cut short by or is caught',
cut_short_assignments('<?php $a = f() or g();'), array('1 $a = ... or'));
check('and one cut short by and',
cut_short_assignments('<?php $a = f() and g();'), array('1 $a = ... and'));
check('the same expression with || is fine',
cut_short_assignments('<?php $a = f() || g();'), array());
check('an or inside brackets is fine',
cut_short_assignments('<?php $a = ($x or $y);'), array());
check('including in a ternary condition',
cut_short_assignments('<?php $a = ($x or $y) ? 1 : 2;'), array());
check('and in a call\'s arguments',
cut_short_assignments('<?php $a = f($x or $y);'), array());
check('an or in a condition is not an assignment',
cut_short_assignments('<?php if ($x or $y) { $a = 1; }'), array());
check('nor is one in a while',
cut_short_assignments('<?php while ($x or $y) { $a = 1; }'), array());
check('a comparison is not an assignment',
cut_short_assignments('<?php if ($a == f() or g()) { }'), array());
check('a later statement does not leak into an earlier one',
cut_short_assignments('<?php $a = 1; if ($x or $y) { }'), array());
// The line that prompted all this, as it was written.
check('the reported line is caught',
cut_short_assignments(
'<?php $drawable = ($action->Schedule() === \'\') or ($parts !== null);'),
array('1 $drawable = ... or'));
echo "nothing new under web/ cuts an assignment short\n";
// What the scan found in the rest of the tree when this test was written.
// This is a list of open questions, not a list of things that are fine.
//
// Two of them are the deliberate idiom: "$fp = fopen(...) or error_log(...)"
// relies on the loose binding on purpose, and the variable does get the fopen
// result. They are listed because the scan cannot tell intent, and because
// writing the failure branch out as an if would let them come off the list.
//
// The other four are the same bug as the one this test was written for, and
// each changes behaviour to fix, so they are left for a decision rather than
// swept up here:
//
// filter.php:400 $canSave = !$filter->Id() or $canEdit
// -- reads as "a new filter can always be saved, an
// existing one only with edit rights", but assigns only
// !Id(), so an existing filter is never savable.
// filter.php:401 $canDelete = $filter->Id() and $canEdit
// -- assigns only Id(), so delete is offered for any
// existing filter whether or not the user may edit it.
// This one is access control, so it is not mine to guess at.
// console.php:110 $show_storage_areas -- assigns only the count test, so
// the canEdit('System') half never applies.
// monitor.php:1483 $stream_available = canView('Stream') and ... or ...
// -- assigns only canView('Stream'). The intent is also
// ambiguous between A && (B || C) and (A && B) || C, which
// is a second reason not to rewrite it unasked.
//
// Anything not on this list fails, so a new one cannot be added -- including
// the one this test was written for, which is deliberately absent.
$known = array(
'web/includes/config.php:302 $cfg = ... or',
'web/skins/classic/views/_options_dnsmasq.php:107 $cfg = ... or',
'web/skins/classic/views/filter.php:400 $canSave = ... or',
'web/skins/classic/views/filter.php:401 $canDelete = ... and',
'web/skins/classic/views/console.php:110 $show_storage_areas = ... and',
'web/skins/classic/views/monitor.php:1483 $stream_available = ... and',
);
$offenders = array();
$scanned = 0;
$iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($root.'/web'));
foreach ($iterator as $file) {
if ($file->isDir() or $file->getExtension() !== 'php') continue;
$path = $file->getPathname();
// web/api is cakephp, assets are third party.
if (strpos($path, '/api/') !== false) continue;
if (strpos($path, '/assets/') !== false) continue;
$scanned++;
foreach (cut_short_assignments(file_get_contents($path)) as $find) {
$offenders[] = substr($path, strlen($root) + 1).':'.$find;
}
}
check('a useful number of files were scanned', $scanned > 100, true);
sort($offenders);
$expected = $known;
sort($expected);
check('nothing cuts an assignment short that was not already on the list',
array_values(array_diff($offenders, $expected)), array());
// The other direction, so the list shrinks as they are dealt with rather than
// sitting here forever describing lines that no longer look like that.
check('and every line on the list is still there',
array_values(array_diff($expected, $offenders)), array());
echo "the actions table's schedule reads as a sentence again\n";
// The line this was all for. Checked by name rather than by scanning, because
// its absence from the list above would pass quietly if the line were deleted
// along with the feature.
$view = file_get_contents($root.'/web/skins/classic/views/monitor.php');
check('the drawable test is a single expression',
(bool)preg_match('/\$drawable\s*=\s*\(.*?\)\s*\|\|\s*\(.*?\);/', $view), true);
check('and does not use the loose or',
(bool)preg_match('/\$drawable\s*=[^;]*\bor\b/', $view), false);
// What it has to decide, stated once: an unset schedule draws (the pickers get
// their defaults), a schedule cronToParts understands draws, and one it does
// not is shown as cron.
require_once($root.'/web/includes/CronTab.php');
foreach (array('' => true, '0 21 * * *' => true, '30 22 * * 1' => true,
'*/5 * * * *' => false, 'nonsense' => false) as $spec => $want) {
$parts = cronToParts((string)$spec);
check('"'.$spec.'" '.($want ? 'draws' : 'shows as cron'),
(((string)$spec === '') || ($parts !== null)), $want);
}
echo "\n$passed passed, $failed failed\n";
exit($failed ? 1 : 0);