Problem/Motivation

Duplicate AJAX wrappers are causing some errors to be rendered twice.
Symptoms have been reported around file field and image field.
In #115 @lauriii notes the element prefix and suffix are added in the twig template, as well as in the renderer.

Steps to replicate using the File field:

  • Add a file field to basic page content type.
  • Upload file of any type other than text.
  • Double error is triggered.

Double Error is displayed

Proposed resolution

Fix this at the source in the ajax rendering system, not just at the points where symptoms occur.
Patch at #176

@xjm notes the #render_children property is defined in core/lib/Drupal/Core/Render/Element/RenderElement.php

Remaining tasks

  • Needs theme system maintainer review.
  • Create a follow-up issue to backport this to D7.
    Done #2827956: [D7] Duplicate AJAX wrapper around a file field
  • Fix templates in a non backwards compatible manner for D9.
  • Resolve ongoing discussion of related changes to metadata.
  • Decide whether or not to bubble up the cache and assets here. [See #185 ]

Example of a user interface change

Show error once, instead of twice! :-) This is just one example. There are likely other places that will be impacted by this change.

Before patch:

before patch

Patched:

patched

API changes

When #render_children is set, everything inside render array except children will be ignored.

Data model changes

None.

Discussion Summary

@idimopoulas reports doing a manual review at #177
@Fabianx explains the problem clearly in #134 and provides a patch at #137
@joelpittet manual review with screenshots at #122
@swentel reports symptom is fixed for image fields but still occurring for file fields in #69

See also
#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

Original report

If I edit my profile, and upload a picture to Picture field, after that I see the messages but twice all times. (drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead (1920886) ) not about the ajax its just rendering the element.

user edit duplicate messages

I find these message only one place (/core/modules/file/file.module;466) and I think somewhere is called twice. István Palócz see that bug, and he said the problem somewhere in ajax response.

Files: 
CommentFileSizeAuthor
#205 interdiff.txt2.55 KBlauriii
#205 duplicate_ajax_wrapper-2346893-205.patch13.84 KBlauriii
#204 Screenshot from 2017-05-20 17-09-44.png57.54 KBkattekrab
#199 duplicate_ajax_wrapper-2346893-194-reroll.patch12.88 KBjoelpittet
#197 Screenshot from 2016-11-16 19-36-26.png323.84 KBkattekrab
#194 interdiff.txt12.27 KBlauriii
#194 duplicate_ajax_wrapper-2346893-194.patch12.88 KBlauriii
#194 duplicate_ajax_wrapper-2346893-194-tests-only.patch4.78 KBlauriii
#182 2346893-double-error-wrongfileformat.png36.34 KBkattekrab
#176 interdiff.txt3.24 KBlauriii
#176 duplicate_ajax_wrapper-2346893-176.patch10.33 KBlauriii
#174 interdiff.txt3.96 KBlauriii
#174 duplicate_ajax_wrapper-2346893-174.patch9.82 KBlauriii
#168 interdiff.txt2.09 KBlauriii
#168 duplicate_ajax_wrapper-2346893-168.patch10.13 KBlauriii
#168 duplicate_ajax_wrapper-2346893-168-test-only.patch5.64 KBlauriii
#162 interdiff.txt1.33 KBlauriii
#162 duplicate_ajax_wrapper-2346893-162.patch7.79 KBlauriii
#159 interdiff.txt1.39 KBlauriii
#159 duplicate_ajax_wrapper-2346893-159.patch7.04 KBlauriii
#153 duplicate_ajax_wrapper-2346893-153.patch6.87 KBYogesh Pawar
#147 duplicate_ajax_wrapper-2346893-147.patch7.43 KBdeepak_zyxware
#137 duplicate_ajax_wrapper-2346893-136.patch7.43 KBFabianx
#137 interdiff.txt757 bytesFabianx
#122 Create_Article___Site-Install--patched.png129.19 KBjoelpittet
#122 Create_Article___Site-Install_and_Downloads.png157.76 KBjoelpittet
#122 Image___Site-Install.png70.05 KBjoelpittet
#122 Create_Article___Site-Install.png121.31 KBjoelpittet
#121 interdiff.txt3.13 KBlauriii
#121 duplicate_ajax_wrapper-2346893-121.patch4.58 KBlauriii
#121 duplicate_ajax_wrapper-2346893-121-test-only.patch2.67 KBlauriii
#120 interdiff.txt515 byteslauriii
#120 duplicate_ajax_wrapper-2346893-120.patch4.57 KBlauriii
#119 interdiff.txt767 byteslauriii
#119 duplicate_ajax_wrapper-2346893-119.patch4.67 KBlauriii
#118 interdiff.txt562 byteslauriii
#118 duplicate_ajax_wrapper-2346893-118.patch4.81 KBlauriii
#115 interdiff.txt2.16 KBlauriii
#115 duplicate_ajax_wrapper-2346893-115.patch4.79 KBlauriii
#111 duplicate-messages-file-upload.patch938 bytesdeepak_zyxware
user_edit_duplicate_messages.png216.01 KBcsakiistvan
#17 duplicate_ajax_wrapper-2346893-17.patch659 byteslauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,810 pass(es), 2 fail(s), and 0 exception(s). View
#17 Screen Shot 2014-11-27 at 9.33.36 PM.png26.46 KBlauriii
#17 Screen Shot 2014-11-27 at 9.33.56 PM.png65.5 KBlauriii
#17 Screen Shot 2014-11-27 at 9.34.43 PM.png27.26 KBlauriii
#17 Screen Shot 2014-11-27 at 9.35.02 PM.png67.51 KBlauriii
#22 duplicate_ajax_wrapper-2346893-22.patch898 byteslauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,568 pass(es). View
#29 2346893-duplicate-ajax-wrapper.patch1.55 KBRavindraSingh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,555 pass(es). View
#33 2346893-duplicate-ajax-wrapper-removed-white-spaces.patch1.54 KBRavindraSingh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2346893-duplicate-ajax-wrapper-removed-white-spaces.patch. Unable to apply patch. See the log in the details link for more information. View
#36 2346893-duplicate-ajax-wrapper-updated.patch2.19 KBRavindraSingh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,582 pass(es). View
#39 2346893-duplicate-ajax-wrapper-updated.patch2.19 KBRavindraSingh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,564 pass(es). View
#40 rermove_extra_lines-2346893-40.patch1.3 KBVj
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,576 pass(es). View
#46 2346893-46.fail_.patch1.45 KBidebr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es), 1 fail(s), and 0 exception(s). View
#46 2346893-46.patch3.82 KBidebr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2346893-46.patch. Unable to apply patch. See the log in the details link for more information. View
#58 2346893-58.fail_.patch3.59 KBidebr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,935 pass(es). View
#58 2346893-58.patch3.59 KBidebr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#61 2346894-61-fail.patch1.23 KBidebr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,027 pass(es), 1 fail(s), and 0 exception(s). View
#61 2346893-61.patch3.59 KBidebr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,023 pass(es). View
#67 2346894-61-applied.jpg85.95 KBRade
#72 2346893_72.patch3.21 KBslashrsm
#72 2346893_72_TEST_ONLY.patch2.72 KBslashrsm
#75 2346893_75_TEST_ONLY.patch2.69 KBslashrsm
#75 2346893_75.patch3.18 KBslashrsm
#75 interdiff.txt1006 bytesslashrsm
#92 interdiff.patch608 bytesgauravjeet
#92 2346893_92.patch3.29 KBgauravjeet
#98 2346893_98.patch3.26 KBslashrsm
#98 interdiff.txt630 bytesslashrsm
#105 2346893-105.patch3.23 KBswentel
#108 2346893-interdiff.txt1.5 KBswentel
#108 2346893-108.patch3.59 KBswentel

