Log error instead of trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl

In 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?

Took a look but the CR link throws a 404.

Because it's just a plaholder ATM.

Added CR and reference in code.

Thanks @mondrake!

This feels so wrong. Our test infrastructure should not be stopping us from using trigger_error. There has to be a better way.

@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 makes trigger_error() untestable.

Discussion in https://drupal.slack.com/archives/C1BMUQ9U6/p1707946428463489

Discussed 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.

One 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.

Yes 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 📌 Remove ContainerAwareTrait from session middleware Fixed , 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.

@catch 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed 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.

@alexpott @catch @longwave mind moving the discussion on what to do with expect[Error|Warning]*() to 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed ?

I'll give a try to service closures. On it.

Here we go.

Appears all feedback has been addressed so going to remark

This fix is so elegant - we should make more use of service enclosures for logging.

Committed and pushed 25697a17e8 to 11.x and 5959b69d70 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

mondrake → closed merge request !6491