PHPUnit 13.2 breaks Drupal's approach for ignoring deprecations

[mondrake]

Q	A
PHPUnit version	13.2.0
PHP version	8.5
Installation Method	Composer

Summary

PHPUnit 13.2.0 breaks Drupal deprecation ignore process, that is based on the assumption that an error handler set in the bootstrap PHP file is the one active during the testing.

What we are doing in Drupal is we set in bootstrap the \Drupal\TestTools\ErrorHandler\BootstrapErrorHandler which itself falls back to PHPUnit's own one. So we can exclude some deprecations to be ignored (we ignore via a file containing a set of patterns to be ignored) and return to test execution without even letting PHPUnit know that a deprecation was triggered.

13.2 reverses that - it always sets its own error handler as the main one, and only falls back to \Drupal\TestTools\ErrorHandler\BootstrapErrorHandler after the error (including deprecation ones) have been already collected. So deprecations get through to PHPUnit and are reported.

Current behavior

How to reproduce

Expected behavior

[sebastianbergmann]

This change is intentional, and I consider the new behaviour correct; PHPUnit should arguably have always worked this way.

The relevant background is #5845: error handlers registered before PHPUnit runs (for example via auto_prepend_file, a PHP extension, or a bootstrap file) used to make the number of risky tests depend on whether such a handler was present. Fixing that required PHPUnit to take clear ownership of the error-handler stack during a test run: it now snapshots the stack, knows where its own handler sits on it, restores the stack to a known state around every test, and processes every issue itself before forwarding it to whatever handler was registered previously.

A consequence is that a handler registered in the bootstrap, such as Drupal's BootstrapErrorHandler, which wraps PHPUnit's handler and silently drops some deprecations before delegating the rest, can no longer hide deprecations from PHPUnit. PHPUnit observes, classifies (self/direct/indirect), baselines, de-duplicates and reports the deprecation first, and only then forwards it.

My reasoning is this: a deprecation triggered while PHPUnit runs your tests is an event in the test run, and deciding whether it is reported, ignored, or fails the suite is PHPUnit's responsibility. That decision needs to be made consistently and in one place so that the baseline, --display-deprecations, failOnDeprecation, deduplication, and the issue-trigger classification all agree. A custom error handler that suppresses issues before PHPUnit sees them moves that decision outside of PHPUnit and makes those features disagree with each other.

PHPUnit already provides first-class, supported mechanisms for "ignore these deprecations", which I'd encourage using instead of a wrapping error handler.

[sebastianbergmann]

I looked into whether a configuration option could restore the pre-13.2 behaviour, and what that would cost. I'm laying this out neutrally rather than arguing for or against it.

What "previous behaviour" means technically

The behaviour to restore is: let an error handler registered in the bootstrap intercept issues and suppress some of them before PHPUnit processes (observes / baselines / emits) them; i.e. give a user-supplied handler precedence over PHPUnit's own handler during the test run.

Approach A: Precedence flip behind a flag (smallest change)

In ErrorHandler::__invoke() and ErrorHandler::handleNonTestCaseIssue(), when the flag is set, call the previously registered handler first; if it returns true (the issue was handled/suppressed), return early without emitting any event. Only when the previous handler declines does PHPUnit run its normal emit path.

• Pros: localized to ErrorHandler; no change to the stack-ownership machinery from Error handlers registered before PHPUnit (e.g. via auto_prepend_file) cause false "risky test" warnings #5845; preserves the "PHPUnit on top" invariant so risky-test detection keeps working.
• Cons: any deprecation the bootstrap handler suppresses becomes invisible to the baseline, to deduplication, to issue-trigger classification, to --display-deprecations, failOnDeprecation, and to every logger/extension subscribed to the issue events. Reporting can silently disagree with reality, which is exactly the problem Error handlers registered before PHPUnit (e.g. via auto_prepend_file) cause false "risky test" warnings #5845 and PHPUnit 13.2 breaks Drupal's approach for ignoring deprecations #6705 set out to remove.

