An empty status message in the AJAX response is added to the commands[] array and added to the DOM as an empty div tag

Subscribe. #1 looks good to me, though jessebeach and I worked on that together, so it needs a review from someone else before RTBC.

Most of the time, there aren't any status messages, and there's no reason to so commonly issue an empty prepend().

For reasons explained inside Drupal.ajax.prototype.commands.insert() in misc/ajax.js (not touched by this patch), the empty string is wrapped in a DIV. Possibly the Drupal.ajax.prototype.commands.insert() function should be made to check for empty string and treat that case specially (no div wrapper, no effects, no attachBehaviors()), but that's a separate issue: regardless of whether empty inserts should be supported in the abstract, there's no reason for core to create the condition.

Nicely spotted @jessebeach! I confirm that the patch works on Drupal 7, which has the same problem. This code is trivial and identical in both D7 and D8. Even though I would have used !empty() instead of != '' I have two enthusiastic thumbs up for the patch. I'd mark it RTBC but I think my clout has run dry.

Thanks! This is just a reroll for D8's "core" folder. No substantive change, so RTBC.

Is it worth adding a test for this? Looks like it could be done.

Good point.

Like pointed out in#2, it's ultimately a JS issue: #1031600: AJAX requests cause duplicate, invalid CSS includes and multi-nested response data output anyway the fix totally makes sense.

4: ajax-empty_status_messages-1237012-4.patch queued for re-testing.

copying skitch files to local

Ajax.inc file removed from 8.0.x branch. I guess, no further action on this issue. Working on Backport to D7

D7 Patch created

Can someone help how can I test D7 patch here? This is my first backport patch

Moving back to 8.0.x, because per #5, this still needs tests. Looks like this was fixed as part of #1843220-48: Convert form AJAX to new AJAX API based on a review comment in #1843220-35: Convert form AJAX to new AJAX API, but no test was added for it there. Possibly some test coverage was added since, so here's a patch to try to expose such coverage if there is any.

Additionally, the if statement commented out in #21 is no longer a sufficient fix for this issue, even if there is test coverage for it, because the Classy theme outputs a newline instead of an empty string when there are no messages. Here's a patch with one possible fix for that.

Possibly we should create a spin-off issue for it, especially if there are any non Ajax related problems with a newline instead of empty string as a status message output.

The commented out "fix" in 21 is not a fix. Clearly the issue is that PrependCommand and removing the check will make it be used every time no matter what. But since the return of $this->drupalRenderRoot($status_messages) is never empty that condition will never work.

A quick fix is to trim. $output = trim($this->drupalRenderRoot($status_messages)); .

I didn't test the patch in #22, but since it's only in the classy theme I can safely assume that it doesn't fix the problem since I experienced this in Seven.

There shouldn't be any problems with empty lines in HTML. This is only an issue with that AJAX command sent on every request no matter if there are messages or not.

If we want to trim everything we need to return a trimmed output from Renderer::doRender();

Let's find out what @effulgentsia thinks about that.

+1 to #26, because yeah, I think it's totally reasonable to assume that whitespace isn't a meaningful status message list. I queued #21 for a retest, because I still want to make sure we have sufficient test coverage both for cases where $this->drupalRenderRoot($status_messages) returns empty and for cases where it returns only whitespace.

+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -72,7 +72,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    $output = $this->drupalRenderRoot($status_messages);
+    $output = trim($this->drupalRenderRoot($status_messages));
     if (!empty($output)) {
       $response->addCommand(new PrependCommand(NULL, $output));
     }

I wonder if we should check emptiness on the trimmed value, but in the case where it's not empty, output the untrimmed value. I know that in #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs, we're running into separate problems of trailing whitespace, and in the course of solving that issue, we may decide to trim in some/all places, but for purposes of this issue, I wonder if we should avoid removing whitespace that a template may have decided to put in there intentionally (which I realize is unlikely given that status messages usually get themed with block-level HTML elements).

