Problem/Motivation

Often one of the first four tests often fails on DrupalCI when using postgres. Whilst this is obviously is an issue for DrupalCI the way in which it fails shows that we have a problem in the installer.

Failing tests:

Action.Drupal\action\Tests\ConfigurationTest

✗ testActionConfiguration
   fail: [Completion check] Line 31 of
core/modules/action/src/Tests/ConfigurationTest.php:
   The test did not complete due to a fatal error.

✗ getDefinition
   exception: [Uncaught exception] Line 796 of
vendor/symfony/dependency-injection/ContainerBuilder.php:
   Symfony\Component\DependencyInjection\Exception\InvalidArgumentException:
The service definition "renderer" does not exist. in
Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line
796 of
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php).
Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('renderer')
   Symfony\Component\DependencyInjection\ContainerBuilder->get('renderer')
   Drupal::service('renderer')
   install_database_errors(Array, './sites/simpletest/943579/settings.php')
   install_verify_database_settings('sites/simpletest/943579')
   install_begin_request(Object, Array)
   install_drupal(Object, Array)
   Drupal\simpletest\WebTestBase->doInstall()
   Drupal\simpletest\WebTestBase->setUp()
   Drupal\simpletest\TestBase->run(Array)
   simpletest_script_run_one_test('5',
'Drupal\action\Tests\ConfigurationTest')

The backtrace shows that the early use of the renderer in install_database_error() is problematic where the database settings are available for verification during the first call to install_verify_database_settings() from install_begin_request().

We are using the renderer for several reasons - to format an item list and to use an inline twig template.

Proposed resolution

Check if the renderer service exists and only use it if it does - install_verify_database_settings() only cares if the array returned is not empty - it does not care what is in it.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#14 2662552-2-14.patch4.78 KBalexpott
#11 2662552-11.patch3.29 KBdawehner
#5 2662552-5.patch1.09 KBalexpott
#5 2-5-interdiff.txt833 bytesalexpott
#2 2662552-2.patch1.07 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2662552-2.patch1.07 KB

Unfortunately this is completely untestable because it is super super early installer. But making this change might allow us to see what is going wrong with Postgres occasionally in the first tests run on DrupalCI.

Status: Needs review » Needs work

The last submitted patch, 2: 2662552-2.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review 
+++ b/core/includes/install.core.inc
@@ -1139,7 +1139,11 @@ function install_database_errors($database, $settings_file) {
+    // install_verify_database_settings() and not have a renderer service

not having.

And wouldn't if ($errors) be enough?

But yes makes sense.

Error looks like the /vendor fallout.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2-5-interdiff.txt833 bytes
2662552-5.patch1.09 KB

Hmmm... not my best sentence ever. The if (count($errors) is not really part of the part - kinda out-of-scope imo.

Reported the /vendor issue to Mixologic.

Status: Needs review » Needs work

The last submitted patch, 5: 2662552-5.patch, failed testing.

The last submitted patch, 5: 2662552-5.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Needs review

Looks like a good fix to me if we can get it through an actual test run.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2501835: Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() and ensure multiple messages are printed

So this was introduced in #2501835: Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() and ensure multiple messages are printed.

@alexpott and I discussed this patch. The problem is that install_database_errors() is used in two places, both SiteSettingsForm and install_verify_database_settings(), which happens very early in the installer. Part of the issue is that the return value of install_database_errors() is undocumented -- is it supposed to return just a list of errors, as install_verify_database_settings() seems to expect? Or is it specifically a form error array of error markup, as SiteSettingsForm expects? :P

install_verify_database_settings() does not actually need any of the markup or even the text for the errors -- it just needs to know whether or not there were errors. So, we were wondering if we could move the actual markup/render building into SiteSettingsForm or something similar.

@alexpott also pointed out that one of the other code paths in install_database_errors() runs Drupal\Core\Database\Install\Tasks::validateDatabaseSettings(), which also builds renderable/markup objects (in that case t() strings) into a form error array structure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

We also have a potential problems with drush installation. If you add db information to settings.php with a incorrect prefix - for example blah blha then it'll produce the same error when you do drush si. However even with this fix the error you get when you do this is not helpful. You get Drupal\Core\Installer\Exception\AlreadyInstalledException - funky :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2662552-11.patch3.29 KB

Its a bit hard to describe what I think, but I disagree with this fix. We are hiding errors. One example I played with is to set bad database credentials in settings.php.

An alternative approach could be to have a custom object which can be used as early as possible.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@dawehner we're not hiding errors ... 

function install_verify_database_settings($site_path) {
  if ($database = Database::getConnectionInfo()) {
    $database = $database['default'];
    $settings_file = './' . $site_path . '/settings.php';
    $errors = install_database_errors($database, $settings_file);
    if (empty($errors)) {
      return TRUE;
    }
  }
  return FALSE;
}

This method is called before the container is set up. And before errors could be presented to the user. And it only counts the errors to work out if there is an error.

Status: Needs review » Needs work

The last submitted patch, 11: 2662552-11.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2662552-2-14.patch4.78 KB

Believe or not we actually have test coverage of the inline template used here... see Drupal\system\Tests\Installer\InstallerTranslationTest

Discussed at length with @xjm and a bit on IRC with @dawehner. Both are keen to move the use of render into SiteSettingsForm. I agree. Currently install_database_errors() does two things; it discovers if there any database errors, it links these errors to form elements. I don't think we should consider install_database_errors() API. It is only, and can only, ever been called by the installer whilst installing drupal.

New approach and therefore no interdiff.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thanks a ton alex! It is nearly always worth to do things the right way

  • xjm committed 39c9d54 on 8.1.x 
    Issue #2662552 by alexpott, dawehner, xjm, catch: Early use of render...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks @alexpott. That is much cleaner and the added documentation also will make this easier to understand in the future.

My one concern with this approach when @alexpott and I discussed it was that there is technically a slight BC break for install_database_errors(), if it were ever used outside of these two places in core. We're changing the expected contents of the return array. @alexpott and I tried to think of any usecase for this function outside of core, but could not come up with one. According to the D8 API policy, though, the installer is all considered internal API. I added this change record just in case we overlooked something: https://www.drupal.org/node/2677482

I committed and pushed this to 8.1.x. For 8.0.x, we have a few options:

  1. Do the original patch instead as a hotfix for the test failures.
  2. Leave it as 8.1.x only, since 8.0.x's life is coming to an end soon. (Downside is that it is obscuring other random test failures, etc.)
  3. Decide to break the internal API anyway since we really doubt anything could ever use it, and mention it in the release notes.

I can see cases for all three approaches, so setting PTBP for now for others' feedback. I am leaning toward the first.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs followup

Also, a followup to actually document install_database_errors() would not go amiss.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Patch (to be ported) » Fixed

Since 8.0.x is about to be EOL, marking this fixed against 8.1.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.