Replace calls to ::expectError*() from Drupal\Tests\Core\Render\ElementTest

Hi @mondrake

Created patch for this issue, please review.

Hi @quietone
#6 Needs work is due to Tests Failure?

samit.310@gmail.com → made their first commit to this issue’s fork.

Thanks, but as explained in #3427171-6: Replace calls to ::expectWarning*() in Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest → , replacing expectError with expectException is not sufficient for PHPUnit 10 compatibility.

I think this should trigger a deprecation in 10.3.x and an exception in 11.x.

Actually given that this isn't really an error, we can just ignore and carry on, then I think an assertion is more appropriate?

longwave → changed the visibility of the branch 3427177-replace-calls-to to hidden.

Looks good to me.

Wouldn't throwing an exception be more helpful here? It feels really odd to change something from an irrecoverable error to an assertion.

Alright, let's do that then.

Initially I thought PHPUnit removing expectError/Warning methods was a bad idea, but now seeing how everyone has a different perspective on 'what' a triggered error/warning is and means and how it should be impacting runtime, I changed my mind... PHP future is likely without error handlers, then. Only exceptions.

@alexpott input was addressed.

RTBC

Yeah, I totally get it - exceptions are more manageable because they can be caught in a useful place in the call stack. You can still have a fatal error if you don't catch the exception, but I can imagine here if someone needed to they could catch this further up in the renderer stack and just not render this section of output - it gives everyone more options. Similarly, logging PHP level warnings seems dated now we have assertions, which fail harder than warnings for developers but still let us continue in production.

It's the "still let us continue in production." bit that always makes me shudder.

Committed and pushed b59e63e990 to 11.x and 3d645c1b3c to 10.3.x. Thanks!

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

Hi Folks,

I agree exceptions are more manageable (when actually managed) and making tests happy (specially when testing a specific method) is great, but this really changes the behavior (from the user perspective) throwing an uncaught exception when rendering inline templates or templates via render elements that allow so, not giving an end user enough info and thus the chance to act on an invalid passed array as value.

With the previous user_error \Drupal\Core\Render\Renderer::doRender when calling

// Get the children of the element, sorted by weight.
$children = Element::children($elements, TRUE); // Line 462 of Renderer.php

We would just get as output a bunch of errors that an user/admin could act on, but still rendering everything else on screen.

Now because the exception is not caught one basically gets a white page. From that perspective, what is your suggestion? Adding try/catch when calling this method in Renderer? And acting on that via the previous (or similar behavior) but on the caller (or callers)? Or is a failure to render the new expected behavior?

For some good reasons I agree with one of the comments shared here, just before the change was committed.

"Actually given that this isn't really an error, we can just ignore and carry on, then I think an assertion is more appropriate?"

Any suggestions would be appreciated. Thanks a lot

@diegopino maybe you can try and file an issue to change the exception to an assertion, and see your chances.

I'm afraid there's no way back to trigger_error: they are untestable under PHPUnit 10, and what's more, in PHP 8.4 we will have this Deprecate passing E_USER_ERROR to trigger_error().

quietone → closed merge request !7322

quietone → closed merge request !7286