Don't render status messages if there are no messages but also include their assets if there might be

HAHHAHAHAHAHAH. Genius.

#3 means we save about 1000 function calls on every HTML response. Including BigPipe responses. Including Dynamic Page Cache hits. But excluding Page Cache hits (since those don't do any rendering ever).

I wonder if it means we save 25 out of 80 milliseconds too? Also, WTF, do we really spend 25 ms on rendering an empty status messages template? How is this possible?

It'd be interesting to see how this impacts performance ofan authenticated response of the front page with 5 articles, and of a full node page.

Yep this is a great find, massive +1.

I was concerned for a moment about #77245: Provide a common API for displaying JavaScript messages but this doesn't affect that issue either way.

Great find.

We also agreed to backport this in the 8.3.x beta phase. It's a totally BC-safe improvement and any disruption from not having the empty render array is quite outweighed by the performance improvement, especially before 8.3.0. Thanks!

YAY!

It breaks a lot of messages. (BC-BROKEN)
#2875947: CSS files not loaded on AJAX calls

A simple way to reproduce:
1. admin/structure/menu/manage/account
2. change menu link ordering

The patch only affected a file and can be reverted easily. So I reopen it.

FYI : #77245: Provide a common API for displaying JavaScript messages will need all the messages stuff on every pages, even when there are no messages (on the PHP end).

I've reverted this issue. At a minimum, it should to be updated to include the fix from #2875947: CSS files not loaded on AJAX calls and tests for said fix. If someone can incorporate the info from the duplicate, that would be helpful.

@nod_, is there a way that #77245: Provide a common API for displaying JavaScript messages can be supported alongside the fix here? It really is quite a nice performance improvement.

As an aside:

It's a totally BC-safe improvement

was really, really quite wrong. ;)

Proposed new scope. :P

Not sure we can.

We do need the wrapping HTML to be generated from twig by PHP, so we can inject individual messages in the message area in the frontend. I guess we could only render it if the core/drupal.message is used on the page, by I don't know where or how to check that.

A great catch!