Approach B: Full opt-out (don't install PHPUnit's handler when a bootstrap handler exists)

Effectively revert to pre-13.2: skip the forward-to-previous ownership and let the bootstrap handler be authoritative.

• Pros: exactly restores the old behaviour for consumers like Drupal.
• Cons: reintroduces the Error handlers registered before PHPUnit (e.g. via auto_prepend_file) cause false "risky test" warnings #5845 bug (risky-test counts depend on pre-registered handlers); largest behavioural divergence; two code paths to maintain indefinitely.

Cross-cutting trade-offs

The whole point of #5845 was for PHPUnit to own the handler stack; a flag that hands authority back to a bootstrap handler partly re-opens what #5845 closed.

Any "suppress before PHPUnit" option means the baseline, deduplication, trigger classification, failOnDeprecation, and issue loggers can no longer be trusted to reflect what actually happened during the run.

A permanent compatibility flag is a long-lived second code path and a recurring source of "why does X disagree with Y" questions.

[sebastianbergmann]

If I do decide to do something about this, it would most likely be approach A (see #6705 (comment)).

When this new flag is set, PHPUnit's error handler calls the previously registered handler (the one registered in your bootstrap) first. If that handler reports the issue as handled, PHPUnit returns early and does not process, baseline, or report it. Only issues that the previous handler does not claim are processed by PHPUnit as usual. This restores the pre-13.2 precedence: your handler decides first, PHPUnit only sees what you let through, and would let Drupal's BootstrapErrorHandler work as it did before.

There is a subtlety to get right here, though: a handler like Drupal's does not merely return true/false, it delegates the deprecations it does not ignore back into PHPUnit's handler. So the implementation needs a re-entrancy guard, otherwise PHPUnit would call your handler, your handler would call PHPUnit, and PHPUnit would call your handler again. With the guard in place: ignored deprecations are suppressed before PHPUnit ever records them, and non-ignored ones are emitted exactly once.

The important consequence, and the reason this would only ever be opt-in, is that anything your handler suppresses becomes invisible to PHPUnit: the baseline, deduplication, trigger classification, --display-deprecations, failOnDeprecation, and every logger or extension subscribed to the issue events. PHPUnit can no longer guarantee that its reporting reflects what actually happened during the run.

So if such a flag is ever added, using it would cause PHPUnit to emit a test runner warning on every run, stating loud and clear that reporting integrity cannot be guaranteed because issue handling has been delegated to a handler outside of PHPUnit's control. The escape hatch would exist, but it would never be silent.

No promises that this will be implemented, but that is the shape it would take.

[mondrake]

Thank you for the detailed feedback.

I understand and fully buy the intent.

A project like Drupal, that provides a framework that can be extended by multiple independent projects, takes a long (sometimes very long) time to deal with deprecations, and with change in general. As an example, PHPStan in Drupal still lingers on level 1 after years since adoption.

For this purpose, a mechanism to 'ignore' selected deprecations by defining a set of patterns in a file was developed and contributed to Symfony's PHPUnit-Bridge. However, this feature did not make its way in PHPUnit when it developed its own deprecation management logic, #5841. For this reason, in Drupal 11 we introduced an own implementation.

Right now, Drupal's deprecation handler only addresses this task - using an ignore file like this

# This file contains patterns to be ignored while testing for use of
# deprecated code.
# See https://www.drupal.org/node/3285162 for more details.

# Cross-module return type deprecations within Drupal core. Downstream
# implementations should add return types first, then we can remove this.
%Method "Drupal\\[^"]+::[^"]+\(\)" might add "[^"]+" as a native return type declaration in the future. Do the same in (?:implementation|child class) "Drupal\\(Core|basic_auth|big_pipe|block|block_content|breakpoint|ckeditor5|comment|config|config_translation|contact|content_moderation|content_translation|contextual|datetime|datetime_range|dblog|dynamic_page_cache|editor|field|field_ui|file|filter|help|image|jsonapi|language|layout_builder|link|locale|media|media_library|menu_link_content|menu_ui|migrate|migrate_drupal|mysql|mysqli|navigation|node|options|package_manager|path|path_alias|pgsql|responsive_image|rest|search|serialization|shortcut|sqlite|system|taxonomy|telephone|text|toolbar|update|user|views|views_ui|workflows|workspaces|workspaces_ui)\\[^"]+" now to avoid errors or add an explicit @return annotation to suppress this message%
# Test modules that we skip for now, but can be fixed any time.
%Method "Drupal\\[^"]+::[^"]+\(\)" might add "[^"]+" as a native return type declaration in the future. Do the same in (?:implementation|child class) "Drupal\\(Tests\\(Core|block|block_content|comment|editor|field|file|filter|image|language|layout_builder|media|menu_link_content|node|path_alias|responsive_image|search|shortcut|system|taxonomy|user|views|workflows|workspaces)|FunctionalTests\\Rest|test_.*|.*_test|.*_test_.*|ckeditor5_plugin_elements_subset|dummydb|search_embedded_form|search_extra_type)\\[^"]+" now to avoid errors or add an explicit @return annotation to suppress this message%

# Internal code that we cannot avoid extending.
%The "Twig\\Environment::getTemplateClass\(\)" method is considered internal\. It may change without further notice\. You should not extend it from "Drupal\\Core\\Template\\TwigEnvironment"\.%

# Skip some dependencies' DebugClassLoader forward compatibility warnings, in
# order to let contrib modules make their necessary fixes first.
#
# (none at the moment)

# Indirect deprecations. These are not in Drupal's remit to fix, but it is
# worth keeping track of dependencies' issues.
%Method "Iterator::[^"]+" might add "void" as a native return type declaration in the future. Do the same in implementation "org\\bovigo\\vfs\\vfsStreamContainerIterator" now to avoid errors or add an explicit @return annotation to suppress this message%

