[DX]: throw an exception if #lazy_builder callback does not return a (renderable) array

Hii,

This is Pooja Ganjage, I am commenting over the issue,
There is an error syntax in code, put closed parenthesis of if statement, this is below correct code.
if(!is_array($new_elements)) {
throw new \LogicException("#lazy_builder callbacks must return a valid renderable array, got $value from $callback.");
}

Thanks

Looks like a fine DX improvement to me! 👍🥳🙏

Hi,

I have implemented the code changes as per above suggestion.

I have also attached the screenshot showing the exception, if callback does not return a renderable array.

Looks better to check special private method !isString() because PHP 8 bringing interface https://github.com/php/php-src/pull/5083

Proper branch, other naming could be validateResponse()

updated the comment and rolled the patch

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -353,6 +353,12 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        throw new \LogicException("#lazy_builder callbacks must return a valid renderable array, got $wrong_type from " . $elements['#lazy_builder'][0]);
   

   Do we need to pass this through XSS filter?

2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
   @@ -962,6 +962,19 @@ public function testCreatePlaceholderPropertyWithoutLazyBuilder() {
   +   * @covers ::render
   

   Nit: We should have a full sentence for a method in addition to the annotations.

3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
   @@ -314,11 +314,21 @@ public static function callbackTagCurrentTemperature($animal) {
   +   * #lazy_builder callback; returns invalid renderable.
   

   Nit: Reword this sentence.

   "Returns invalid renderable to #lazy_builder callback."

Fixing these up is relatively straightforward so I added the Novice tag.

LGTM!

One minor nit otherwise this looks pretty helpful.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -353,6 +353,23 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+        throw new \LogicException("#lazy_builder callbacks must return a valid renderable array, got $wrong_type from " . Xss::filter($callable_name));

Exception text is filtered if it is output to a browser. See \Drupal\Core\EventSubscriber\FinalExceptionSubscriber::onException().

This looks helpful, however I'm wondering what happens if some buggy code is already on production (i.e. maybe it only returns FALSE occasionally) - this will convert a PHP error to an exception. Maybe that is OK but maybe it's disruptive.

One possible option would be to trigger_error('....', E_USER_WARNING) the helpful error message, and change to an exception in Drupal 10, but I'm not sure if that's being overly careful for what is already a bug.

@catch isn't the fact we hit a type error mean that we're replacing a fatal with an exception - so there's no really any disruption. In both cases execution is going to stop and you're going to a get 500?

Ah good point if it's already a fatal error then #29 is moot, one fatal to a better one is fine.

Committed and pushed e9a53cb2b3 to 9.3.x and eebe864522 to 9.2.x. Thanks!

After reviewing #3212049: Renderer::renderRoot() can cause fatal errors when trying to describe object-based callables I decided to revert this. As @phenaproxima points out - there is a callback type that this would error on. But also in reviewing the code I realised that we ought to be using an assert here instead.

The code in Renderer should look something like this:

assert(is_array($new_elements), sprintf("#lazy_builder callbacks must return a valid renderable array, got %s from %s", gettype($new_elements), Variable::callbackToString($elements['#lazy_builder'][0]));

Alright; gave it a shot. I really like the change that this issue makes, so hopefully we can get this back in soon :)

@phenaproxima that's looking very nice. I think we should only add one test to RenderPlaceholdersTest - since now we should be adding a test for Variable::callableToString() to \Drupal\Tests\Component\Utility\VariableTest

Fixed!

First point is not important. The second one is. After that I think this is ready to go

Credited @tim.plunkett for review.

I think this is ready for another look.

Okay I think that addresses all my concerns. Thanks @phenaproxima!

Committed and pushed a3c114e65f to 9.3.x and 9b94f12237 to 9.2.x. Thanks!