Problem/Motivation

When adding an empty extra field in Layout Builder, the resulting page will always render the wrapping div of ExtraFieldBlock even when the extra field has empty content.

Details

ExtraFieldBlock.php's build() method will always return a placeholder render array if an extra field is present. While BlockComponentRenderArray.php's onBuildRender() methods attempts to screen out empty blocks, the extra field block will always pass because of the placeholder. This causes it to add the block for rendering ($build = ['#theme' => 'block', ...). When time comes to render the contents, since core/lib/Drupal/Core/Render/Renderer.php's doRender() method will always render arrays with a #theme present, it renders the wrapper div, even when it contains no content.

When rendering the page without using Layout Builder, the extra field just doesn't exist in the render array and no wrapper div is rendered.

Unsure whether this is expected behavior or not for Layout Builder. But I do expect that it would behave similar to how it did without using Layout Builder.

References:

Steps to reproduce

  1. Add an extra field on a node using hook_entity_extra_field_info().
  2. Add the hook_ENTITY_TYPE_view() for the extra field, but don't add anything to the $build array.
  3. Create a node type with Layout Builder enabled on Full Content.
  4. Add that field into the type's layout.
  5. Create a page of that node type.
  6. View the page.

Proposed resolution

Add a pre-render callback to ensure empty extra_field blocks are not rendered.

Remaining tasks

Update summary

User interface changes

No empty div is printed when extra field is empty.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#55 3152281-nr-bot_p0nib0ei.txt90 bytesneeds-review-queue-bot
#52 3152281-nr-bot_1zwa59ay.txt90 bytesneeds-review-queue-bot
#48 3152281-10.2-48.diff6.14 KBherved
#41 3152281-nr-bot.txt2.07 KBneeds-review-queue-bot
#29 3152281-nr-bot.txt2.28 KBneeds-review-queue-bot
#24 interdiff_11-24.txt601 byteschrisolof
#24 drupal-empty-extra-field-block-3152281-24.patch4.92 KBchrisolof
#23 interdiff_11-22.txt3.21 KBchrisolof
#22 empty_extra_field_block.patch3.72 KBpfeiffer
#11 empty_extra_field_block-3152281-11.patch4.31 KBherved
#11 empty_extra_field_block-3152281-11-test_only.patch1.86 KBherved
#5 empty_extra_field_block-3152281-5.patch2.36 KBherved

Issue fork drupal-3152281

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fskreuz created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

My suggestion here would be to create your own block plugin instead of using the extra fields API if you're moving to Layout Builder - is that possible in this case?

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

msnassar’s picture

msnassar commented

I could reproduce this using some core extra fields e.g. "Moderation control"

Steps to reproduce:
- Enable layout builder for any content type that has moderation enabled.
- Add new section
- Add "Moderation control" to the section
- Add new "published" content

The resulting page will always render the wrapping div!

herved’s picture

herved commented
StatusFileSize
new empty_extra_field_block-3152281-5.patch2.36 KB

I'm having the same issue.
I suggest to add a pre_render and apply the same emptiness logic as \Drupal\block\BlockViewBuilder::preRender

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Active » Needs review

+1 for the patch. This patch worked for me and successfully cleared the extra block from our layout.

I think @larowlan's solution makes sense, however in our case the extra fields are used in layout builder but also other display modes which don't use layout builder in the site, so it having them as extra fields simplifies the project. That being said, the block approach would likely be a simpler solution if it would work for all displays.

Version: 9.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

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.

herved’s picture

herved commented
Status: Needs work » Needs review
StatusFileSize
new empty_extra_field_block-3152281-11-test_only.patch1.86 KB
new empty_extra_field_block-3152281-11.patch4.31 KB

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.
In my case, those empty divs are really problematic when assessing the emptiness of layout builder regions which sometimes have specific styles applied on them.

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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?

herved’s picture

herved commented

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>
smustgrave’s picture

smustgrave commented
Issue tags: -Needs tests

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

herved’s picture

herved commented

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

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Cleared cache a few times and it finally took.

Thanks for baring with me on that one haha.

Status: Reviewed & tested by the community » Needs work

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

herved’s picture

herved commented
Status: Needs work » Reviewed & tested by the community

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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Blocks-Layouts

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pfeiffer’s picture

pfeiffer commented
StatusFileSize
new empty_extra_field_block.patch3.72 KB

I rerolled the patch on comment #11 to Drupal 10.2

chrisolof’s picture

chrisolof
he/him
Primary language English
commented
StatusFileSize
new interdiff_11-22.txt3.21 KB

Adding interdif.

chrisolof’s picture

chrisolof
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-empty-extra-field-block-3152281-24.patch4.92 KB
new interdiff_11-24.txt601 bytes

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

Hardik_Patel_12 made their first commit to this issue’s fork.

Hardik_Patel_12 opened merge request !6384

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3152281-nr-bot.txt2.28 KB

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.

chrisolof’s picture

chrisolof
he/him
Primary language English
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Didn’t mean to change status

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

vaish made their first commit to this issue’s fork.

vaish’s picture

vaish commented
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Needs work

Cleaned up the issue summary some.

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

mlncn’s picture

mlncn commented
Issue summary: View changes
Status: Needs work » Needs review

Updated summary.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

quietone’s picture

quietone commented

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

Leaving at RTBC.

nod_’s picture

nod_
Primary language French
Location Lille
commented

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3152281-nr-bot.txt2.07 KB

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.

shalini_jha’s picture

shalini_jha commented
Assigned: Unassigned » shalini_jha
shalini_jha’s picture

shalini_jha commented

I have checked this MR and found some conflicts, which I have addressed. The pipeline was failing due to an unknown word. For fixing this issues , reviewing the existing test method for same word, so same way I added it to the cspell:ignore , and the pipeline passed successfully. After that, I revalidated the test coverage, and it is working as expected.

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
shalini_jha’s picture

shalini_jha commented
Assigned: shalini_jha » Unassigned
Status: Needs work » Needs review

Moving this for NR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Rebase seems fine.

alexpott made their first commit to this issue’s fork.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

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

herved’s picture

herved commented
Status: Needs work » Needs review
StatusFileSize
new 3152281-10.2-48.diff6.14 KB

I rebased the MR and added test assertions to address #47.
Attaching static patch for composer that applies on 10.2.x.

herved’s picture

herved commented
Status: Needs review » Needs work

phpcs fails, some more work needed

herved’s picture

herved commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

All feedback appears to be addressed

Test-only gives

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.
/builds/issue/drupal-3152281/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3152281/vendor/behat/mink/src/WebAssert.php:492
/builds/issue/drupal-3152281/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php:492
FAILURES!
Tests: 16, Assertions: 187, Failures: 1, PHPUnit Deprecations: 17.
Exiting with EXIT_CODE=1
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3152281-nr-bot_1zwa59ay.txt90 bytes

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.

herved’s picture

herved commented
Status: Needs work » Needs review

MR rebased

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good rebase.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3152281-nr-bot_p0nib0ei.txt90 bytes

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.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring previous status.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

We generally don't add empty updates purely to clear render caches - updating core may run other updates anyway, or if this ends up being the only update that gets run, then sites can clear caches on deployment. Went ahead and removed that prior to commit.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !6384

  • catch committed 0bf6a1c0 on 11.3.x 
    fix: #3152281 Extra field blocks render even when empty

By: herved
By:...

  • catch committed f0c25113 on 11.x 
    fix: #3152281 Extra field blocks render even when empty

By: herved
By:...

Status: Fixed » Closed (fixed)

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