Comments

Rade’s picture

Component: user system » ajax system
FileSize
105.41 KB

I was able to reproduce the issue and investigated a bit further. It seems the issue is with the ajax upload showing messages twice, not with the user system. The file is not uploaded twice, it's just the messages being shown twice.

In the attached screenshot you can see that the same issue occurs when adding an image to an article. I added a maximum resolution to the image field settings so that uploading would produce a message.

Rade’s picture

FileSize
148.07 KB

I believe the problem is related to the fact that within the image field DOM, there are two identical ajax wrappers, and the status messages will appear directly after those.

Not sure how to fix this. Maybe the component should be "file system" instead of "ajax system"?

pp’s picture

Title: Duplicate messages on profile edit page » Duplicate AJAX wrapper around a file field

Clarify the title

Rade’s picture

Assigned: Unassigned » Rade

I'm working on this today in the sprint in amsterdam.

Rade’s picture

Rade’s picture

Component: ajax system » forms system
Assigned: Rade » Unassigned

I'll leave this for now and maybe pick it up some other time. Anyone feel free to continue on this!

Changing component to forms system as I believe the issue is related to it, more than the ajax system.

Anyone investigating this, have a look at these lines in ManagedFile.php

  // Prefix and suffix used for Ajax replacement.
  $element['#prefix'] = '<div id="' . $element['#id'] . '-ajax-wrapper">';
  $element['#suffix'] = '</div>';

That's where the wrapping div comes from.

klakegg’s picture

This may be found in FileWidget.php:

// Add a new wrapper around all the elements for Ajax replacement.
$element['#prefix'] = '<div id="' . $element['#id'] . '-ajax-wrapper">';
$element['#suffix'] = '</div>';
benelori’s picture

Confirming this bug...the form item(the container) and the inputs(file widget itself) are wrapped in a div with the same id, which makes the replaceWith method display two error messages.

Rade’s picture

I'll have a look at this.

Rade’s picture

FileSize
115.39 KB

I can confirm that the bug exists for both image and file fields, as seen in the screenshot below:

Rade’s picture

FileSize
726 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,427 pass(es). View

This patch fixes the bug for image fields, but not for file fields. Need to investigate further.

Rade’s picture

FileSize
667 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,406 pass(es), 36 fail(s), and 74 exception(s). View

When commenting out these two lines, both Ajax wrappers disappear from file fields. So somewhere the prefix and suffix get duplicated...

Rade’s picture

Component: forms system » file system

Starting to believe this might be file system bug, so changing the component.

Rade’s picture

Status: Active » Needs work

Also, I'm not working on this anymore now so anyone feel free to contribute.

lauriii’s picture

Status: Needs work » Needs review

Sending to bot

Status: Needs review » Needs work

The last submitted patch, 12: duplicate_ajax_wrapper-2346893-12.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
659 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,810 pass(es), 2 fail(s), and 0 exception(s). View
26.46 KB
65.5 KB
27.26 KB
67.51 KB

I found the reason for this. I don't know why we're processing the parent also while building file widget. First gonna check what testbot says and then I might investigate more.

Before:

After:

Status: Needs review » Needs work

The last submitted patch, 17: duplicate_ajax_wrapper-2346893-17.patch, failed testing.

lauriii’s picture

Ok more about the problems we're having now #2172241: Files and image widgets completely broken

Rade’s picture

@lauriii: Patch in #17 fixes the issue only image widgets, not for file widgets. That's pretty much the same I tried in #11.

Vj’s picture

Can you please try following patch

https://www.drupal.org/node/2349835#comment-9453065

lauriii’s picture

Status: Needs work » Needs review
FileSize
898 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,568 pass(es). View

This is the latest patch from the duplicate issue. It clearly fixes the problem at least in all cases I tested.

Vj’s picture

I have tested patch https://www.drupal.org/node/2349835#comment-9453065 with following cases :
1. File / Image field from content types
2. User profiles image
3. Ckeditor Image widget

