Extra field blocks render even when empty

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

For this issue to move forward next it will need a test case showing the problem.

Thanks.

Here is a minimal test which should highlight clearly the issue.
Test only patch should fail of course, and the other one includes the pre_render from #5.

About #2 and #10: Extra fields are used in many places now. Modules like https://www.drupal.org/project/extra_field → rely on it.
If the render array is NULL or only contains #cache, it is only logical to hide the container div completely. Those empty divs can be really problematic when asserting the emptiness of paragraphs for example.

The last submitted patch, 11: empty_extra_field_block-3152281-11-test_only.patch, failed testing. View results →

So tried replicating

On Drupal 10.1 with standard install
Enabled layout builder for Basic page
Added section
Added Moderated Control block
Create a basic page node (published)
The section markup appears with and without the patch
I do not see an empty div for blocks with or without the patch

Am I missing a step?

Are you sure?
I just tried with 9.5.x and 10.1.x and was able to reproduce in both cases.
Indeed the section markup still shows up but this issue only resolves the extra field markup.
For sections I think this is up to layouts to check for emptiness and not render if that's the case.

The markup for a published node is as follows:

<div class="layout layout--onecol">
  <div class="layout__region layout__region--content">
    <!-- START: this is what the patch removes -->
    <div class="block block-layout-builder block-extra-field-blocknodearticlecontent-moderation-control">
      <div class="block__content">
      </div>
    </div>
    <!-- END -->
  </div>
</div>

So tried retesting and seems like this does work but doesn't fix existing pages. Should we have an upgrade path for those?

There is no upgrade path needed, only a clear cache is required.
Did you clear caches after applying the patch?
Thanks

Cleared cache a few times and it finally took.

Thanks for baring with me on that one haha.

The last submitted patch, 11: empty_extra_field_block-3152281-11.patch, failed testing. View results →

Setting back to RTBC.

It looks like the testbot had some issues a few days ago which seem completely unrelated to this patch... after re-queuing the patch the errors disappeared.
https://www.drupal.org/pift-ci-job/2604971 → for reference.

An empty post_update hook will force the caches to be cleared, it's worth adding that IMO

I rerolled the patch on comment #11 to Drupal 10.2

Adding interdif.

I tried applying the patch from #22, but ran into errors due to the removal of Drupal\Core\Security\TrustedCallbackInterface. I think the removal of TrustedCallbackInterface was likely unintentional.

Attached is a re-roll of the patch in #11 against 10.2.x, with the requested post_update hook added so a cache clear is forced.

Manually triggered #24 bur appears to have a failure. Didn't do a full review but recommend switching to MR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Left a nitpicky but a comment on the tests if possible to also get a positive assertion.

Didn’t mean to change status

Been a few days so moving to NW for the small comment.

I resolved the merge conflict and addressed the nitpicks. Moving to needs review.

Cleaned up the issue summary some.

Appears close but moving to NW to update the missing pieces of the summary, left TBD in there.

Updated summary.

Feedback appears to be addressed.

Triaging the RTBC queue. I didn't find any unanswered questions and the comments in the MR are clear.

Leaving at RTBC.

Code seems reasonable, not a subject I'm comfortable committing though. RTBC +1

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

I have checked this MR and found some conflicts, which I have addressed. The pipeline was failing due to an unknown word. After reviewing the existing test method, I added it to the cspell:ignore list, and the pipeline passed successfully. After that, I revalidated the test coverage, and it is working as expected. Let me know if you need any further adjustments!
Kindly review.

Failing test :

There was 1 failure:

1) Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testExtraFields
Behat\Mink\Exception\ExpectationException: An element matching css ".block-extra-field-blocknodebundle-with-section-fieldlayout-builder-test-empty" appears on this page, but it should not.

/var/www/html/vendor/behat/mink/src/WebAssert.php:888
/var/www/html/vendor/behat/mink/src/WebAssert.php:492
/var/www/html/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php:505

Moving this for NR.

Rebase seems fine.

I think we need to add a positive test in some way - at least the test fails without the fix but this is very fragile as we're depending on the CSS class names for the test - which are not API - therefore I think we need to go to the effort of a positive test case somehow - suggestion left on MR