Problem/Motivation

Discovered at #2469431-153: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

This affects BigPipe and all AJAX responses that render messages (think dialogs' form errors).

Proposed resolution

Only attach the asset library when there are actual messages being rendered, i.e. when there is some markup that the CSS can apply to.

Remaining tasks

Review & RTBC.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Something like this.

I think we want test coverage. Especially because I don't even understand why this works. Note that we don't check if there are in fact messages; we simply move this from outside the block to inside the block. Seems like there's some special Twig stuff happening that makes this only be invoked in case a Twig block is not empty?

Wim Leers’s picture

Status: Active » Needs review
Fabianx’s picture

Interesting, yes tests would be good for this.

Wim Leers’s picture

Issue tags: +php-novice, +Novice

Tweeted about this, hopefully somebody wants to write the tests for this: https://twitter.com/wimleers/status/659679378096410624 :)

For this test, we need a WebTestBase test that:

  • loads a page without a message, asserts that the messages CSS
  • is not
  • loaded

  • loads a page with a message, asserts that the messages CSS is loaded
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Twig

Thanks!

Dom.’s picture

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

Wim Leers’s picture

Status: Needs review » Needs work

Yes!

But I'm confused why this test passes. I suspect it's simply not being executed by testbot? (Because it's in the wrong location?) It's asserting that the CSS exists when no message is shown, and vice versa. Which is the opposite of what we want :)

This is definitely the correct track :)

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

    Needs to be updated.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
    @@ -0,0 +1,47 @@
    +  protected $profile = 'standard';
    

    You should be able to just delete this line, as you already indicated.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Common/DrupalSetMessageLoadsCssTest.php
    @@ -0,0 +1,47 @@
    +   * Check that a page does not load message asserts when unrequired.
    

    s/asserts/assets/ :)

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

    Let's be more specific: core/themes/classy/css/components/messages.css.

snehi’s picture

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

Status: Needs review » Needs work
Dom.’s picture

  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

Dom.’s picture

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

Dom.’s picture

  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

Dom.’s picture

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 ?

Status: Needs review » Needs work
Dom.’s picture

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.

Status: Needs review » Needs work
sdstyles’s picture

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

Status: Needs review » Needs work
Wim Leers’s picture

That's because:

  • the test user was the anon user, which does not have a session, and therefore cannot get any messages
  • the message was being set not in the test site, but in the parent site

Fixed those things.


+++ b/core/themes/classy/templates/misc/status-messages.html.twig
@@ -23,7 +23,9 @@
+{% if message_list is not empty %}
+  {{ attach_library('classy/messages') }}
+{% endif %}
 {% block messages %}

This now lives outside of the Twig block.

Is that correct, Twig experts?

Fabianx’s picture

#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’s picture

Status: Needs review » Needs work

Thanks for confirming my suspicion!

Dom.’s picture

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

Cottser’s picture

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.

andypost’s picture

Issue tags: -rc eligible, -php-novice, -Novice

clean-up tags

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.