Status: Needs review » Needs work

The last submitted patch, 22: duplicate_ajax_wrapper-2346893-22.patch, failed testing.

Status: Needs work » Needs review
jeqq’s picture

Status: Needs review » Reviewed & tested by the community

I've tested https://www.drupal.org/node/2346893#comment-9466125. It works fine for all cases I tested.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This seems a pretty fundamental change to the rendering system - we need needs to for this.

If this fix is correct then we should do it like this:

    if (isset($elements['#render_children'])) {
      $elements['#markup'] = $elements['#children'];
    }
    else {
      $prefix = isset($elements['#prefix']) ? SafeMarkup::checkAdminXss($elements['#prefix']) : '';
      $suffix = isset($elements['#suffix']) ? SafeMarkup::checkAdminXss($elements['#suffix']) : '';
      $elements['#markup'] = $prefix . $elements['#children'] . $suffix;
    }

Also we should add a comment about why this is necessary.

RavindraSingh’s picture

Assigned: Unassigned » RavindraSingh
Issue tags: +#dcdelhi
RavindraSingh’s picture

FileSize
1.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,555 pass(es). View

Thanks laurii making it for more simpler. I just added the code in as @alexpott said, its fine if we are initializing if markup doesn't exists and adding markup if prefix and suffix exist.

RavindraSingh’s picture

Assigned: RavindraSingh » Unassigned
Status: Needs work » Needs review
oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

1. s/esixts?
2. remove extra final whitespaces

oriol_e9g’s picture

Status: Reviewed & tested by the community » Needs work

Ops!

RavindraSingh’s picture

FileSize
1.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2346893-duplicate-ajax-wrapper-removed-white-spaces.patch. Unable to apply patch. See the log in the details link for more information. View

@oriol_e9g, only one line was adding white space has been removed now. updated patch is attached here. Thanks for reviewing.

RavindraSingh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: 2346893-duplicate-ajax-wrapper-removed-white-spaces.patch, failed testing.

RavindraSingh’s picture

FileSize
2.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,582 pass(es). View

Removed more white spaces with fix.

RavindraSingh’s picture

Status: Needs work » Needs review
oriol_e9g’s picture

esixts should be exists

RavindraSingh’s picture

FileSize
2.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,564 pass(es). View

Fixed the typos

Vj’s picture

FileSize
1.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,576 pass(es). View

removed extra lines.

ifrik’s picture

Applied the patch in #40 and it fixes the problem reported in #2409181: Error message displayed twice.

csardev’s picture

The patch of comment # 40 solves the problem and also resolve the issue https://www.drupal.org/node/2409181

vacho’s picture

I applied the patch in #40. It is works.

harshil.maradiya’s picture

Status: Needs review » Reviewed & tested by the community

it working fine

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Still needs tests.

idebr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es), 1 fail(s), and 0 exception(s). View
3.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2346893-46.patch. Unable to apply patch. See the log in the details link for more information. View

I traced this back to #2172241: Files and image widgets completely broken. By calling the parent::process() function the field is processed multiple times resulting in duplicate markup, so I copied the necessary code from the parent function to the ImageWidget.

The last submitted patch, 46: 2346893-46.fail_.patch, failed testing.

Fabianx’s picture

Yes, the fix for the double process seems correct (though I have not checked the patch in detail, yet).

But this might be also related to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead, which was fixed, but IMHO not totally fixed properly.

I just bring this to attention again - as the original patch changed the #render_children logic ...

lauriii queued 46: 2346893-46.patch for re-testing.

gauravjeet’s picture

Issue tags: +SrijanSprintDay

I tested the patch and got the issue resolved. Patch in #46 seems to have done the fix.

Not changed status to RTBC, looks like @Fabianx #49 has to relate this issue to something else ?

RavindraSingh’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@Fabianx, I didn't find this issue relates with the added issues by you.

Patch in #46 seems to have done the fix as @gauravjit also mentioned.

Moving this to RTBC for committer to get more feedback if issue come back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 2346893-46.patch, failed testing.

idebr’s picture

Status: Needs work » Needs review

Updates for Contrib module one Other LocaleUpdateTest.php 144 Drupal\locale\Tests\LocaleUpdateTest->testUpdateImportSourceRemote()

I doubt this is related. I'll poke the testbot again to make sure.

mondrake queued 46: 2346893-46.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 46: 2346893-46.patch, failed testing.

mondrake’s picture

Issue tags: +Needs reroll
idebr’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,935 pass(es). View
3.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Status: Needs review » Needs work

The last submitted patch, 58: 2346893-58.patch, failed testing.

mondrake’s picture

idebr’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,027 pass(es), 1 fail(s), and 0 exception(s). View
3.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,023 pass(es). View

It appears I botched the test only patch. Attached is a correct one. The contents is unchanged.

The last submitted patch, 61: 2346894-61-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 61: 2346893-61.patch, failed testing.

Fabianx queued 61: 2346894-61-fail.patch for re-testing.

Status: Needs work » Needs review

Fabianx queued 61: 2346893-61.patch for re-testing.

The last submitted patch, 61: 2346894-61-fail.patch, failed testing.

Rade’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
85.95 KB

Applied #61 and it seems to fix the issue with image fields only. The issue still exists with file fields.

Mile23’s picture

Status: Needs work » Postponed (maintainer needs more info)

I can't repro the problem. Is this maybe already fixed elsewhere?

slashrsm’s picture

Status: Postponed (maintainer needs more info) » Needs work
Issue tags: +d8ux

This is fixed for image fields but remains problem with file fields.

Tagging with UX since printing double messages isn't the nicest experience.

Image field seems to be fixing this by adding this code in the template_preprocess_image_widget() and printing "data" instead of "element" in the template:

  $variables['data'] = array();
  foreach (Element::children($element) as $child) {
    $variables['data'][$child] = $element[$child];
  }

