"A #lazy_builder callback's context may only contain scalar values or NULL" assertion should say which parameter is the problem

Didn't review but MR should be against 11.x vs 11.0.x

As the author should be able to update the target, probably will require a rebase.

Hello!

I changed the base of the MR from 11.0.x to 11.x.

Please review the changes and let me know if additional work is required.

Kind regards!

So asked @larowlan about this and he mentioned this change now introduces run-time code, while the code in HEAD is optimized by PHP when asserts are off (as they should be in production)

Not sure a path forward here

So it's possible this is a works as designed

Not disagreeing it's difficult to read, but if it's to change think we need make sure it's not a performance hit.

Ah that is a point I'd not thought of!

I wonder if there's a way we can get a list of failing keys into the assert message.

We could do a dummy outer assert like this:

assert(array_walk($elements['#lazy_builder'][1], function ($v, $key) {
  assert(is_null($v) || is_scalar($v), "message can now give the $key);
}, 'This message never gets output);

Hello again!

I modified the code to utilize array_walk function as suggested for a clearer error message.

Please check the changes when you have a minute, and let me know if additional work is required.

Kind regards!

For good practice can the issue summary be updated to include proposed solution/how this makes things better. Again for practice

Done.

Thanks @joachim!

I read the IS, comments and MR. Everything looks fine here expect that I did not find an answer to the question in #8, about performance. Also, there should be manual testing of this change.

Back to NW for two small tasks.

Performance is unchanged, as it's still a single assert().

The potential performance change was my initial suggestion to put a foreach() loop with the assert() inside.

RendererPlaceholdersTest::testNonScalarLazyBuilderCallbackContext() should be expanded to cover the updated message, which will also alleviate the need for manual testing.

I am working on this issue

We are working on this issue.

Goal is to update the run the test on RendererPlaceholdersTest::testNonScalarLazyBuilderCallbackContext and ensure it passes with the appropriate messages

We worked on this issues as part of DrupalCon. We updated the exception message to be more specific. Please could review our change.

Looks good.

I'd nitpick over 'Utilise' instead of the most simple 'Use', but a) not worth changing and b) I can't remember which bits of this MR I worked but it's possible it's me who put that in :)

I can't RTBC as it was me who suggested the dummy outer assert.

The GitLab CI pipeline is failing due to a test failure.

There was 1 failure:

1)
Drupal\Tests\layout_builder\FunctionalJavascript\BlockFilterTest::testBlockFilter
Failed asserting that actual size 1 matches expected size 0.

/builds/issue/drupal-3455746/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php:134

FAILURES!
Tests: 1, Assertions: 26, Failures: 1.

Is this a regression caused by the changes to the assertion in the earlier commits?