Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Spin off from #3417650: [meta] Replace calls to ::expectError*() and ::expectWarning*().
PHPUnit 9 deprecated ::expectError*() methods. They're removed from PHPUnit 10.
Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl is triggering an E_USER_ERROR in some circumstances, and a test exists expecting that.
That test needs to remove the expectation, and the runtime code need to find a different strategy to raise the error if we want to keep it testable.
Proposed resolution
Replace trigger_error() with logging the error to the logger service.
Merge request link
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3419693
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3419693-find-an-alternative
changes, plain diff MR !6491
1 hidden branch
Comments
Comment #2
mondrakeIn this case, the user is anyway informed about the issue via the 400 response with the message. The point is whether we want to track the same message for site admin. If we use
trigger_error(), the error will only be logged if the setting allows, which is not recommended in prod sites. So it'd be better log it directly via a service, if the container is available?Comment #4
mondrakeComment #5
smustgrave CreditAttribution: smustgrave at Mobomo commentedTook a look but the CR link throws a 404.
Comment #6
mondrakeBecause it's just a placeholder ATM.
Comment #7
mondrakeComment #8
mondrakeAdded CR and reference in code.
Comment #9
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks @mondrake!
Comment #10
alexpottThis feels so wrong. Our test infrastructure should not be stopping us from using trigger_error. There has to be a better way.
Comment #11
mondrake@alexpott re #10 we could backfill the
expect[Error|Warning]*()methods, but current position is NOT to do that and abide by PHPUnit 10 approach that simply makestrigger_error()untestable.Discussion in https://drupal.slack.com/archives/C1BMUQ9U6/p1707946428463489
Comment #12
alexpottDiscussed a bit with @catch.
I'm not a big fan of PHPUnit making us change run-time code - it feels the wrong way around. But in this specific case I think the changes are an improvement - and maybe that will turn out to be the case with all trigger_error stuff.
Comment #13
alexpottOne point that @catch makes is that logger channels are not lazy. So we have to instantiate services in order to inject something that is rarely used.
Comment #14
catchYes the reason to not inject loggers everywhere is because if they're logging errors it's for a 1/10000 event, and they have dependencies that also then need to be instantiated like the database layer.
Discussed this with @alexpott in slack and he mentioned service closures. There's a good example of that being used in #3420215: Remove ContainerAwareTrait from session middleware, so I think we can do that.
I'm personally not convinced that completely dropping expectError is going to help us, because phpunit has also dropped support for what symfony-phpunit bridge does for @trigger_error() too - so will we do a lot of work and still be stuck? Might need a meta issue. However, that doesn't mean we can't go ahead here as long as the service is lazily instantiated.
Comment #15
mondrake@catch #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency is taking care of replacing the bridge deprecation reporting features that will not be ported to PHPUnit 10. That is (mostly) working already. In there, we could easily backfill
expect[Error|Warning]*()methods, and in fact earlier versions of the MR had that in place; lately though, that part was dropped based on the Slack discussion in #11.Comment #16
mondrake@alexpott @catch @longwave mind moving the discussion on what to do with expect[Error|Warning]*() to #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency?
Comment #17
mondrakeI'll give a try to service closures. On it.
Comment #18
mondrakeHere we go.
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears all feedback has been addressed so going to remark
Comment #20
alexpottThis fix is so elegant - we should make more use of service enclosures for logging.
Comment #21
alexpottCommitted and pushed 25697a17e8 to 11.x and 5959b69d70 to 10.3.x. Thanks!