Skip to content

Fix repeated error suppression across error types - #24757

Open
kairosci wants to merge 5 commits into
php:masterfrom
kairosci:fix/gh-19509-ignore-repeated-errors
Open

kairosci wants to merge 5 commits into
php:masterfrom
kairosci:fix/gh-19509-ignore-repeated-errors

Conversation

@kairosci

Copy link
Copy Markdown
Contributor

Fixes #19509.

@kairosci
kairosci requested a review from bukka as a code owner October 26, 2026 20:23
@anun333

anun333 commented Oct 29, 2026

Copy link
Copy Markdown

Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.

Tested on today's master (17cfa007d5d) with this applied, debug builds, display_errors=1, counting messages shown:

master this PR
the issue's loop, E_ALL: the warning / the deprecation after it 10 / 10 1 / 1
the same, E_ALL & ~E_DEPRECATED 10 1
ignore_repeated_errors=0 (control) 10 10
one message from two lines, ignore_repeated_source=0 / 1 6 / 1 6 / 1
error_get_last() after the loop the deprecation the deprecation
a user error handler, calls 20 20
Zend/tests and tests 6,448 passed 6,449 passed, 0 failed

One design note: this keeps a last message per error type, so a repeat is also suppressed when other displayed errors come in between (the deprecation above drops to one too). The issue's report shows only warnings, so its deprecations were filtered out by error_reporting; tracking just the last error that passed error_reporting, as suggested on the issue, should cover that case without widening what counts as repeated. Which of the two is wanted is a call for the reviewer.

How this was tested

Linux x86_64, gcc 13.3, debug builds of origin/master and of it with this PR's commit cherry-picked:

./buildconf --force
./configure --enable-debug --disable-cgi --disable-phpdbg --disable-fileinfo --enable-pcntl --enable-zend-test --enable-fpm --with-curl --enable-mbstring --disable-mbregex
make -j4

Each row: sapi/cli/php -n -d display_errors=1 -d html_errors=0 -d ignore_repeated_errors=1 -d error_reporting=… -r 'for ($i = 0; $i < 10; $i++) { trim($at); }', counting "Undefined variable" and "Deprecated" lines; the source rows use echo $a; on two lines, three times. Suites: TEST_PHP_EXECUTABLE=$PWD/sapi/cli/php sapi/cli/php run-tests.php -n -q -j4 Zend/tests tests.

@kairosci
kairosci force-pushed the fix/gh-19509-ignore-repeated-errors branch from e470a30 to ec720cd Compare November 1, 2026 13:27
@kairosci
kairosci requested a review from derickr as a code owner November 1, 2026 13:27
@kairosci
kairosci force-pushed the fix/gh-19509-ignore-repeated-errors branch from ec720cd to 5196728 Compare November 1, 2026 13:28
@eyupcanakman

Copy link
Copy Markdown
Contributor

I built this and its parent (e92d9c4) to compare. The quoted part was written with an LLM, per CONTRIBUTING.

It fixes the reported loop. Three other cases change, and no existing test catches them. Each case runs as its own script with -d ignore_repeated_errors=1.

function a() { trigger_error("A", E_USER_WARNING); }
function n() { trigger_error("N", E_USER_NOTICE); }

// 1. master shows A twice, this PR once
a(); error_clear_last(); a();

// 2. master shows all 6, this PR shows A and N once each
for ($i = 0; $i < 3; $i++) { a(); n(); }

// 3. master prints "A", this PR prints "D"
error_reporting(E_ALL & ~E_USER_DEPRECATED);
a(); trigger_error("D", E_USER_DEPRECATED); a();
echo error_get_last()['message'];

The first two come from the per-type slots. error_clear_last() doesn't reset them, and today a visible error only counts as a repeat when it comes right after the same one.
In the third, the second a() is hidden, so error_get_last() still returns the deprecation.

@kairosci
kairosci force-pushed the fix/gh-19509-ignore-repeated-errors branch from 5196728 to e2cb0af Compare November 2, 2026 21:18
@kairosci

Copy link
Copy Markdown
Contributor Author

Written with ChatGPT. This response summarizes the changes and verification in the current PR head.

Thanks for checking these edge cases. I changed the implementation to track only the last error that passed error_reporting, rather than keeping a separate repeat slot per error type. error_clear_last() now clears both the last-error value and the repeat-suppression state.

The new tests cover the cases you identified: repeated A after error_clear_last(), interleaved A/N errors, and a filtered deprecation between repeated visible warnings. The full CI matrix is green on the current head. Please take another look.

@eyupcanakman

Copy link
Copy Markdown
Contributor

Re-ran the three cases from my earlier comment against ff1249b. The quoted part was written with an LLM, per CONTRIBUTING.

All three are fixed. error_clear_last() re-arms, the A/N loop shows all 6 lines, and case 3 prints "A". Zend/tests and ext/standard/tests/general_functions pass.

One side effect is left. It needs ignore_repeated_source=1 on top of ignore_repeated_errors=1. A hidden repeat from another line then replaces the last error.

trigger_error("S", E_USER_WARNING);
trigger_error("S", E_USER_NOTICE); // hidden as a repeat
var_dump(error_get_last());

Master reports the warning (type 512, first line). This PR reports the notice (type 1024, second line), which was never shown or logged. The new last-error check in php_error_cb() compares file and line even when ignore_repeated_source is on. I can't tell if that is intended, and no test covers it.

Honor ignore_repeated_source when deciding whether a suppressed repeat
replaces error_get_last(), matching the repeat-suppression comparison.
Keep restoring the last error after an intervening filtered diagnostic.

Add coverage for different lines and files, filtered errors,
error_clear_last(), and enabling source comparison again.

Fixes the regression reported in phpGH-24757 while addressing phpGH-19509.
@eyupcanakman

Copy link
Copy Markdown
Contributor

b2742cb fixes the case from my last comment. The quoted part was written with an LLM, per CONTRIBUTING.

With both settings on, error_get_last() keeps the warning there, same as master. The earlier cases still pass, and so do Zend/tests and ext/standard/tests/general_functions.

I also ran random scripts against master. Output is the same whenever no error is hidden by @ or error_reporting. With hidden errors, what gets shown changes as intended. One error_get_last() change stood out, and it only needs ignore_repeated_errors=1.

function f($t) { @trigger_error("S", $t); }
f(E_USER_WARNING);
f(E_USER_NOTICE);
var_dump(error_get_last()["type"]);

Master gives 512, this PR gives 1024. error_reporting(0) in place of @ does the same. The repeat state now only follows errors that pass error_reporting, so the second hidden error is no longer a repeat of the first. 1024 is the more recent error, so the new value makes sense to me, but no test covers it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ignore_repeated_errors ini entry does not work

3 participants