Duplicate AJAX wrapper around a file field

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.

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"?

Clarify the title

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

This issue might be related in some way #2187757: Add form id as form's inner div id attribute to use as ajax wraper

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.

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>';

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.

I'll have a look at this.

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

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

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

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

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

Sending to bot

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:

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

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

Can you please try following patch

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

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

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

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

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.

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.

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

Ops!

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

Removed more white spaces with fix.

esixts should be exists

Fixed the typos

removed extra lines.

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

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

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

it working fine

Still needs tests.

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.

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 ...

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 ?

@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.

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.

Note - 'file.ajax_upload' route is being removed as part of #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache which is a critical.

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

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

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

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.

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

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

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.

Looks good

Seems to be passing.

+++ 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.

Added comment as suggested by @alexpott

Changed status -> Needs review

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.

Rtbc'ing assuming it is green

+++ 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.

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.

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.

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.

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)

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

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

This patch works fine for me in 8.02.

#105 works for me.

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

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.

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

Removed some of the unnecessary documentation

Pair coded the next iteration with @joelpittet

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:

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?

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);
      }
    }

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.

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.

This patch to version 8.1.3, thank you.

Patch from #128 working in 8.1.8.

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

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

@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.

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 :)

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.

Lets try this most simple approach.

Fixing test assumptions, which make sense.

Well, there was #2725415: Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted, but it was fixed awhile ago.

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.

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!).

@Fabianx, which duplicate?

#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead I did close as duplicate.

This fixes for me.

This works for me.

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

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

Here is the rerolled the patch against 8.2.x

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

#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.

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

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).

This should fix the tests

Nice, back to RTBC.

The test failures were unrelated to the change here

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.

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

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

New tests look great, back to RTBC.

@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).

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.

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.

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.

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.

Some further cleaning on this patch.

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.

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.

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: 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.

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.

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 :)