# Drupal 13.
%The "cache.backend.memory" service is deprecated in drupal:11.3.0 and is removed from drupal:13.0.0. Use cache.backend.memory.memory instead. See https://www.drupal.org/node/3546856%
%The "cache.static" service is deprecated in drupal:11.3.0 and is removed from drupal:13.0.0. Use the cache.memory bin instead. See https://www.drupal.org/node/3546856%

with only 6 lines (the other are comments), we prevent approximately 3.5 to 4 million deprecation errors being reported. Trying to address such numbers with baseline and/or individual ignores would be ineffective for us.

If we could find a way to build on the latest changes so to also have a way to indicate 'project level ignored deprecations' on top of 'baseline ignored deprecations' and 'test level ignored deprecations', we could potentially even get rid of the current DeprecationHandler in Drupal.

[sebastianbergmann]

@mondrake Can you please test whether #6708 would be suitable for you (at least as a short-term solution)? Thank you.

[mondrake]

Thank you for the very fast turnaround. Looking into it.

If I understand correctly I will have to enable the XML flag and change the BootstrapErrorHandler implementation to NOT instantiate PHPUnit's ErrorHandler and fallback to it, but only return TRUE or FALSE depending on whether processing should continue in ErrorHandler.

Will report back.

[mondrake]

Yes, #6708 works, thank you. These are the changes made in Drupal on top of applying the PHPUnit patch: https://git.drupalcode.org/project/drupal/-/merge_requests/14840/diffs?commit_id=42a9e76fb1e4c6aa11572d1379afbf79e7d77052

While I understand the logic behind adding a notice in case the error handler is deferred, that honestly is not desirable from my perspective:
a) it forces us to disable failOnPhpunitNotice, which is a step back vs the current situation (we are strict here)
b) no tests can ever be green on PHPUnit CLI, as even if not displayed/failing the notice is still reported and displaying a yellow summary (all tests we run in CI have the error handler overridden in the bootstrap PHP file)
c) it adds no actionable information - we know we are doing that, but have no other options

Is there any PHPUnit issue level below notice, like e.g. info?