But I'm with @nod_, it's not trivial to get the .css dependency hell fixed and we always need to render the messages area for the JS Messages API to work. The easiest fix for that is to just add messages.css to the themes base libraries (see discussion here: #77245-227: Provide a common API for displaying JavaScript messages).

Is there another way to improve speed? Move {{ attach_library('classy/messages') }} down, so to only include the CSS when there are actual messages maybe? Statically cache some functions while there are no actual messages?

It think we should re-title this issue to something like "improve messages performance". And maybe reopen the previously closed duplicate. Otherwise this will become a heavy blocker for JS Messages API, which in turn is needed for Inline Form Errors and Outside in / Settings tray.

Quick review, will do some extra later.

1. +++ b/core/tests/Drupal/FunctionalTests/Theme/BartikTest.php
   @@ -0,0 +1,42 @@
   +  /**
   +   * Tests the Bartik theme.
   +   */
   +  public function testBartik() {
   

   Needs better description and method name.

2. +++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
   @@ -0,0 +1,41 @@
   +   * Tests the Classy theme.
   +   */
   +  public function testClassy() {
   

   Same as above.

3. +++ b/core/themes/classy/classy.info.yml
   @@ -8,6 +8,7 @@ hidden: true
   +  - classy/messages
   

   Woohoo this helps JS Message API.

Manually tested the patch, messages.js present, even without messages on screen, in Bartik and Classy.

+++ b/core/modules/system/system.post_update.php
@@ -65,3 +65,9 @@ function system_post_update_hashes_clear_cache() {
+function system_post_update_classy_message_library() {
+}

Like all other empty post update functions, add: "// Empty post-update hook."

The rest of the logic was already RTBC-ed in #2875947: CSS files not loaded on AJAX calls.
So after these nits are fixed we are good to go it seems.

I mentioned it (#24) because of our own documentation:

Add test cases by adding method names that start with 'test' and have no arguments, for example testYourTestCase(). Each one should test a logical subset of the functionality.

https://api.drupal.org/api/drupal/core!core.api.php/group/testing/8.2.x

But, that (separating subsets) also has a negative speed impact, so I'm ok with keeping it like this for now.

From #22

the twig templates print nothing is there are no messages

Yes, the other patch changes that. And anyway the other patch not ready yet: the data attribute is still in the wrong place.

And I'd be really curious why an empty template add a 10% overhead on logged pages, cache busting going on?

I guess we can always duplicate the template in js. we'll need to add tests to check all this in the other issue.

And I'd be really curious why an empty template add a 10% overhead on logged pages, cache busting going on?

I haven't profiled this yet, but there's a few things going on with the messages block specifically:

1. This is rendered in a special cased placeholder, so that it always runs as late as possible (to pick up as much from $_SESSION as it can). It's not cached, although it could probably be cached by a 'session contains messages' cache context then max-age=0 when it contains nothing (which I don't think we should implement here).

2. With dynamic page cache, at least theoretically this could be the only (or one of few) template getting rendered as opposed to pulled from cache, so there may be some overhead of loading templates, other caches etc. that in 7.x would have happened whether it gets rendered or not.

Profiling would be useful, I've opened another issue to discuss that and maybe refactoring how this works more #2877291: The late rendered messages block can take 10% of page generation time.

Happy with the patch I think, but want to give it another look over after food/caffeine before commit.

So, the issue is still filed against 8.3.x, but now also adds a template and library change for Classy. So less sure about backporting it, especially after 8.3.0. I mean it's not going to get more broken than it was in 8.3.2 and below, I guess, but still. :P I'll get a second opinion.

+++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
@@ -0,0 +1,41 @@
+   * Tests the Classy theme.
+   */
+  public function testClassy() {

This is way more than the test does... I mean, I guess other than asserting the theme by itself doesn't cause a 500 or something. :P

Apparently this was also mentioned in another review and @alexpott responded to it in #26, but I still think it's kinda a bad/misleading method name barring some specific test coverage we want to add to the method in a followup.

Thanks for adding the test coverage for this!

From the Classy point of view backporting this change is fine. This change doesn't cause virtually any changes to the actual rendered page. Previously the library was only attached when messages were rendered. But because messages were rendered on every page, the library was already loaded on every page.

A minor difference I could find is that there is a change in the order of the Classy CSS files. This could potentially cause a problem if someone was overriding some of the other classy component styles by replacing messages library. Personally, I wouldn't worry about this since this is an anti-pattern but would require still quite a lot of knowledge how Drupal's assets work. There are a gazillion better alternatives to achieve same.

At last, there is no harm caused by the fact that overridden templates will be still calling attach_library there since a library can be only attached once for the page.

Thanks @lauriii!

@alexpott:

yes the test don't cover all the functionality of those themes but in an ideal world those themes would already have tests and this would be a simple one liner addition to them.

I understand your perspective and your frustration with people adding unneeded separate methods, and that you want them to reduce test install time by adding to existing tests, but this is not the effective way to accomplish that. Without a specific idea of what test coverage we might add, and @todos and followups for their coverage, it's just overpromising and can confuse people into thinking classy already has test coverage so they don't need to write it.

If you think Classy and themes in general need test coverage of some sort, file an issue and put it in your test in a @todo. I have no concept of what test coverage themes need but lack, so if you feel they're lacking, then outline a few things you think should be tested in at least a stub followup that frontend maintainers can expand if they choose.

Or just rename the method like a couple people have suggested. :P I don't think having the method named testClassy() is going to increase the likelihood of tests being added in that method and not another by someone else.

The update hook point is reasonable; while the docs say "changes requiring an upgrade path" and this is just a cache clear, I can see wanting to avoid that update hook, especially since we had so many problems with them with the minor update. However, yesterday I was told this issue was critical because we're saving the earth. :P If this issue is really critical and that's the only disruption per @lauriii, I can see a case for backporting it, to save the earth five months earlier.

I did confirm that there are no other updates yet since the minor:

[ibnsina:maintainer | Fri 05:53:52] $ git log --since=2017-04-05 -S_update_
[ibnsina:maintainer | Fri 05:54:11] $ 

For reference, these 8.2.x issues may have had an update hook after 8.2.0:

[ibnsina:maintainer | Fri 06:00:17] $ git log --since=2016-10-05 -S'_update_' --pretty=oneline
12fbebb1787c26be73f2379df0b61113a800298f Issue #2840595 by amateescu, Berdir: The 'Source feed' field of aggregator items has to be updated and marked as required
6663917e5c3e2d614ae98777d80dae1c754b94e2 Issue #2623568 by yanniboi, claudiu.cristea, himanshugautam, anil280988, sidharthap, Sagar Ramgade, dawehner, tstoeckler: Config schema of argument_default plugins is incorrect
607ae50a78e95c3fff25033b96131bc5901e8fb2 Issue #2828559 by amateescu, alexpott: More random fails in UpdatePathTestBase tests: "settings.cache failed with: missing schema"
c5da97f9e7e8f145541c5422e3ccde6a8b5ae680 SA-CORE-2016-005 by larowlan, xjm, David_Rothstein, Dave Reid, Crell, cilefen, alexpott, mlhess, catch, pwolanin, YesCT, dawehner, quicksketch, Heine, znerol, charlotte.b, jnicola, ezraw
da66644e8124ceaf5057264dc37f675d629ed4c5 Issue #2796151 by mpdonadio, jhedstrom, myLies, vijaycs85, lomasr, alexpott, Gábor Hojtsy, penyaskito, catch, xjm: Date range separator should be translatable
ea275ec1396fee69193585de0360537beba3bfd3 Issue #2811519 by tim.plunkett, dbourrion, cilefen, futurmat: Blocks do not appear after being placed with the Rules module enabled (or other missing schemata for Condition plugins)

Some of those are for criticals, some are for experimental modules, but the last couple (at the top) are normal issues.

Edit: #2828559: UpdatePathTestBase tests randomly failing is a false positive; apparently I can't figure out how match grep with -S properly. But I checked the others and they are.

I agree that testClassy/testBartik is not a good name. Its not better than "test". For me tests fall under a different category of how to write them than normal code for example you can even throw them away if it actually adds more maintenance burden that profit.

From my point of view adding a new test class for bartik/classy is a good step in the right direction. It provides more context.
Regarding the method names, as we have seen in #17 we are dealing with a regression, so something like testRegressionMissingMessagesCss on both classes would cut it for me at least. One property of regression tests is that they can be way more specific than other tests, as they target a specific problem, rather than provide a descriptive test case, which is one of the best practises of functional tests.

I did offer a strategy to accomplish what you want in #39.

#42 seems a reasonable case not to backport; I can see that causing a disruption for some small set of sites and the fewer disruptions we put between them and sec releases the better. But let's be consistent then and not backport upgrade paths for normals.

(Comment below was a crosspost).

+++ b/core/tests/Drupal/FunctionalTests/Theme/BartikTest.php
@@ -28,9 +28,9 @@ public function setUp() {
+   * Tests the Bartik theme always adds classy's and its message CSS.

+++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
@@ -28,9 +28,9 @@ public function setUp() {
+   * Tests the Classy theme always adds it's message CSS.

There are some typos in these:
- "Its" and not "it's"
- capitialize "Classy"
- The Bartik one is confusing and sorta ungrammatical; maybe "Tests that the Bartik theme always adds its message CSS and Classy's.

Edit: also "Tests that" in the case of these sentences.

+++ b/core/tests/Drupal/FunctionalTests/Theme/ClassyTest.php
@@ -28,7 +28,7 @@ public function setUp() {
   /**
-   * Tests the Classy theme always adds it's message CSS.
+   * Tests the Classy theme always adds its message CSS.
    */
   public function testRegressionMissingMessagesCss() {

This diff just proves that adding single line documentation to test methods is a bit weird in the first place.

+++ b/core/tests/Drupal/FunctionalTests/Theme/BartikTest.php
@@ -0,0 +1,42 @@
+    // Use the classy theme.
...
+    // Ensure that Classy's message.css is loaded.
+    // @see classy.info.yml

C&P I think and should say "Bartik theme" and mention that it's testing Bartik also?

Also yes to #49 but that is a battle for another day. ;) As @msonnabaum always said, we often use docs to do what the code itself should. :)

+++ b/core/tests/Drupal/FunctionalTests/Theme/BartikTest.php
@@ -0,0 +1,43 @@
+    // Use the classy theme.
+    $this->assertTrue($this->container->get('theme_installer')->install(['bartik']));

Missed a bit here I think. Could be fixed on commit but letting tests run also.

We discussed this on the committer triage call last week, and decide it should stay critical (although also that we need to further update the definitions for 'performance critical' on https://www.drupal.org/core/issue-priority now that 8.x is released).

Added reviewer credit from #2875947: CSS files not loaded on AJAX calls and this issue.

One the one hand the change here is minimal and is already in 8.3.x in a broken form. On the other, don't really like having to add an update to a patch release. So I'm going to mark this as fixed, but if someone feels it should be backported to 8.3.x, please re-open.

LOOL That's some other graber you credited :) I'm here not for the credits though, hope we'll meet again, cheers!

Doh. Fixed on the issue credit anyway.

Case-sensitive usernames FTW. ;)

Ding!

COMMAND: git log -n 1 6b489d5 | grep droplet
CreditGo: She forgot you
...
(UP arrow, Press enter)
..
.

Dear @drupal,

Do you know? I re-compiled uncountable builds to identify the problem between 400 commits. No matter you acknowledge me or not. I will continue to help your back to health.

Adding credit for duplicate, #2601232: Follow-up for #2566597: only attach the 'classy/messages' asset library if there are messages