since it's only in the classy theme I can safely assume that it doesn't fix the problem since I experienced this in Seven

Seven uses Classy as a base theme, so fixing Classy fixes Seven too. But regardless, #26 is better.

But the only way a template would output

Like the provided patch?

I know that in #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs we're running into separate problems of trailing whitespace,

I think I will provide a patch which doesn't care about text nodes.

Seven uses Classy as a base theme, so fixing Classy fixes Seven too.

I looked then for a base theme value and I saw none. I found out later that's the case.

This is wrong.

No need to run $this->drupalRenderRoot($status_messages) twice.

No interdiff, cause now the patch is actually smaller.

Looks good to me.

#33 could use a comment explaining why we're not doing #26. At least it's not clear to me from the discussion why it's worth making the distinction. @todo for #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs might be enough.

The reason for #33 is

but for purposes of this issue, I wonder if we should avoid removing whitespace that a template may have decided to put in there intentionally (which I realize is unlikely given that status messages usually get themed with block-level HTML elements).

Although now that I think of it, this messages are displayed if the developer didn't add them already, which he should have done. And if he's not happy that the empty space got removed he can add the command himself.

I will provide a patch with a comment.

Need help with what to test and how to test it to be able to wrap up this patch.

+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -73,7 +73,9 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+    $output_trimmed = trim($output);
+    if (!empty($output_trimmed)) {
+      // There are messages so output them exactly as the theme renders them.

We shouldn't use empty() on strings. '0' is a valid status message. Either strlen($output_trimmed) or isset($output_trimmed[0]).

I added a test for an empty message. The test currently in head is actually checking the current behavior and not that the code actually does what it should do.

The interdiff is the test and in AjaxRenderer

- if (!empty($output_trimmed)) {
+ if (!empty($output_trimmed) && $output_trimmed != '') {


Looks good to me.

There's a test coverage!

+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -73,7 +73,9 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    if (!empty($output)) {
+    $output_trimmed = trim($output);
+    if (!empty($output_trimmed) && $output_trimmed != '') {

php -r "var_dump(empty(''));"
bool(true)

So the second part of this conditional is unnecessary.

In fact I think changing this to if (!empty(trim($output))) { as since 5.5 PHP supports arbitrary expressions in empty - https://secure.php.net/manual/en/migration55.new-features.php#migration5...

Re #46: because of #41.

Here is a reroll with simplified condition. We already know that $output_trimmed, as a result of trim(), is of type string.

Even simpler, based on comment #47.

Triaged this as part of bug-smash initiative

I suspect the original issue was specific to the status message theme template in use on Digital Gardens.

Re-rolled the patch because the test still fails without the change.

@effulgentsia do you still want to be assigned here?

Reviewing previous comments... looks RTBC to me since:

1. It was previous reviewed and approved in 44 and 45 other than some minor changes noted in 46 which were addressed in 49 and 59

2. Test-only patch fails as expected

3. Automated tests pass

4. Not sure how to test manually but hoping the new test is sufficient since this is a simple change

Oops, I had forgotten all about this issue and having it assigned to me. Sorry about that.

I'm closing this as a duplicate, because:

+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -72,7 +72,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    if (!empty($output)) {
+    if (!empty(trim($output))) {

When this issue was first opened, the problem was that this if statement wasn't here at all, so an empty div was output regardless of theme. Per #21, that was fixed in #1843220: Convert form AJAX to new AJAX API.

Per #22, $output containing only whitespace was a problem present in Classy and possibly other themes. However, #2853509: Don't render status messages if there are no messages but also include their assets if there might be fixed that for all themes by adding an if ($messages) { statement in Drupal\Core\Render\Element\StatusMessages::renderMessages(). Therefore, there's no need to trim here anymore.

Thank you to everyone who worked on this. It's satisfying to close out a 10 year old issue, even if it's not by committing anything.