Problem/Motivation

If a form organizes elements in groups (vertical tabs) using #group AJAX functionality via #ajax may break. If one tries to do an AJAX update of an element inside #group it gets simply wiped out.

The reason is \Drupal\Core\Render\Element\RenderElement::preRenderGroup(), which marks element as #printed and prevents it from actually rendering.

Proposed resolution

Let's make \Drupal\Core\Form\FormAjaxResponseBuilder::buildResponse() aware of this and prevent preRenderGroup() from being run before sending element to the renderer.

CommentFileSizeAuthor
#15 2567091_15.patch4.8 KBslashrsm
#15 interdiff.txt1.85 KBslashrsm
#13 interdiff.txt1.08 KBslashrsm
#13 2567091_13.patch4.04 KBslashrsm
#8 2567091_8_TEST_ONLY.patch3.03 KBslashrsm
#8 2567091_8.patch3.91 KBslashrsm
#3 interdiff.txt972 bytesslashrsm
#3 2567091_3.patch900 bytesslashrsm
#2 2567091_1.patch902 bytesslashrsm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Status: Active » Needs review
FileSize
2567091_1.patch902 bytes

Attached patch tries to fix this issue in a general way.

Fix addresses the issue only for the cases where parts of form are returned. Ajax callbacks that return own ajax commands are not covered and I am not sure how to solve that efficiently.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
FileSize
2567091_3.patch900 bytes
interdiff.txt972 bytes

Rephrased comment a bit.

marcingy’s picture

marcingy CreditAttribution: marcingy at Examiner.com commented
Status: Needs review » Reviewed & tested by the community

Looks good

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +rc target triage

Can we write some tests for this? Added a tag for core contributors.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Yes this needs test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: +Needs issue summary update

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
Issue tags: -rc target triage, -Needs tests, -Needs issue summary update
FileSize
2567091_8.patch3.91 KB
2567091_8_TEST_ONLY.patch3.03 KB

We are past RC. 8.1.x material I assume.

Added test.

Status: Needs review » Needs work

The last submitted patch, 8: 2567091_8_TEST_ONLY.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Status: Needs work » Needs review

Expected fail.

Primsi’s picture

Primsi CreditAttribution: Primsi at Examiner.com for Examiner.com commented
Status: Needs review » Reviewed & tested by the community

Test looks ok

swentel’s picture

swentel CreditAttribution: swentel commented 
+++ b/core/lib/Drupal/Core/Form/FormAjaxResponseBuilder.php
@@ -81,6 +81,13 @@ public function buildResponse(Request $request, array $form, FormStateInterface
+      // If the result element is a member of a group it won't be rendered
+      // because \Drupal\Core\Render\Element\RenderElement::preRenderGroup()
+      // sets #printed as when the elements get rendered as part of the group.
+      if (!empty($result['#group'])) {

The comment confuses me a little. Could probably use a comma after 'it'. The 3rd line sounds weird to me, it feels like something is missing after/between 'as when' ? Won't block this as I don't have a better proposal for now.

Also, this is a straight bug fix, this can go in 8.0.x as well I guess no ?

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Version: 8.1.x-dev » 8.0.x-dev
FileSize
2567091_13.patch4.04 KB
interdiff.txt1.08 KB

Better? Let's see what maintainers say about 8.0.x vs 8.1.x.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review

Do we need to be concerned about the case where #ajax is set on a parent element, and #group is set on a child? Patch doesn't look like it would cover that if so. I'm assuming things are only broken when it's the same level, but just in case, and doubt we have test coverage for that.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
FileSize
interdiff.txt1.85 KB
2567091_15.patch4.8 KB

Something along those lines?

Primsi’s picture

Primsi CreditAttribution: Primsi at Examiner.com for Examiner.com commented
Status: Needs review » Reviewed & tested by the community

Comments addressed, I would say RTBC again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Thanks for the extra test coverage. Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 1909ebd on 8.1.x 
    Issue #2567091 by slashrsm: AJAX updates of an element in a #group are...

  • catch committed 2df1fe0 on 8.0.x 
    Issue #2567091 by slashrsm: AJAX updates of an element in a #group are...

Status: Fixed » Closed (fixed)

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