File widget uses entire "element" instead which causes #suffix to be printed twice.

slashrsm’s picture

Applied #61 and it doesn't fix the issues (obviously since it touched Image widget only).

Mile23’s picture

It seems like the test from #61 could be modified for any of the fields where this is still a problem.

slashrsm’s picture

This fixes the problem for file fields too. I kept the test from #61 (it makes sense to test image fields too) and added similar test for file fields.

Fix is not the nicest. I'd prefer similar solution to the one image field has. That would mean BC-breaking file_managed_file template, which is not acceptable at this point.

The last submitted patch, 72: 2346893_72.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
3.18 KB
1006 bytes

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

The last submitted patch, 72: 2346893_72.patch, failed testing.

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 75: 2346893_75.patch, failed testing.

slashrsm’s picture

Status: Needs work » Reviewed & tested by the community

The last submitted patch, 72: 2346893_72.patch, failed testing.

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 75: 2346893_75.patch, failed testing.

slashrsm’s picture

Status: Needs work » Reviewed & tested by the community

Seems to be passing.

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

The last submitted patch, 72: 2346893_72.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +rc target triage
+++ b/core/modules/file/file.module
@@ -1212,6 +1212,9 @@ function template_preprocess_file_managed_file(&$variables) {
+
+  unset($variables['element']['#prefix']);
+  unset($variables['element']['#suffix']);

We need a comment here to explain why.

It also seems to me that this fix would be worth getting in during RC. But for that we need to 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.

gauravjeet’s picture

Added comment as suggested by @alexpott

gauravjeet’s picture

Status: Needs work » Needs review

Changed status -> Needs review

Status: Needs review » Needs work

The last submitted patch, 92: 2346893_92.patch, failed testing.

The last submitted patch, 72: 2346893_72.patch, failed testing.

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

slashrsm’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.26 KB
630 bytes

Rephrased comment a bit and attempt to fix corrupt patch. Added RC triage part to the summary.

I propose that we open a 9.0.x issue to fix this as part of the template as described in #72.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Rtbc'ing assuming it is green

The last submitted patch, 92: 2346893_92.patch, failed testing.

The last submitted patch, 72: 2346893_72.patch, failed testing.

The last submitted patch, 72: 2346893_72_TEST_ONLY.patch, failed testing.

The last submitted patch, 75: 2346893_75_TEST_ONLY.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -1212,6 +1212,10 @@ function template_preprocess_file_managed_file(&$variables) {

@@ -1212,6 +1212,10 @@ function template_preprocess_file_managed_file(&$variables) {
+  // Remove unnecessary double errors on file upload which results in bad UX.

But you're not removing double errors - you're remove the prefix and suffix because it already exists elsewhere. TBH this seems a strange place to do this - I think we should be doing this where the duplication occurs - but I guess that is what you are hinting at with saying that this would take a template change.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -rc target triage
FileSize
3.23 KB

pfew, this is a silly bug. Patch didn't apply anymore, so a reroll first. Not sure if there's a better place either to fix this bug.

swentel’s picture

So looked why the image field didn't have this problem and the reason is that it's using a dedicated theming function for the widget and a template file. Still have no clue where the double wrapper comes from though, but without that template file the image widget would suffer from the same problem.

We could do this for the file widget as well, problem is that the managed file element would still have the problem then, and probably individually using the image element has the same problem. Digging further.

swentel’s picture

Starting to wonder if this a general problem anyway, just stumbled upon #2630960: Doubled pre- and suffix in datetime FormElement, will do some tests with prefixes and suffixes.

swentel’s picture

So this is a different approach, which @lauriii already proposed in #22 and effectively works.
I couldn't come up with a decent comment yet, because I'm not entirely sure what happens there in the render process (that function is quite complex - which is an understatement)

Wim Leers’s picture

Component: file system » render system

Welcome to drupal_render()! This particular part of it is equivalent with the D7 logic.

swentel’s picture

Note, then in the end I'm running with #105 - so maybe we should go with that one and be done with it.

deepak_zyxware’s picture

Status: Needs review » Needs work

The last submitted patch, 111: duplicate-messages-file-upload.patch, failed testing.

juanramonperez’s picture

Status: Needs work » Reviewed & tested by the community

#105 works for me.

catch’s picture

Status: Reviewed & tested by the community » Needs work

#105 at least needs a better comment to address @alexpott's comment in #104.

lauriii’s picture

Version: 8.0.x-dev » 8.1.x-dev
FileSize
4.79 KB
2.16 KB

I just spent a nice day debugging why is this necessary and why is this the right thing to do. So what happens there is that the render element is being rendered in the file-widget-multiple.html.twig template which is used in a children of that render array and the prefix and suffix will be added as a result in there. Also when the render element itself is being rendered in the Renderer the prefix and suffix is added. Prefix and suffix should be only when actually rendering the render element.

lauriii’s picture

Assigned: Unassigned » Cottser
Status: Needs work » Needs review

Assigning for Cottser's review since I believe he has most idea what's happening there

Status: Needs review » Needs work

The last submitted patch, 115: duplicate_ajax_wrapper-2346893-115.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Amsterdam2014, -#dcdelhi, -SrijanSprintDay
FileSize
4.81 KB
562 bytes
lauriii’s picture

Removed some of the unnecessary documentation

lauriii’s picture

lauriii’s picture

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
121.31 KB
70.05 KB
157.76 KB
129.19 KB

Reviewed the code for the nitpicks in the last patch and tested it manually to ensure it does what is expected.

Single image works as expected:

Change to unlimited images in field settings:

Before patch:

Patched:

The last submitted patch, 121: duplicate_ajax_wrapper-2346893-121-test-only.patch, failed testing.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.34 KB
2.1 KB

Wow, great work here!

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -507,15 +507,21 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    // Only apply the prefix and suffix markup around the children if they have
+    // not already been rendered by the theme manager.
+    if (!isset($elements['#render_children'])) {
+      // We store the resulting output in $elements['#markup'], to be
+      // consistent with how render cached output gets stored. This ensures that
+      // placeholder replacement logic gets the same data to work with, no
+      // matter if #cache is disabled, #cache is enabled, there is a cache hit
+      // or miss.
+      $prefix = isset($elements['#prefix']) ? $this->xssFilterAdminIfUnsafe($elements['#prefix']) : '';
+      $suffix = isset($elements['#suffix']) ? $this->xssFilterAdminIfUnsafe($elements['#suffix']) : '';
+      $elements['#markup'] = Markup::create($prefix . $elements['#children'] . $suffix);
+    }
+    else {
+      $elements['#markup'] = Markup::create($elements['#children']);
+    }

The inner comment in the first branch actually applies to the second branch also.

So I think a cleaner/easier to understand solution would be what's in the attached patch.


Finally, doesn't this need to backported to Drupal 7?

alexpott’s picture

So do we have the same problem with the post rendering too?

    // Filter the outputted content and make any last changes before the content
    // is sent to the browser. The changes are made on $content which allows the
    // outputted text to be filtered.
    if (isset($elements['#post_render'])) {
      foreach ($elements['#post_render'] as $callable) {
        if (is_string($callable) && strpos($callable, '::') === FALSE) {
          $callable = $this->controllerResolver->getControllerFromDefinition($callable);
        }
        $elements['#children'] = call_user_func($callable, $elements['#children'], $elements);
      }
    }
Cottser’s picture

Assigned: Cottser » Unassigned

Yeah I'm unassigning, the render_children stuff has always just confused me, sorry for the delayed response. Will keep an eye on this issue though.

I will say it seems a bit risky for 8.1.x, we may want to bump to 8.2.x.

akalata’s picture

I believe this bug is also causing issues where the file upload runs twice, creating two entries in file_managed table, both pointing to the same file. One fid is mapped to the field and marked permanent, the other is marked temporary. When file cleanup is triggered, the temporary fid's file is deleted -- which also deletes the file the permanent fid is pointing to. It's an annoyingly intermittent bug making it appear that files are deleted at random.

josmera01’s picture

enapthine’s picture

Patch from #128 working in 8.1.8.

MattDanger’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #128 works nicely on 8.1.7 and 8.1.8 for file fields.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

I don't see an answer yet for #125, so marking NR for that.

xjm’s picture

Title: Duplicate AJAX wrapper around a file field » Duplicate AJAX wrapper around a file field, possibly causing unexpected file deletions
Priority: Normal » Critical

@akalata:

One fid is mapped to the field and marked permanent, the other is marked temporary. When file cleanup is triggered, the temporary fid's file is deleted -- which also deletes the file the permanent fid is pointing to. It's an annoyingly intermittent bug making it appear that files are deleted at random.

Whoa, that sounds like a critical issue. Thanks for pointing that out. Promoting for visibility to check whether that is the case. If it is, and this patch fixes it, we should add a test for that case too.

swentel’s picture

Hmm, I know of #2666700: User profile images unexpectedly deleted which describes that as well, but I haven't been able to reproduce this at all. I also think we'd be flooded with bug reports alike, and that hasn't been the case yet, unless users don't notice, but that seems unlikely. So my gut feeling is telling me that this duplicate wrapper wouldn't cause that, but better safe than sorry of course :)

Fabianx’s picture

Oh, well ... #render_children continues to hunt me ...

#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is the original issue for that. Bad if that is what creates the duplicate ajax wrapper.

Let me explain this a little, to give others a chance to see what _should_ happen here:

This happens when a render array tries to render itself to render its remaining fields:

e.g. consider a twig template like this called foo.html.twig a theme hook, called 'test_foo', which uses a render element of 'foo':

{{ foo.bar }}
{{ foo|without('bar') }}

Now what happens is:

$build = [
  '#theme' => 'test_foo',
  '#prefix' => 'Prefix',
  'bar' => 'bar',
  'child_1' => 'baz',
  'child_2' => 'llama',
  '#suffix' => 'Suffix',
];

Now some code does:

$markup = drupal_render($build);

Because of how 'render element' works, the whole render array becomes the 'foo' element.

So now drupal_render() [Renderer->render()] starts and sets #render_children => TRUE, which prevents an endless recursion.

So in theory, when '#render_children' is set, then drupal_render() should really only consider the children, e.g. 'baz' and 'bar', because that is what the calling code expects.

We tried to short circuit that by doing that case very early in the function, however somehow that failed some tests originally.

Maybe this works now, so that is what I would try first:

drupal_render() {

  if (isset($element['#render_children'])) {
    // render children
    return;
  }
}

Likely the simplest to make this work with render_context + co is to just create a new array with _just_ the children and recursively call itself.

e.g.

drupal_render() {

  if (isset($element['#render_children'])) {
    $new_build = array_intersect_key($element, element_children($element));
    return drupal_render($new_build);
  }
}

Then return that.

Fabianx’s picture

Status: Needs review » Needs work

The last submitted patch, 135: duplicate_ajax_wrapper-2346893-135.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
757 bytes
7.43 KB

Fixing test assumptions, which make sense.

xjm’s picture

xjm’s picture

Priority: Critical » Normal
Issue tags: +Needs issue summary update

I promoted #2666700: User profile images unexpectedly deleted to critical and am re-demoting this one. @akalata, could you follow up there, maybe help with steps to reproduce it? I have not been able to yet.

I also haven't been able to reproduce the original issue in the summary with the duplication of the message on user profile fields. I also can't reproduce the validation message being duplicated for images in general unless it's a mutlivalue field. However, I can reproduce it with a single-value non-image file field.

xjm’s picture

Issue summary: View changes
xjm’s picture

Title: Duplicate AJAX wrapper around a file field, possibly causing unexpected file deletions » Duplicate AJAX wrapper around a file field
Fabianx’s picture

Priority: Normal » Major

The closed duplicate issue was triaged major, so this one should stay major as well.

However I think this solves it properly for all cases (finally!).

xjm’s picture

@Fabianx, which duplicate?

seanr’s picture

This fixes for me.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

deepak_zyxware’s picture

Status: Needs review » Needs work

The last submitted patch, 147: duplicate_ajax_wrapper-2346893-147.patch, failed testing.

lucastockmann’s picture

It seems that it's fixed in 8.2.x. Can anyone confirm that?

Beside that I think we should definitely bring this into 8.1.x. - I'm not that much into Drupal 8 release cycle, is that something we can do?

Drupal 8.1.x will not receive any further development aside from security fixes
Fabianx’s picture

Issue tags: +Needs reroll

This is definitely still broken, patch needs a re-roll.

Fabianx’s picture

Version: 8.1.x-dev » 8.2.x-dev
Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
FileSize
6.87 KB

Here is the rerolled the patch against 8.2.x

Yogesh Pawar’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 153: duplicate_ajax_wrapper-2346893-153.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Needs work

The code fix itself looks good for me. I think we could improve the docs a little bit so it would be easier for someone just looking at the code to understand what's happening.

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,25 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // Render only the children if the #render_children property is set.
    +    if (isset($elements['#render_children'])) {
    

    We could document here that #render_children is internal property and add a @see to the ThemeManager::render().

  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,25 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      // A non-empty #children property takes precedence.
    

    We should explain here why the #children is not empty sometimes.

  3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,25 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      // It is okay to modify the original elements for this, because this will
    +      // be called from a twig template, where the variable is passed by value.
    

    This is rather confusing comment since render could be also called outside Twig template

Fabianx’s picture

#157

1. Agree

2. Should only happen if a pre-process function is populating #children (no one in core does that), likely no reason for this except for tests and BC compatibility.

3. The wanted reasoning here is:

#render_children property is internal and hence will only be set from within the ThemeManager, which then most likely will use a twig template. Though indeed it could get messy ...

Maybe:

function (&$elements) {
  if (...) {
    // ...
    $new_elements = array_intersect_key($elements, $children);
    $elements = &$new_elements;
  }
}

Hm, that should work: https://3v4l.org/dHiB1

Though it means the $elements is not updated with the result ...

However any other engine than twig should use drupal_render_children() anyway ...

So I guess overwriting the 'reference' should work and avoid accidentally changing the callers state, which in 99% of the cases won't use the changed $elements anyway.

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.04 KB
1.39 KB

Made these changes for the patch. Thanks for the feedback @Fabianx

Status: Needs review » Needs work

The last submitted patch, 159: duplicate_ajax_wrapper-2346893-159.patch, failed testing.

Fabianx’s picture

Hm, those unit tests ...

We might need to populate at least #markup and #attached in the original array before returning ... (e.g. save $elements before).

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.79 KB
1.33 KB

This should fix the tests

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Nice, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 162: duplicate_ajax_wrapper-2346893-162.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

The test failures were unrelated to the change here

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

The issue summary still needs updating as it hasn't been since the tag was added in #139. Also whilst it is great to see the implicit test coverage added on the file field level it'd be great to see some additional test coverage added in Drupal\Tests\Core\Render\RendererTest since the renderer is complex and unit test coverage really helps.

lauriii’s picture

Assigned: Unassigned » lauriii

Thanks @alexpott for your feedback. I will work on writing the additional unit test coverage for this.

lauriii’s picture

Added some test coverage for both, unit and kernel tests.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

New tests look great, back to RTBC.

The last submitted patch, 168: duplicate_ajax_wrapper-2346893-168-test-only.patch, failed testing.

xjm’s picture

Priority: Major » Normal

@Cottser, @alexpott, @lauriii, @joelpittet and I agreed that the current scope of the issue is a normal bug. Thanks everyone! Glad to see it RTBC. (Though it still hasn't had the issue summary update requested in #139 which makes it take longer to review).

xjm’s picture

Just taking some notes -- none of this is blocking at the moment but wanted to document a few things I saw in the process of trying to review it.

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,32 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // Render only the children if the internal #render_children property is
    +    // set.
    

    The #render_children property is defined in core/lib/Drupal/Core/Render/Element/RenderElement.php:

     * - #render_children: (bool, internal) Set to FALSE by the rendering process   
     *   if the #theme call should be bypassed (normally, the theme is used to      
     *   render the children). Set to TRUE by the rendering process if the children
     *   should be rendered by rendering each one separately and concatenating.     
    

    So that all is documented correctly already.

  2. +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
    @@ -156,4 +157,26 @@ protected function getFieldSettings($min_resolution, $max_resolution) {
    +  public function testAJAXValidationMessage() {
    

    This method did not actually fail on the test-only run; was it expected to?

  3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,32 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      // it could cause unexpected behaviour with other templating engines than
    

    behavior :)

  4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,32 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      // Twig. Save the original referenced variable to allow making changes for
    +      // the referenced variable in case explicitly wanted so.
    +      $original_elements = &$elements;
    +      $new_elements = array_intersect_key($elements, array_flip($children));
    +      $elements = &$new_elements;
    
    @@ -513,6 +535,13 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // #attached and #markup values should be always saved to the referenced
    +    // elements variable.
    +    if (isset($original_elements)) {
    +      $original_elements['#markup'] = $elements['#markup'];
    +      $original_elements['#attached'] = $elements['#attached'];
    +    }
    

    I don't quite understand the last sentence in the first inline comment here.

    Can we use a variable name that describes the purpose of $original_elements? The fact that they are "original" doesn't actually mean anything out of context.

    Also, my head nearly exploded trying to follow the array manipulation. :)

  5. +++ b/core/modules/file/src/Tests/FileFieldValidateTest.php
    @@ -71,8 +71,10 @@ function testFileMaxSize() {
    -    $small_file = $this->getTestFile('text', 131072); // 128KB.
    -    $large_file = $this->getTestFile('text', 1310720); // 1.2MB
    +    // 128KB.
    +    $small_file = $this->getTestFile('text', 131072);
    +    // 1.2MB.
    +    $large_file = $this->getTestFile('text', 1310720);
    

    These changes are out of scope and should not be included in the patch.

  6. +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
    @@ -8,6 +8,7 @@
     class ImageFieldValidateTest extends ImageFieldTestBase {
    +
       /**
    

    Out of scope change.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

It's great to see unit tests added to just test the render system changes that are made.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -209,6 +209,32 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+      // Avoid making this change for the referenced elements variable because
+      // it could cause unexpected behaviour with other templating engines than
+      // Twig. Save the original referenced variable to allow making changes for
+      // the referenced variable in case explicitly wanted so.
+      $original_elements = &$elements;
+      $new_elements = array_intersect_key($elements, array_flip($children));
+      $elements = &$new_elements;

I think each line needs to be commented here to explain what is occurring. We all need to point out that this is necessary because $elements is passed in by reference to doRender() and doRender() is recursive.

Before we can commit this patch we need to update the issue summary so that the actual problem and resolution is detailed.

lauriii’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
9.82 KB
3.96 KB

Thanks for the review @xjm! I rewrote the documentation on the confusing part with some help from @alexpott. I also made other changes that was requested.

joelpittet’s picture

Some minor bits and things from code. Just taking it for a tour. Assume all is good if I don't come back with any reports

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,36 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      if (!empty($elements['#children'])) {
    +        $children = ['#children'];
    +      }
    +      else {
    +        $children = Element::children($elements);
    +      }
    +
    +      if (empty($children)) {
    +        return '';
    +      }
    

    The empty check can be nested in the else because it is only relevant there.

    The $children variable as elsewhere in our codebase should be $children_keys because that is what we get back.

  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -209,6 +209,36 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      // already rendered when the #render_children is set, and therefore they
    

    nit: don't need the comma before and because it's not in a series.

lauriii’s picture

Some further cleaning on this patch.

idimopoulos’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I have tested this in Drupal 8.2 and it's working as expected. Nice work.
In UI, tests were done in single and multi cardinality and the behaviour was normal.
It works great in our project where we also use a custom theme. Error and success messages only show once.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

I know this issue is still being discussed by @lauriii and @Fabianx and other theme maintainers so setting back to needs review and adding needs subsystem maintainer review.

It also really needs an issue summary update since the patch and the summary are not aligned since the problem is deep in the render system innards and not much to do with the file module.

Wim Leers’s picture

Yep, nothing to do with file field, everything to do with AJAX system. This also affects BigPipe, because it uses the AJAX system. See #2738685: [PP-1] BigPipe leaves wrapper <div>s in place, because AJAX system leaves them — this can cause some CSS to break. Should probably be mentioned in the updated IS as an example, file field is just another example.

Fabianx’s picture

Issue tags: +needs backport to D7

Maybe we should bring this back to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead as that has the older history instead.

Still discussing somewhat with Laurii.

And I found out we can likely backport that to D7 with keeping BC.

kattekrab’s picture

We've hit this one on a current project too and can confirm that duplicate_ajax_wrapper-2346893-176.patch fixes the issue for us!

I'll see if I can make a start on an issue summary update :)

kattekrab’s picture

Issue summary: View changes
kattekrab’s picture

Issue summary: View changes
alexpott’s picture

For me the other thing to work out before this is RTBC is whether or not we should bubble up the cache and assets here:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -513,6 +539,12 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    // #markup should be always saved to the referenced elements variable to
+    // prevent re-rendering.
+    if (isset($original_elements)) {
+      $original_elements['#markup'] = $elements['#markup'];
+    }

I think everything is working atm because the render system is doing it automagically for us. At the very least we need a comment as to why we are only preserving the markup.

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -513,6 +539,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +      $original_elements['#cache'] = CacheableMetadata::createFromRenderArray($elements['#cache']);
    

    Shouldn't this be

    CacheableMetadata::createFromRenderArray($elements);
    

    and not CacheableMetadata::createFromRenderArray($elements['#cache']); and if so, indicates we're missing test coverage for cacheability metadata bubbling with #render_children?

  2. +++ b/core/tests/Drupal/KernelTests/Core/Render/RenderTest.php
    @@ -44,6 +44,23 @@ function testDrupalRenderThemePreprocessAttached() {
    +    $this->assertNoRaw('<div>kangarookitten</div>');
    

    Should we have a positive assert too, just to ensure that something is happening (a good habit to get into)?

larowlan’s picture

Also, that's some boss level issue summary updating Donna!

larowlan’s picture

Wim Leers’s picture

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

#183+#184: that looks splendid! Except I realized that #179 is wrong, I confused this issue with #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs, which also has been around for years. What I'm still missing in the IS, is a root cause analysis. I suspect this is it:

In #115 @lauriii notes the element prefix and suffix are added in the twig template, as well as in the renderer.

(Quoted from the current IS.)


#186.1 is absolutely right, that's wrong. NW for that.

kattekrab’s picture

Thanks @Wim Leers!

Except I realized that #179 is wrong, I confused this issue with #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs, which also has been around for years.

So does that mean this has nothing to do with big pipe afterall?

Also, that's some boss level issue summary updating Donna!

Thanks @larowlan :-)

kattekrab’s picture

Issue summary: View changes

Adding this to the IS.

  • Decide whether or not to bubble up the cache and assets here. [See #185 ]

Is that something that could be done in a follow up?

This bug was RTBC before we released D8 and is impacting a site I'm working on right now. The patch fixes the issue we're having. If there are other problems that also need to be addressed, it might make sense to have a new issue that's not nearing the 200 comment mark to address those?

Speaking of follow-ups, I'll make one for the D7 backport now.

Thanks everyone! :-)

kattekrab’s picture

Issue summary: View changes
kattekrab’s picture

Issue summary: View changes
lauriii’s picture

Added support for the cacheable metadata bubbling. I also fixed a bug where #access on the mainlevel wasn't being taken in account when rendering children. I also added test coverage for these things.

The last submitted patch, 194: duplicate_ajax_wrapper-2346893-194-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 194: duplicate_ajax_wrapper-2346893-194.patch, failed testing.

kattekrab’s picture

Issue summary: View changes
FileSize
323.84 KB

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 199: duplicate_ajax_wrapper-2346893-194-reroll.patch, failed testing.

jasonawant’s picture

Adding related issue: https://www.drupal.org/node/2745491. That issue's patch conflicts with this one in /core/modules/image/src/Tests/ImageFieldValidateTest.php.

SKAUGHT’s picture

nod_’s picture

kattekrab’s picture

Issue summary: View changes
FileSize
57.54 KB

Just saw this again testing media!

Maybe time for a reroll and see if we can get this in?

pretty please?

lauriii’s picture

We don't have to do the bubbleable metadata dance since we use RenderContext::update to apply the collected bubbleable metadata to the render array. I also added some more test coverage to asset handling in children render arrays since I couldn't find any pre-existing test coverage for that.

Wim Leers’s picture

This is an API change. The IS says: When #render_children is set, everything inside render array except children will be ignored.. We need a CR for this.

Concern

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -236,6 +236,36 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    // Render only the children if the internal #render_children property is
+    // set.
+    // @see \Drupal\Core\Theme\ThemeManager::render().
+    if (isset($elements['#render_children'])) {

@@ -428,10 +458,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
-    // element have to be rendered there. If the internal #render_children
-    // property is set, do not call the #theme function to prevent infinite
-    // recursion.
-    if ($theme_is_implemented && !isset($elements['#render_children'])) {
+    // element have to be rendered there.
+    if ($theme_is_implemented) {
       $elements['#children'] = $this->theme->render($elements['#theme'], $elements);

@@ -440,10 +468,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
-    // If #theme is not implemented or #render_children is set and the element
-    // has an empty #children attribute, render the children now. This is the
-    // same process as Renderer::render() but is inlined for speed.
-    if ((!$theme_is_implemented || isset($elements['#render_children'])) && empty($elements['#children'])) {
+    // If #theme is not implemented and the element has an empty #children
+    // attribute, render the children now. This is the same process as
+    // Renderer::render() but is inlined for speed.
+    if (!$theme_is_implemented && empty($elements['#children'])) {

@@ -467,9 +495,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
-    // If the internal #render_children property is set, do not call the
-    // #theme_wrappers function(s) to prevent infinite recursion.
-    if (isset($elements['#theme_wrappers']) && !isset($elements['#render_children'])) {
+    if (isset($elements['#theme_wrappers'])) {

Even though I am one of the people who worked the most on the Render API in Drupal 8:

  • I never ever touched #render_children, so I can't say that I fully comprehend it, and therefore don't feel qualified to review the changes here
  • it's been more than 18 months since I worked on a significant patch for it

    Review

    1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
      @@ -236,6 +236,36 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
      +    // Render only the children if the internal #render_children property is
      +    // set.
      +    // @see \Drupal\Core\Theme\ThemeManager::render().
      +    if (isset($elements['#render_children'])) {
      

      So this is trying to deal with '#render_children' earlier. But I don't understand why yet. How does this break currently?

      We'll need a clear answer for somebody to RTBC this, but also for a core committer. I realize this is hard to explain, but that's exactly why we need to explain/document it clearly. If it isn't, risks are not understood well either, and it'll be RTBC for a very long time (because core committers are less likely to commit it).

    2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
      @@ -552,6 +578,17 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
      +    // #markup should be always saved to the referenced elements variable to
      +    // prevent re-rendering. #cache and #attached ensures that correct cacheable
      +    // metadata is applied for the re-rendered instances.
      +    if (isset($original_elements)) {
      +      $original_elements['#markup'] = $elements['#markup'];
      +      $original_elements['#cache'] = $elements['#cache'];
      +      $original_elements['#attached'] = $elements['#attached'];
      +      $original_elements['#printed'] = $elements['#printed'];
      +    }
      

      I don't understand this at all.

      $original_elements is never used anywhere?

    3. +++ b/core/modules/file/src/Tests/FileFieldValidateTest.php
      @@ -185,4 +185,25 @@ public function testFileRemoval() {
      +  public function testAJAXValidationMessage() {
      
      +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
      @@ -156,4 +156,26 @@ protected function getFieldSettings($min_resolution, $max_resolution) {
      +  public function testAJAXValidationMessage() {
      

      These are clear.

    4. +++ b/core/tests/Drupal/KernelTests/Core/Render/RenderTest.php
      @@ -44,6 +45,23 @@ public function testDrupalRenderThemePreprocessAttached() {
      +    // Ensure that #prefix and #suffix is only being printed once since that is
      +    // the behaviour the caller code expects.
      

      s/since that …//

geovanni.conti’s picture

Hi.

Patch #205 works great for me.

Thanks!

kattekrab’s picture

I just got another bug report from a client about this one.
:(

What do we need to do to move this forward - looks like @Wim Leers did a pretty comprehensive review - is anyone able to address those points and do a re-roll?

At 208 comments this is getting pretty long in the tooth now. I'm happy to do anything within my power to help here, but I'm not sure how best to assist.