Rendering forms in AjaxResponses does not attach assets automatically

Working on this one. I worked with dstol to figure out the root cause, and the root cause is my patch in #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render. Figuring out a solution to clean this all up.

Closed the following as duplicates:

1. #2347175: Omitting required alt text in ckeditor image upload first time causes wrong redirection after saving
2. #2349629: Insert image popin breaks when trying to add alt text
3. #2349279: Removing an image using Quick Edit produces a quickeditFieldFormSaved error

One basic thing we could be done is to extend the ajax commands to accept render arrays and then handle that there.

I was thinking along the same lines.

Another issue about this was filed: #2369151: Errors in wysiwyg image modal forms break the go to url after submit.

I wonder whether we could introduce something like drupal_render_with_attachments?

Another issue about this was filed: #2373703: Image upload via WYSIWYG modal incorrectly redirects.

I'm now actively working on this.

And another issue about this was filed, this time for Quick Edit module: #2374921: It is not attaching the javascripts in Quick Edit.

Hi, in this issue description the author says: "Just calling drupal_render here should have been sufficient" and this is already happening in Drupal 7 but for some reason in Drupal 8 the function "drupal_process_attached" was removed from "drupal_render" even though it is stated in the documentation.
I have tested that these issues were solved by this patch

• #2369151: Errors in wysiwyg image modal forms break the go to url after submit
• #2374921: It is not attaching the javascripts in Quick Edit

Ups my bad, I had not seen this #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render
Sorry :P

Unassigned Wim, because that probably demotivated people from rerolling the patch.

Well, actually was planing to solve this as part of #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests, because they touch the same areas. The quick fix is just adding drupal_process_attached() everywhere, but that's terrible DX, so we don't want to introduce that.

That being said, a patch to do #4:

One basic thing we could be done is to extend the ajax commands to accept render arrays and then handle that there.

would be fine, because I could still refactor that later.

extend the ajax commands to accept render arrays and then handle that there

haven't been following closely, but that sounds a bit concise for a novice (it does for me ;-)
Like "which ajax commands should do what where ?"

This is why I was tempted to remove the "Novice" tag :)

And another duplicate: #2387205: Insert Image modal redirected to image upload form. The 8th report!

re-rolled the patch, review for testbot testing.

@tadityar, the patch is not correct I think Gives PHP syntax errors in bot. Please verify once. To get some help you can follow Making a Drupal patch with Git & Advanced patch contributor guide links.

Thanks!

It sounds like this needs a new patch based on #17 rather than a reroll.

Discussed with @xjm, @catch, @effulgentsia. We decided that this is critical because it how developers expect things to work - as shown by the fact that we have 8 reports of this already.

The issue summary needs an update to detail the solution and it would be great if we could have a test.

Assigned to myself. Working on this during next 24 hours based on #17 and #25.

New start then :)

This patch allows AJAX commands that require HTML input, to accept alternatively render arrays. If a render array is passed, then it is rendered to HTML by the Ajax command, and it is passed through drupal_process_attached() as well.

Calls to Ajax commands in code should be adjusted, either here or as follow-ups. I started converting only the calls in the editor module's EditorImageDialog and EditorLinkDialog classes, and it seems that the issue reported in #2387205: Insert Image modal redirected to image upload form is fixed by this patch.

Adding the possibility to pass along render arrays totally makes sense IMHO.

