[meta] Replace calls to ::expectError*() and ::expectWarning*()

Looks nice, moreover in context of PHP 8.3 and assert requirements in php.ini

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Dizzy bot? The MR is mergeable.

Tagged with no-needs-review-bot to tell bot to f.....ind something else to do.

Change looks fine, clearly didn't break anything. But not sure a good way to search for others. Any suggestions?

#11 what we need here is to stop using TestCase::expectError*() methods. If you don’t find others, it should be ok.

Searching for expectError and all instances are replaced.

Added some review questions. These are all kind of awkward things where we don't have a logger available and can't necessarily throw an exception, but something bad has happened and we should be able to inform the site owner.

Yeah the problem in some of the cases here is notifying ‘somehow’ that an error occurred. So I think we have z options:

1. We keep throwing user errors, and we abide to PHPUnit 10 vision that is no longer going to test on such errors by expecting them. We do not implement a gap fill to expect errors. Therefore we drop any test that expects an error.
2. We keep throwing user errors, and we fill the gap vs PHPUnit 10 using our own error handler to test error expectations. We keep the tests as they are now. This is currently done in 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed .
3. We stop throwing user errors, and again abide to PHPUnit 10. We replace the user error, where we expect the user to be informed, with a log call like \Drupal::service(‘logger something’)->error(‘whatever’). Assuming the container is available.
4. ?

Each of the cases here will probably need to have its own solution.

Thoughts?

Appears to have a unit test failure.

#16 yes, but I’d be more interested on reviews and input on #15 ATM.

IMHO, I would prefer NOT to reimplement expectError*() methods.

Between the lines, I read PHPUnit 10 approach as one where using trigger_error in SUT code is no longer seen as a good practice. We might debate whether this is right or not, but that’s OOS here.

So what is the practical effect of the PHPUnit 10 change is that triggering E_USER_ERROR in SUT is not expectable in test and therefore FAILS the test itself. In other words, triggering an error should not be in any covered code path. That’s a bold assumption, with its own meaning i.e. that you should develop code where triggering errors is really exceptional. If we backfill the expectation we’re swallowing the assumption.

Possibly we could take a different stance for expectWarning* but that’s for another issue.

I guess we have to ask who is the error intended for, and how should we be notifying these users.

We already have multiple ways of notifying different types of users: the messenger service (if the error is intended for UI), the logger services (if the error is intended for a site administrator), and exceptions (if the error is intended for a developer or another piece of code).

We also have assertions which are usually used when we would have an exception for a developer but at runtime we should carry on.

But what is the intended audience for trigger_error errors and warnings? Are these really yet another category on top of the ones listed above, or should we migrate these to one of those types?

Maybe we need another meta like the withConsecutive one to take each case and decide how to refactor it?

#20 fine, but IMHO we need first to establish whether we will have expectError*() methods as our own stuff, or not. I am for NO.

#20 fine, but IMHO we need first to establish whether we will have expectError*() methods as our own stuff, or not. I am for NO.

Isn't @longwave in #19 📌 [meta] Replace calls to ::expectError*() and ::expectWarning*() Active with

But what is the intended audience for trigger_error errors and warnings? Are these really yet another category on top of the ones listed above, or should we migrate these to one of those types?

kinda implying the same?

At least that's the way I read it.

FWIW: I'm on #TeamNo on shimming expectError*().

OK, not sure about a meta, for the moment I created two spin-offs, 📌 Find an alternative to trigger_error in Drupal\Core\Database\Query\Condition::compile Active and 📌 Find an alternative to trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl Active , that are tackling a couple of cases that were reverted from the MR here.

I think we still need this MR to fix the clear bugs (for example, Statement objects trigger E_USER_WARNING but the test expects errors), and to cleanup the deprecation and warning ignores.

Similar to the conclusion of withConsecutive() I also lean towards "no", but let's also summon the framework managers to see if they have opinions on the use of trigger_error().

#24 thanks. For completeness: let's differentiate between triggering E_USER_ERROR and triggering E_USER_WARNING, though. PHPUnit 10 drops expect* methods for both. I'm on a NO to reintroduce expectError*() methods as our own implementation; more open to keep expectWarning*()methods.

See 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed for the current status of reimplementing deprecation, warning and error expectations, that were dropped in PHPUnit 10 or for which Symfony's PHPUnit-bridge has not come up with a solution yet. Decisions here will impact how much fill-gap code we will have to write and maintain.

Adjusted IS with latest direction discussed in Slack.

mondrake → changed the visibility of the branch 3417650-replace-calls-to to hidden.

All of the sub-issues are done here, and it's noted in the change record. Unfortunately not one way to fix this since it has mostly meant refactoring code.

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