The known-issues list recorded each finding as path:line, and a rebase onto a
master carrying 1677470af1 inserted four lines above one of them in
monitor.php. The finding moved from 1483 to 1487, so it read as both a new
offender and a missing list entry, and the test failed on code nobody had
touched.
A line number is not a stable identity for a finding. Key on the file and the
variable being assigned instead, which survives anything inserted above and
still names the offender uniquely -- there is no file here assigning the same
variable twice with a cut-short expression. The line is still reported, since
it is what someone needs in order to go and look, but it is not what gets
compared.
Verified the test still catches a new instance, and that shifting every line
in a listed file no longer breaks it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
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