+++ b/core/lib/Drupal/Core/Ajax/InsertCommand.php
@@ -52,14 +52,21 @@ class InsertCommand implements CommandInterface {
-  public function __construct($selector, $html, array $settings = NULL) {
+  public function __construct($selector, $content, array $settings = NULL) {
     $this->selector = $selector;
-    $this->html = $html;
+    if (is_array($content)) {
+      $this->html = \Drupal::service('renderer')->render($content);
+      drupal_process_attached($content);
+    }
+    else {
+      $this->html = $content;
+    }
     $this->settings = $settings;
   }

I'd love to do that lazy, which means we just render it, when the result is rendered.

#29 something like this?

I am starting to wonder whether it would make more sense to accept *ONLY* render arrays for input (i.e. typehint the $content variables to array), instead of either HTML or render array. It would simplify the code and make it more consistent (see e.g. the ::drupalAttachLibrary() method in OpenDialogCommand, that could be dropped entirely). Also calling code would benefit from having to behave to pass render arrays instead of making drupal_render() calls. OTOH, it would be an API change and require quite some work to convert all the calling code.

Opinions?

I would argue it is an API change which is not worth to do.
On top of that pure API change level, you could also argue that your APIs should be decoupled, so there is nothing wrong with supporting strings as well.

+++ b/core/lib/Drupal/Core/Ajax/InsertCommand.php
@@ -52,18 +54,39 @@ class InsertCommand implements CommandInterface {
+   * Processes the content for output.
+   *
+   * If content is a render array, it may contain attached assets to be
+   * processed.
+   *
+   * @return string
+   *   HTML rendered content.
+   */
+  protected function getRenderedContent() {
+    if (is_array($this->content)) {
+      $html = \Drupal::service('renderer')->render($this->content);
+      drupal_process_attached($this->content);
+      return $html;
+    }
+    else {
+      return $this->content;
+    }
+  }

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -122,6 +125,26 @@ public function setDialogTitle($title) {
+   * Processes the content for output.
+   *
+   * If content is a render array, it may contain attached assets to be
+   * processed.
+   *
+   * @return string
+   *   HTML rendered content.
+   */
+  protected function getRenderedContent() {
+    if (is_array($this->content)) {
+      $html = \Drupal::service('renderer')->render($this->content);
+      drupal_process_attached($this->content);
+      return $html;
+    }
+    else {
+      return $this->content;
+    }

Could this be a trait? Seems nearly identical.

The trait could also include a method getRenderer() and setRenderer() which would replace the \Drupal::service('renderer') and allow unit-testing.

#31 ok maybe not generally, however at least for OpenDialogCommand and OpenModalDialogCommand I would suggest to change to render array only - isn't it that you would have to pre-render an array in calling code in any case?

Proposing that here. From a quick look at the developments in #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests it seems that quite some pieces of the same code are being touched by this patch too, shall this be postponed on that?

#32 in this patch now the two are diverging so I do not see the need for a trait any longer.

Edit:
Retracted the proposal

I'd prefer this to be postponed on that other patch, yes, because the solution in here still relies on the statics in _drupal_add_(css|js)(), which are (FINALLY!) being removed in that issue.

Once that issue is done, this patch will become simpler though :)

Bumped up against this one again during D8 tutorial.

#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests has landed now. I do not like the patch in #33 anymore as it is going into dire straits (how to decide when to render vs renderRoot, implications on bubbling metadata, etc.). So to keep it simple for now I just rerolled patch in #30 introducing a trait as suggested in #32.

By deferring rendering and attaching assets to the Ajax command, here the risk is what is being filed in #2407201: Consider introducing AjaxResponse::addAttachments(), i.e. the last Ajax command in a response will rule.

Just posting for progress and to run testing, will set as postponed again later.

#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests landed, hence this is now unpostponed. It solved all occurrences of this problem in D8 core. Tempted to make it major instead of critical, but since the DX problem still remains, keeping at critical.

#36: wow! Awesome work! The fact that the various AJAX commands are blissfully unaware of the AjaxResponse object was also the main obstacle when I looked into this.

Your great work in #36 gave me an alternative idea, which allows us to remove the indirect dependency of the AJAX command objects on the AjaxResponse object commands are rendered in. The idea: commands could instead implement an CommandWithAttachedAssetsInterface, allowing the AjaxResponse, after invoking CommandInterface::render(), checking if instanceof CommandWithAttachedAssetsInterface, and if so, getting the attached assets (an AttachedAssets instance). The AjaxResponse can then merge together all the AttachedAssets objects.
What do you think?

#2407201: Consider introducing AjaxResponse::addAttachments() now contains a patch; in the current state of *this* patch, only a single line needs to be changed.

Re. #37 why not. Here's a first cut - this however has reintroduced #2387205: Insert Image modal redirected to image upload form, do not know why :(

Will need more work.

+++ b/core/lib/Drupal/Core/Ajax/CommandWithAttachedAssetsTrait.php
@@ -0,0 +1,56 @@
+      $this->attachedAssets->createFromRenderArray($this->content);

This should be
$this->attachedAssets = AttachedAssets::createFromRenderArray($this->content);

And then the bug you mention should disappear.

fixes #39
Tagging for office hours

right

more cleanup - drupal_merge_attachments can't be used in a unit test, so moved it to a static method on AttachedAssets and deprecated the old function (retained as a wrapper for now)

W00t. I just did a manual test to pick up the issue reported a few times around image upload. I'm not sure how to check for the bigger issue here, but this clearly nails

• #2349629: Insert image popin breaks when trying to add alt text
• #2347175: Omitting required alt text in ckeditor image upload first time causes wrong redirection after saving
• #2373703: Image upload via WYSIWYG modal incorrectly redirects
• #2409371: Can't upload an image using ckeditor after failing to add alt text
• #2369151: Errors in wysiwyg image modal forms break the go to url after submit

I reviewed the code and it seems to be all good. With @kattekrab's manual test in #47 I'm going to stick a fork in this.

Awesome progress here! There is one thing I'm afraid I have to disagree with though:

drupal_merge_attachments can't be used in a unit test, so moved it to a static method on AttachedAssets and deprecated the old function (retained as a wrapper for now)

It's the right thing to do, but the wrong place for that method. It should live on Renderer. dawehner moved it there in his patch in #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests, and I agree with that. This reroll moves it there, but since it's a pure move operation, keeping at RTBC.

Also fixed 3 nitpicks (one missing newline, and two times "Definition of …" instead of "Contains …").

It's the right thing to do, but the wrong place for that method. It should live on Renderer

yep, took a stab in the dark, +1 to the move

Maybe a couple things that could be fixed on commit and/or ignored nitpicks.

1. +++ b/core/lib/Drupal/Core/Ajax/CommandWithAttachedAssetsInterface.php
   @@ -2,7 +2,7 @@
    
   
   +++ b/core/lib/Drupal/Core/Ajax/CommandWithAttachedAssetsTrait.php
   @@ -2,7 +2,7 @@
   - * Definition of Drupal\Core\Ajax\CommandWithAttachedAssetsTrait.
   
   +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -304,4 +304,48 @@ public function getCacheableRenderArray(array $elements);
   +   * Merges two attachments (#attached) arrays.
   ...
   +   * rather than re-indexed as is common in PHP array merging.
   ...
   +   *   Another attachments (#attached) array.
   

   The parenthesis feels a bit strange here, I'd be curious if jhodgdon has any input on this? Maybe just use the word 'via' or 'through' to help convey the attachments are on the '#attached' key of the render array.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -582,4 +583,19 @@ public static function mergeBubbleableMetadata(array $a, array $b) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +
   +  public static function mergeAttachments(array $a, array $b) {
   

   Extra line break here. Edit: Better context lines.

3. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -304,4 +304,48 @@ public function getCacheableRenderArray(array $elements);
   +   * match the behavior of
   

   Should take a colon at the end of this.

Fixed all nitpicks in #51.

1. The issue summary is needs updating. Stuff TBD and

   drupal_merge_attachments() deprecated in favour of AttachedAssets::mergeAttachments

   I think that is Renderer::mergeAttachments() and we should have a CR to announce the deprecation.

2. Is the needs tests tag still relevant?

3. +++ b/core/includes/common.inc
   @@ -889,15 +891,12 @@ function drupal_js_defaults($data = NULL) {
   + * @deprecated To be removed in Drupal 8.0.x. Use
   + *   \Drupal\Core\Render\Renderer::mergeAttachments() instead.
   

   Let's deprecate this for Drupal 9. We're in beta and there is no compelling reason to remove the function. We can still convert all the usages.

@Wim Leers pointed out in IRC that drupal_merge_attached was adding in D8 therefore I've changed my mind - it is okay to remove before d8 is released.

1. I don't think this needs its own CR, because
   • A) there isn't an explicit change notice for its introduction either, I can just update https://www.drupal.org/node/1911578 (which I just did)
   • B) only the most advanced code will need this — things like drupal_pre_render_links()
2. Indeed, this is indeed still relevant. Fixed in this reroll: changes DialogTest's test code so that both the automatic rendering of render arrays and automatic attaching of assets are exercised. Explicitly testing this would be even better, but this should be sufficient. Keeping at RTBC because it's a tiny change that Alex will either find sufficient test coverage or not.
3. Already handled by #54 :) No changes necessary.

Still needs an issue summary update to fix the TBD sections.

Updated the issue summary with the beta eval and fixed one TBD and removed the other.

Just a small leftover in the IS.

Re ran manual test. Still works. Still RTBC in my view.

Committed 8ee0358 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Huzzah!! Such collaboration. :)

Thanks everyone.

and also personal milestone... my 10th commit credit.