Follow-up for #2566597: only attach the 'classy/messages' asset library if there are messages

Interesting, yes tests would be good for this.

Thanks!

@Wim Leers : Are you waiting for something this kind ?!
If yes, then the remaining @todo whould be I guess:
- validate you are okay with where to put this test files
- change the test to work with a lighter profile (ie testing)

Done as suggested in #8 except 1.
What to write in case of 1.

1. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
   @@ -0,0 +1,46 @@
   + * Tests CSS classes on comments.
   

   This tests actually :
   "Tests that message assets get loaded only when needed".

2. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
   @@ -0,0 +1,46 @@
   +    $this->assertNoRaw("message.css", "Message CSS assets should not be loaded.")
   

   This needs also to be as specific as core/themes/classy/css/components/messages.css

+++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
@@ -0,0 +1,46 @@
+  /**
+   * The profile to install as a basis for testing.
+   *
+   * @var string
+   */

Also because I suggested changing from standard profile in test to testing, Wim Leers said to remove such lign.
But it also takes:
1: to remove associated comments.
2: to make sure the message block is placed on a region by default, which I am not sure about...

1. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
   @@ -0,0 +1,46 @@
   +    $this->assertRaw("core/themes/classy/css/components/messages.css", "Message CSS assets should be loaded.")
   

   ';' is missing

2. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
   @@ -0,0 +1,46 @@
   +    $this->assertNoRaw("message.css", "Message CSS assets should not be loaded.")
   

   ';' is missing

In this comment, I'm including a new test-only patch (with interdiff).
Also including same patch with fix by Wim #2 included.

@Wim Leers: do you want us to continue only with usual patch file fix+tests in next comments or test-only ?

In order to check if CSS files is included or not from source code analysis, it takes to unactivate default css aggregation first.
Also test was upside-down: messages.css should not be loaded when no message is display, but loaded when there is a message.
Correction in patch attached.

Seems core/themes/classy/css/components/messages.css is added even a message doesn't exist.

#21: That should be wrong.

A template 'A' using extends on 'B' is just:

class A extends B {

  function block_messages() {
  }

}

So the render function of template B is used, which then calls block_messages() - which is why there is {{ parent() }} command within blocks ...

@Wim Leers and @Fabianx :
I got confused on what's remaining to do here. Could you elaborate ? Is it still Novice ?

It seems like it's fine being outside the Twig block, and that is how it was before. Maybe I'm missing something though. Moving the attach_library() inside the Twig block would IMO be considered a BC break as well, for example I think it would be a regression for Bartik (let's pretend Bartik was a contrib/custom theme for a moment).

+++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
@@ -0,0 +1,53 @@
+ * Contains \Drupal\KernelTests\Core\Common\DrupalSetMessageLoadsCssTest.
...
+namespace Drupal\KernelTests\Core\Common;
...
+class DrupalSetMessageLoadsCssTest extends WebTestBase {

I was wondering why this is in KernelTests if it's a WebTestBase test. I think maybe because it's right next to DrupalSetMessageTest :) it should be moved, /core/modules/system/src/Tests/Bootstrap is probably closer to where we want it.

clean-up tags

Doing Bug smash triage.

Dig some digging and it looks like that was fixed in a later issue #2853509: Don't render status messages if there are no messages but also include their assets if there might be.

Closing as a duplicate and I'll add credit over there.

Thanks.