Problem/Motivation

Entity forms move the "Published" checkbox into the core-provided footer group with '#group' => 'footer'. default_admin copies that footer into its sidebar by value in FormHooks::stickyActionButtonsAndSidebar():

$form['default_admin_sidebar']['footer'] = ($form['footer']) ?? [];

A group is identified by its #parents, not its array key (RenderElementBase::processGroup()). On an untreed form the copy keeps ['footer'] and still matches. On a form that sets $form['#tree'] = TRUE it becomes ['default_admin_sidebar', 'footer'], so '#group' => 'footer' resolves to the original $form['footer'] instead, which node-edit-form.html.twig excludes from output.

The checkbox is then never rendered, but still built and still processed on submit. An unrendered checkbox is not posted, so Form API reads 0 and the entity is unpublished. No checkbox, no warning, nothing in the save message.

Node forms are unaffected only because NodeForm does not set #tree.

Steps to reproduce

  1. Drupal 11.4.5 with default_admin as admin theme, plus Commerce 3.3.8.
  2. Create a store, a product type with the status checkbox in its form display, and a published product.
  3. Edit the product. There is no "Published" checkbox on the form.
  4. Press Save. The product is now unpublished.

Commerce's ProductForm is just a convenient reproduction, as it sets #tree. Any content entity form that sets #tree and uses the footer group is affected.

Proposed resolution

Pin the copied footer's group identity so it resolves regardless of #tree:

$form['default_admin_sidebar']['footer']['#parents'] = ['footer'];

Safe: grouped children are attached at pre-render, after their own #parents are computed, so no submitted field name changes. Verified locally on 11.4.5 with Commerce 3.3.8. The product form renders status[value] in the sidebar and saves published; node forms are unchanged.

Remaining tasks

  • MR with the fix above.
  • Test coverage: a #tree entity form should render its footer-grouped status checkbox and keep its published state on save.

User interface changes

None intended. The checkbox appears in the sidebar where it was always meant to, on forms where it currently does not appear at all.

API changes

None.

Data model changes

None.

Issue fork drupal-3618586

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

f0ns created an issue. See original summary.

f0ns opened merge request !16806

f0ns’s picture

f0ns commented

MR pushed.

The group name comes from #parents, so pinning the copy's to ['footer'] makes it register as footer on treed forms too.

Verified on 11.4.5 with Commerce 3.3.8: the product form now renders name="status[value]" and saving keeps it published. Node forms unchanged.

f0ns’s picture

f0ns commented
Status: Active » Needs review
nitinkumar_7’s picture

nitinkumar_7 commented

Could we add a regression test for a form with #tree enabled that verifies the copied footer keeps #parents as ['footer'] and that elements using '#group' => 'footer' resolve to the copied footer correctly?

The Commerce verification is useful, but this is a subtle Form API regression, so having a core test for the #tree case would help ensure the behavior doesnot regress.

f0ns’s picture

f0ns commented

Test added in FooterGroupTest. Fails without the fix, passes with it.

f0ns’s picture

f0ns commented

The red PHPUnit run is unrelated: the test passes, but it trips a pre-existing deprecation fixed in #3618601.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I left one minor comment on the MR. But overall I'm wondering how critical the form submission is to the test. This issue became apparent because entities were being unpublished without the status checkbox on the form. But maybe verifying the presence of the checkbox is enough. If that's true, then that would allow this test to be a Kernel test instead. Not only would that be cheaper to run, the hook could be moved directly into the test and we wouldn't need the test module.

For the record, I checked the Core entity types for this issue. It doesn't seem like any of them are natively affected by this bug, which is unfortunate. I verified the issue by installing the MR, reverting the fix, and enabling the new test module to alter the Node edit form.

f0ns’s picture

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

smustgrave commented

Left 1 question on the MR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Okay I see what happened, the existing AdminTest was also doing that. I just went ahead and fixed both instances and renamed the file to be more generic for future node tests. Believe I'm still good to mark as the rest is fine.

poker10’s picture

poker10 commented
Priority: Normal » Major

Given default_admin is now the default admin theme, silently unpublishing entities seems at least Major.

  • amateescu committed bea4f5e4 on 11.4.x 
    fix: #3618586 default_admin loses the footer group on forms that set #...

  • amateescu committed 58e36a23 on 11.x 
    fix: #3618586 default_admin loses the footer group on forms that set #...

  • amateescu committed eaba66f4 on main 
    fix: #3618586 default_admin loses the footer group on forms that set #...
amateescu’s picture

amateescu commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed eaba66f4188 to main and 58e36a2376f to 11.x and bea4f5e45b5 to 11.4.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.

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

godotislate opened merge request !17232

godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs review

This broke the 11.4.x build

Admin Node (Drupal\Tests\default_admin\Functional\AdminNode)
     ⚠ Published checkbox on treed form
    
    1 test triggered 1 deprecation:
    
    1) /builds/issue/drupal-3625553/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:51
    Theme "default_admin" is overriding a deprecated library. The "node/form" asset library is deprecated in drupal:11.4.0 and is removed from drupal:12.0.0. There is no replacement. See https://www.drupal.org/node/3566511
    
    Triggered by:
    
    * Drupal\Tests\default_admin\Functional\AdminNodeTest::testPublishedCheckboxOnTreedForm
      /builds/issue/drupal-3625553/core/themes/default_admin/tests/src/Functional/AdminNodeTest.php:60
    
    OK, but there were issues!
    Tests: 1, Assertions: 11, Deprecations: 1.

MR with 11.4.x fix, just add #[IgnoreDeprecations] to the test: https://git.drupalcode.org/project/drupal/-/merge_requests/17232

  • longwave committed 57ff5213 on 11.4.x 
    test: #3618586 default_admin loses the footer group on forms that set #...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Fixed

Committed from NR as the fix is trivial and it unbreaks HEAD.

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.