[regression] Empty messages container appearing when a Notice occurs

I've been seeing this intermittently since Beta13 but I haven't tried to debug it yet.

If #4 is true, then #2543332: Auto-placeholdering for #lazy_builder without bubbling can't be the cause.

I wonder if this is some strange caching problem? Can you please make sure all caches are cleared? Better yet, can you post steps to reproduce on simplytest.me?

I've just reinstalled D8 HEAD to try to reproduce this, but I really can't. :(

I had this issue last night as well, I just did a fresh re-install and it's still there for me at 1569740d0caf3570aeb414a5bbf767bcabca86e4

Screenshot attached

I updated to HEAD. I can repro easily on my local by clearing the cache with drush and then viewing any page. It looks like it shows up on all content pages the first time I load them after clearing the cache. Then is goes away on subsequent node views.

Steps to reproduce:

1. Make sure you're on the latest, clean 8.x code:

git reset --hard
git pull --rebase

2. Install using Drush:

$ drush si --account-pass=admin -y

3. Open some browser you never use (in my case, Safari) in private browsing mode to eliminate caching issues

4. Go to the site front page. Voila.

Given it's a regression, this should probably be critical?

I guess we could make it critical, though I'd be very hard-pressed to not ship D8 if for some reason this was the last one.

However, in reviewing the critical definition: https://www.drupal.org/node/45111#critical it doesn't seem to match any of that criteria. (There's no data loss, security issue, etc. and there is a workaround.) Definitely ugly, user-facing bug tho.

I finally managed to reproduce it.

Requirements to reproduce

This only happens if both of these requirements are met:

1. you DID NOT update your site's sites/default/services.yml file (during the installation, this file is copied from sites/default/default.services.yml), i.e. you still have one without the auto_placeholder_conditions key in the renderer.config container parameter
2. and you ARE NOT using a settings.local.php (which sets $config['system.logging']['error_level'] = 'verbose';)

Circumstances in which you cannot reproduce it

• If your site does not meet requirement 1, then you don't have any problems at all. (i.e. totally fresh install)
• If your site meets both requirements, then you have the screenshot shown in #10
• If your site meets requirement 1 but not requirement 2 (i.e. you are using settings.local.php), then you get an actually useful message: Notice: Undefined index: auto_placeholder_conditions in Drupal\Core\Render\Renderer->shouldAutomaticallyPlaceholder() (line 656 of core/lib/Drupal/Core/Render/Renderer.php)., plus a stack trace.

Root cause

This is happening because sites/default/services.yml overrides core.services.yml's container parameters. And because a non-updated services.yml contains ['required_cache_contexts' => […]] as the container parameter, whereas an updated one would contain ['required_cache_contexts' => […], 'auto_placeholder_conditions' => […]], this causes problems.

Because then the compiled container contains this:

            'renderer.config' => array(
                'required_cache_contexts' => array(
                    0 => 'languages:language_interface',
                    1 => 'theme',
                    2 => 'user.permissions',
                ),
            ),

So, no 'auto_placeholder_conditions' key. And hence this causes a notice: Undefined index: auto_placeholder_conditions.

But why no notice and that strange brokenness instead?

1. So, when that PHP notice occurs, _drupal_error_handler() is called. This is Drupal's error handler.
2. That, in turn calls _drupal_error_handler_real(), which generates an "error array", which contains the error type (notice), error message (the undefined index notice), file, line number and backtrace.
3. Finally, that generated error array is passed on to _drupal_log_error(). This is where things go very wrong.
   1. First, this happens:
      

        else {
          // Display the message if the current error reporting level allows this type
          // of message to be displayed, and unconditionally in update.php.
          $message = '';
      

      i.e. $message is initialized to the empty string.

   2. Then, we check if the error is "displayable".
      

      if (error_displayable($error)) {
      

      This checks the aforementioned $config['system.logging']['error_level'] setting, whose default value is ERROR_REPORTING_HIDE === 'hide'. The default value causes _error_displayable() to return FALSE. So we don't enter this branch.

   3. The next check we encounter is:

          if ($fatal) {
            // We fallback to a maintenance page at this point, because the page generation
            // itself can generate errors.
            // Should not translate the string to avoid errors producing more errors.
            $message = 'The website encountered an unexpected error. Please try again later.' . '<br />' . $message;
      

      … but because this is just a notice, and not a fatal error, we don't enter this branch either. Hence $message still equals the empty string.

   4. Therefore, we enter the else-branch:
      

          else {
            if (\Drupal::hasService('session')) {
              // Message display is dependent on sessions being available.
              drupal_set_message(SafeMarkup::set($message), $class, TRUE);
            }
            else {
              print $message;
            }
          }
      

      This then finally sets that message… which is just the empty string.

   This bug was introduced by #2509898: Additional uncaught exception thrown while handling exception after service changes, which first appeared in beta 13. Which confirms what @Jaesin said that he's been seeing since then, in #4.

   Before the changes in that issue, we only ever generated output for a displayable error OR for a fatal error. Now we pretend like we generate output always, but we actually don't. Hence this bug. The first patch to contain this bug is #2509898-21: Additional uncaught exception thrown while handling exception after service changes.

   Attached is the solution — very simple :) (Though I may have missed something.)

Applied the patch, repeated steps in #10, no more box!

So #14 fixes the reported bug.

But, arguably, that bug is just a symptom. The real problem here is that core's container parameters have been updated, but updating from one beta to the next WITHOUT updating your site's services.yml can clearly cause big problems. How are we going to fix that problem?
That feels like it needs its own issue.

Tagging for #16, until that's moved into a separate issue.

To help people upgrade their existing D8 beta sites correctly, I created a CR that will help prevent the notice: https://www.drupal.org/node/2547351.

I can confirm that after blowing away my old D8 install and git cloning from scratch I no longer am seeing this error.

The patch in #14 works for me weather or not I update "renderer.config" my services.yml file.

Updating my services.yml file without the patch does not fix this issue I am experiencing as described in #9.

So, #16 was discussed on the D8 EU criticals call. Opened a new issue for it: #2547447: Stop creating services.yml by default.

So now this issue is again solely about fixing the bug in errors.inc!

#20: That sounds impossible :). You probably forgot to rebuild your container (drush cr)?

Also seen this error recently. Patch look fine to me! :)

Working on a test now.

Oh I've seen that for quite a while and was aways wondering whether the hack this was coming from.

I still get an empty, rendered highlighted region, but that may be unrelated?

Yes, see #953034: [meta] Themes improperly check renderable arrays when determining visibility.

1. +++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
   @@ -191,4 +192,14 @@ function assertNoErrorMessage(array $error) {
   +    return $this->assertFalse($this->xpath('//div[contains(@class, "messages")]'), 'Ensures that also no messages div exists.');
   

   s/exists./exists, which proves that no messages were generated by the error handler, not even an empty one./

   -> otherwise the test coverage looks extremely vague.

2. +++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
   @@ -191,4 +192,14 @@ function assertNoErrorMessage(array $error) {
   +  }
    }
   

   Missing \n.

So, what you're saying is that when I pull updates I, and get that stupid permission denied error on default.services.yml, I shouldn't just yell at the computer and fix the permissions? I should actually see what changed in the file? You've gone too far.

:P :D

Closed #2539200: PHP errors cause empty messages to display when display errors is off as a duplicate.

Also back to Needs work because the feedback from #27 hasn't been implemented yet.

Attached patch implements the feedback from #27.

No interdiff, because I don't have a development environment on this laptop and manually edited the patch.

Looks great, thanks!

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed cd7e7ce and pushed to 8.0.x. Thanks!

Yay!