Problem/Motivation

Drupal\Core\Database\Install\Tasks::runTasks() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

Remove the call to SafeMarkup::set() by refactoring the code.

The installer uses TaskException to communicate errors to the settings form. It's adding translated messages to the exception and then it trying to list multiple messages. We should not be translating exception messages and all the text should be translatable (it's not currently). The latest patch refactors the installer db tests to not use TaskException since it is completely unnecessary and adds complexity where it is not needed.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.Note: The task messages are fixed strings in code, not user input
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. In core/lib/Drupal/Core/Database/Install/Tasks.php on line 42 change 'CREATE TABLE {drupal_install_test} (id int NULL)', to a SQL comment '-- CREATE TABLE {drupal_install_test} (id int NULL)',
  2. Attempt a clean install of Drupal 8 via the UI installer.
  3. Compare the output of the error messages in HEAD and with the patch applied. Confirm that there is no double-escaping in the error messages and that all the $messages (there should be 4 errors in addition to the intro text) are displaying.

User interface changes

Database fail error on head

Database fail error with patch

API changes

Remove TaskException
Remove db_run_tasks()

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the installer is using translated messages in exceptions and SafeMarkup::set()
Issue priority Major because it is a child of the SafeMarkup critical
Prioritized changes The main goal of this issue is security as SafeMarkup::set() should not be used.
Disruption Potential disruption for alternate database drivers (there are very very few). @chx has confirmed no disruption for Mongodb.
CommentFileSizeAuthor
#72 2501835.72.patch13.15 KBalexpott
#72 65-72-interdiff.txt395 bytesalexpott
#65 2501835.65.patch13 KBalexpott
#65 62-65-interdiff.txt10.92 KBalexpott
#63 2501835-62.patch6.75 KBstefan.r
#63 interdiff-60-62.txt2.9 KBstefan.r
#60 2501835-60.patch4.69 KBstefan.r
#60 interdiff-57-60.txt3.54 KBstefan.r
#57 2501835-57.patch3.66 KBstefan.r
#56 interdiff-52-55.txt2.56 KBstefan.r
#56 2501835-55.patch2.39 KBstefan.r
#52 remove_safemarkup_set-2501835-52.patch3.14 KBjoelpittet
#52 interdiff.txt1.78 KBjoelpittet
#47 screencapture-install-php-error.png1.17 MBkgoel
#45 interdiff.txt2.5 KBkgoel
#45 2501835-45.patch3.25 KBkgoel
#45 Screen Shot-1.png108.37 KBkgoel
#41 Screen Shot.png107.55 KBkgoel
#37 interdiff.txt1.46 KBkgoel
#37 2501835-37.patch3.35 KBkgoel
#35 interdiff.txt1.88 KBkgoel
#35 2501835-35.patch3.3 KBkgoel
#33 interdiff.txt778 byteskgoel
#33 2501835-33.patch2.29 KBkgoel
#22 databasefail.png779.61 KBkgoel
#22 databasefailhead.png585.32 KBkgoel
#22 2501835-22.patch1.53 KBkgoel
#15 2501835-15.patch1.33 KBseantwalsh
#15 interdiff-2501835-15.txt519 bytesseantwalsh
#14 interdiff-2501835-14.txt1.03 KBseantwalsh
#14 2501835-14.patch1.85 KBseantwalsh
#13 Screen Shot 2015-06-14 at 4.47.41 PM.png13.09 KBakalata
#12 interdiff-2501835-11.txt944 bytesseantwalsh
#12 2501835-11.patch1.33 KBseantwalsh
#8 interdiff-2501835-8.txt1.13 KBseantwalsh
#8 2501835-8.patch1.32 KBseantwalsh
#8 PATCH8-core-install-php-1434304801977.png448.38 KBseantwalsh
#8 PATCH6-core-install-php-1434304512923.png542.31 KBseantwalsh
#8 HEAD-core-install-php-1434304402640.png191.81 KBseantwalsh
#6 interdiff.txt1.27 KBkgoel
#6 2501835-6.patch1.17 KBkgoel
#3 remove_safemarkup_set-2501835-3.patch1.3 KBjoelpittet
#3 interdiff.txt1.38 KBjoelpittet
#2 remove_safemarkup_set-2501835-2.patch1.23 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Working on this at NH Dev Days 2.

star-szr’s picture

Status: Active » Needs review
FileSize
1.23 KB

Renaming $message to $failure_message for the initial usage makes this easier for my brain to understand, at least right now.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.38 KB
1.3 KB

This looks good. There is an extra space in the message and removed the unused $message = ''; initialization.

alexpott’s picture

Category: Task » Bug report
Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -148,14 +148,13 @@ public function runTasks() {
-    $message = '';
     foreach ($this->results as $result => $success) {
       if (!$success) {
-        $message = SafeMarkup::isSafe($result) ? $result : SafeMarkup::checkPlain($result);
+        $failure_message = $result;
       }
     }
-    if (!empty($message)) {
-      $message = SafeMarkup::set('Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.' . $message);
+    if (!empty($failure_message)) {
+      $message = SafeMarkup::format('Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.@failure_message', ['@failure_message' => $failure_message]);
       throw new TaskException($message);
     }

This code looks weird as it will only report one error when there are several... The d7 version of this code looks like this:

    foreach ($this->results as $result => $success) {
      if (!$success) {
        $message .= '<p class="error">' . $result  . '</p>';
      }
    }
    if (!empty($message)) {
      $message = 'Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="http://drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.' . $message;
      throw new DatabaseTaskException($message);
    }

I think we should fix this here too ... which means this is a bug. Hmmm that said this was discussed in #2374847: Only last one of the database settings form errors is being printed and decided that this behaviour is correct - I'm not sure I'm convinced by the argument there. What is for sure is that we're missing a space between after the fullstop... hosting provider.@failure_message. I'm not entirely sure that removing the paragraph with the error class was correct either.

kgoel’s picture

working on this

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
1.27 KB

I think we should fix this here too ... which means this is a bug. Hmmm that said this was discussed in #2374847: Only last one of the database settings form errors is being printed and decided that this behavior is correct - I'm not sure I'm convinced by the argument there.

I went ahead and replaced the code how it has been done in D7

- $message = SafeMarkup::isSafe($result) ? $result : SafeMarkup::checkPlain($result);
+ $message .= $result;

I am not sure why $message was rename to $failure_message so reverted that in my patch but if $failure_message makes more sense in readability then sure.
I am not sure how to test this - I tried to drop db and installed site from ui and command line but I don't think thats enough to trigger any db error.

seantwalsh’s picture

Working on testing the latest patch.

seantwalsh’s picture

Below are the steps to reproduce testing, as well as the various screenshots documenting the changes.

  1. Change 'CREATE TABLE {drupal_install_test} (id int NULL)', to a SQL comment '-- CREATE TABLE {drupal_install_test} (id int NULL)',
  2. Run a clean install.
  3. Review that output of the error message, to ensure that the markup is properly escaped.

HEAD:
Only prints the last $message. Uses SafeMarkup::set() & SafeMarkup::isSafe().

PATCH #6:
Prints all $messages but valid markup is escaped. Uses SafeMarkup::format() instead of SafeMarkup::set() and removes SafeMarkup::isSafe().

UPDATED PATCH:
Prints all $messages and properly renders the markup. Removes SafeMarkup::format() from previous patch and replaces it with Xss:filterAdmin().

seantwalsh’s picture

Issue summary: View changes

Updated issue summary to reflect Manual testing steps. Also, removed the "If there is any user or calling code input in the string, submit

alert('XSS');

and ensure that it is sanitized." step since the output is all coming from Tasks.php and not user or calling code input.

seantwalsh’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
seantwalsh’s picture

FileSize
1.33 KB
944 bytes

Silly typos and me uploading the wrong patch!!! Here's the patch that works.

akalata’s picture

Status: Needs review » Needs work
FileSize
13.09 KB

I've applied the patch and was able to confirm that HEAD does not display more than one error message on that step of the install, and that #11 displays all the error messages without double-escaping.

One nit-pick I have is that the multiple error messages run together without even a space after the previous period (see below). Personally I would prefer more of a break than that, but I don't know what the standards are when displaying multiple error messages.

seantwalsh’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
1.03 KB

This version fixes the running together adding each message as a new paragraph for better readability.

seantwalsh’s picture

FileSize
519 bytes
1.33 KB

Sorry had to fix an error I left in. :(

The last submitted patch, 14: 2501835-14.patch, failed testing.

akalata’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I have applied this patch and tested using the manual testing steps described in the issue summary. A review of the patch itself notes only the necessary changes.

xjm’s picture

Status: Reviewed & tested by the community » Postponed

So @alexpott and I discussed this and we agreed that Xss::filterAdmin() is actually not the best approach here. In HEAD, the unsafe message text is checkPlain()ed. This patch loosens that sanitization. It also adds things to the call stack for error handling which is a risk.

This patch is now a case where we're joining an indeterminate number of messages. I was going to suggest an item list, but @alexpott said that would be problematic in this case. Postponing on #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() for now.

alexpott’s picture

I've just tried to prove why I had concerns about the item list idea - I thought this would cause nesting in a very small space but I'm not sure I'm right.

xjm’s picture

Ah so, re: #19, if someone can get it working with an item list render array that'd be great. But if not it can stay postponed.

pwolanin’s picture

@xjm - I don't know that the render system is available if during these early errors? If you want a lit, I think we'd just hard-code the markup.

kgoel’s picture

Issue summary: View changes
Status: Postponed » Needs review
FileSize
1.53 KB
585.32 KB
779.61 KB

YesCT and I worked on it together. In the head in case of database fail during site install, it was displaying only the last error. This patch fixes that problem and it will display all the errors if database fails. There is no interdiff because this is a new approach per @xjm in #20

Database fail error on head

Database fail error with patch

xjm’s picture

Title: Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() » Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() and ensure multiple messages are printed
Issue tags: +Needs tests

Huh, interesting that that does work. And the code is definitely far cleaner and more readable this way. Plus the screenshots look great.

The only concern to discuss is whether it's a good idea to add the dependency to the renderer service here:

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -148,14 +148,25 @@ public function runTasks() {
+      $message = \Drupal::service('renderer')->render(($message), 'error');

...because I have no idea how the installer normally renders things. Per @alexpott, the installer uses the maintenance theme (which is a stripped down version of everything)... This is different from #2505497: Support render arrays for drupal_set_message() because there the render service will eventually be used normally when the page is rendered with your typical drupal_set_message(). But in general the installer builds a lot of things manually.

Meanwhile, since this is a bug (with HEAD discarding most of the messages), we should add test coverage for the multiple messages.

YesCT’s picture

yep, helped code up #22. :)

xjm’s picture

(Updating the issue credit.)

xjm’s picture

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -148,14 +148,25 @@ public function runTasks() {
-        $message = SafeMarkup::isSafe($result) ? $result : SafeMarkup::checkPlain($result);
+        $failure = TRUE;
+        $error[] = $result;
...
+          '#items' => $error,

Oh, can we also get an automated test for sanitization? Because I'm still not sure Twig autoescape is sanitizing the way it does during normal site operation.

lauriii’s picture

Status: Needs review » Needs work
kgoel’s picture

I am working on it.

kgoel’s picture

Twig autoescape sanitizes the stuff but in this case I am not sure if twig sanitize on install. I think we don't need sanitization on install.php page since "we" can assume that the person installing the site has admin permissions. I don't think I need to do any work on #26but reading #23 now.

Does anyone have opinion on my comment above?

kgoel’s picture

I have looked into /core/tests/Drupal/Tests/Core/Database/ConnectionTest.php but I am not entirely sure if that is the correct test file where i should be writing test for displaying multiple database error messages. /core/tests/Drupal/Tests/Core/Database/ConnectionTest.php seems relevant for core/lib/Drupal/Core/Database/Connection.php class.

I have created a follow up issue for adding test coverage since we discovered bug in HEAD while fixing SafeMarkUp issue so seems like a good idea to fix the test in the follow-up issue.

Follow up - #2539902: Add test coverage for Tasks.php

davidhernandez’s picture

It would seem not necessary to test for sanitization during the installer since there is no user entered text. Only test supplied by code or entered by the admin running the installer.

Whether a test to count the correct number of database error messages should be included with this issue is a different question. I'm inclined to say if it was fixed here the test should be added here.

kgoel’s picture

dawehner said \Drupal\system\Tests\Installer\InstallerExistingDatabaseSettingsTest is an example which deals with posting to that form and asked to add a new test class extending \Drupal\simpletest\InstallerTestBase. The new class InstallerNotValidDatabaseSettingstest should live - Drupal\system\Tests\Installer\ and in the setupSettings() method just pass in random db usernames and passwords.

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
778 bytes

Added empty test class InstallerNotValidDatabaseSettingstest.php

Status: Needs review » Needs work

The last submitted patch, 33: 2501835-33.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
1.88 KB

I tried to run test locally but I am not sure if it's caught in loop as it never completes.

Status: Needs review » Needs work

The last submitted patch, 35: 2501835-35.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.35 KB
1.46 KB

Renamed old test class to new test class and fixed the dependency.

Status: Needs review » Needs work

The last submitted patch, 37: 2501835-37.patch, failed testing.

davidhernandez’s picture

With patch applied I get an exception.

Error
The website encountered an unexpected error. Please try again later.
LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead. in Drupal\Core\Render\Renderer->doRender() (line 250 of core/lib/Drupal/Core/Render/Renderer.php).

Drupal\Core\Render\Renderer->doRender(Array, 'error')
Drupal\Core\Render\Renderer->render(Array, 'error')
Drupal\Core\Database\Install\Tasks->runTasks()
db_run_tasks('mysql')
install_database_errors(Array, './sites/default/settings.php')
Drupal\Core\Installer\Form\SiteSettingsForm->validateForm(Array, Object)
call_user_func_array(Array, Array)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_settings_form')
Drupal\Core\Form\FormValidator->validateForm('install_settings_form', Array, Object)
Drupal\Core\Form\FormBuilder->processForm('install_settings_form', Array, Object)
Drupal\Core\Form\FormBuilder->buildForm('Drupal\Core\Installer\Form\SiteSettingsForm', Object)
install_get_form('Drupal\Core\Installer\Form\SiteSettingsForm', Array)
install_run_task(Array, Array)
install_run_tasks(Array)
install_drupal(Object)
davidhernandez’s picture

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -148,14 +148,25 @@ public function runTasks() {
+      $message = \Drupal::service('renderer')->render(($message), 'error');

I don't know why but it definitely seems to be the render line.

kgoel’s picture

FileSize
107.55 KB

I have used renderPlain() but I am still getting the messages with HTML output.

$message = \Drupal::service('renderer')->renderPlain(($message), 'error');
davidhernandez’s picture

If renderRoot or renderPlain are used the problem goes away but the resultant markup is escaped.

So this thing - #2450993: Rendered Cache Metadata created during the main controller request gets lost

I think we need to do one of these maybe?

+        $this->renderer->executeInRenderContext(new RenderContext(), function() use (&$main_content) {
+          return $this->renderer->render($main_content, FALSE);
+        });

Even when I set this up, and get it to work with render(), the markup is still escaped.

kgoel’s picture

Even when I set this up, and get it to work with render(), the markup is still escaped.

I am not sure why to use executeInRenderContext() since renderPlain() or renderRoot() gets rid of the error below.

Error
The website encountered an unexpected error. Please try again later.
LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead. in Drupal\Core\Render\Renderer->doRender() (line 250 of core/lib/Drupal/Core/Render/Renderer.php).

Although, none of the calls renderPlain(), renderRoot() or executeInRenderContext() resolves safe markup issue on the database error page. When I worked on this patch last month #22 then database fail messages were not escaped so I am wondering what else has changed in HEAD beside using \Drupal::service('renderer')->renderRoot() or \Drupal::service('renderer')->renderPlain() instead of \Drupal::service('renderer')->render() .

Anyone has any idea?

davidhernandez’s picture

Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call

That is what the error is. It doesn't have context. renderRoot and renderPlain work because the way is providing context.

kgoel’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
108.37 KB
3.25 KB
2.5 KB

I have used rednerPlain() after getting error which is in #43comment but now I am getting escaped database error markup. I am going to look into escaping issue now and try to figure it out why renderPlain() is causing escaped markup.

Screenshot with escaped markup after using renderPlain()

Status: Needs review » Needs work

The last submitted patch, 45: 2501835-45.patch, failed testing.

kgoel’s picture

Issue summary: View changes
FileSize
1.17 MB

I reproduced the database error on my local. Followed the following steps -

1. Applied #45 patch on local branch
2. sudo rm -r sites/default
3. git checkout -- sites/default
4. SQL comment on line 42 Drupal\Core\Database\Install\Tasks.php
'-- CREATE TABLE {drupal_install_test} (id int NULL)',
5. localhost/install.php, chose language, standard install
6. On database configuration form added correct database username and password
7. Noticed all the database errors but they are all escaped in the screenshot below

-------------------------------------------------------------------------------------------------------------------------------------------------
In my test class, I am passing 'password' => $this->randomMachineName(), and I tried to enter random password on database configuration form but I get different error than with changed behavior in the patch which makes me think that the test is not correct.

Screenshot of database error with random database password

YesCT’s picture

ok. so that means that the test that we have been working on, with the random database password, is not adding coverage for the change in this patch.
we should
a) add that test coverage elsewhere
b) change the test here to test for the change from head: additional error messages
--- but. maybe because of the way we are testing this, by adding a -- comment in front of the database create command, it is not realistic that any actual user would get multiple errors, and we dont need the change in the patch to show multiple errors and dont need the additional test coverage for that here.

I think we need to try and get a way through the UI for a user to make one error message in head, which would have multiple with the patch. and if we cannot do that, we dont need the test coverage, or the change to show multiple.

davidhernandez’s picture

The test isn't working because kgoel didn't finish writing it. The main problem is the functional change is not correct. renderPlain() will escape the markup in the error message, which we don't want.

kgoel’s picture

Its really confusing. I tried using renderRoot but that also escaped the markup so I am not sure what to use. I don't think we want to use executeInRenderContext() but I could be wrong.

I didn't finish working on because database errors are escaped now and also, test case doesn't seem the right place as was suggested in #32

xjm’s picture

  1. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
    @@ -148,14 +148,25 @@ public function runTasks() {
    +    $error = [];
    

    Minor: The name of this variable is misleading as it's actually a plural collection of errors from the results.

  2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
    @@ -148,14 +148,25 @@ public function runTasks() {
    +      $message = \Drupal::service('renderer')->renderPlain(($message), 'error');
    

    Something is not quite right here... >renderPlain() does not do anything with that second argument.

    That wouldn't have anything to do with the failure and double-escaping, but it is worth noting.

joelpittet’s picture

Haven't added anything to the tests but this should clean things up a bit on the fix end. I opted to just use the t() but render out the generated markup with the renderPlain().

@kgoel maybe you can ping me on your intentions on the test and how I can help you get it there.

stefan.r’s picture

#52 addresses the points mentioned in #51, I manually tested it and it seems to display the errors correctly.

It just needs tests now, I'll see if I can update the test scenario.

Status: Needs review » Needs work

The last submitted patch, 52: remove_safemarkup_set-2501835-52.patch, failed testing.

alexpott’s picture

See how to test a broken installer in #2156401: Write install_profile value to configuration and only to settings.php if it is writeable. We need to add an empty setUpSite() method and a test method to assert the output.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.39 KB
2.56 KB

Test added, I would have liked to test for multiple errors but I would need to modify the list of tasks for that, which I'm not sure we want to do.

Right now the task run considers failure of the table creation task fatal (as it should) so it doesn't go on to the next tasks. And there's no easy way to make the insert/update/delete/drop fail consistently on mysql, pgsql and sqlite without first creating a table.

For what it's worth, we may have an actual use case for the multiple error messages in #2529188: Provide better error handling for MySQL client and server utf8mb4 incompatibility, but for now we may have to follow the manual testing steps to confirm this actually displays multiple error messages.

stefan.r’s picture

The actual test was missing there.

A bunch of empty methods might have also worked, instead I added a flag to make setUp() do an early return.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Yup fails with tests only and that seems to be a nice way to cleanup the tests to make them easier to write for this circumstance. Thanks @stefan.r

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/simpletest/src/InstallerTestBase.php
    @@ -72,6 +72,13 @@
       /**
    +   * Whether to bail right after configuring the settings in the installer.
    +   *
    +   * @var bool
    +   */
    +  protected $testSetUpSettingsOnly = FALSE;
    
    @@ -131,6 +138,9 @@ protected function setUp() {
    +    if ($this->testSetUpSettingsOnly === TRUE) {
    +      return;
    +    }
    

    I don't think this is necessary. Just override the methods as necessary. You could be testing errors at any point of the installer. The empty methods in the concrete implementation nicely document the expectation of not reaching these steps.

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerDatabaseErrorMessagesTest.php
    @@ -0,0 +1,39 @@
    +    Database::getConnection('default')->query('CREATE TABLE {drupal_install_test} (id int NULL)');
    

    We should add a comment saying that we are creating a table here to force an error in the installer because it will try and create the drupal_install_test table as this is part of the standard db tests performed by the installer in Drupal\Core\Database\Install\Tasks.

  3. +++ b/core/modules/system/src/Tests/Installer/InstallerDatabaseErrorMessagesTest.php
    @@ -0,0 +1,39 @@
    +    $this->assertRaw('<ul><li>Failed to <strong>CREATE</strong> a test table');
    ...
    +    $this->assertTrue($this->testSetUpSettingsOnly);
    

    Move the assert to the test method as assertTrue(TRUE) is completely not descriptive.

  4. We need to test this test on postgres and sqlite
stefan.r’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
4.69 KB

Status: Needs review » Needs work

The last submitted patch, 60: 2501835-60.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
joelpittet’s picture

Assigned: Unassigned » alexpott
Status: Needs review » Reviewed & tested by the community

@stefan.r I don't think those two fails are this patches fault, I was trying to determine as much and I ran them on an earlier patch in #52.

To me the only one that can let us know is @alexpott on this one, so I'm RTBCing because the comments were addressed regarding the tests.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.92 KB
13 KB
+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -148,14 +147,19 @@ public function runTasks() {
+      $message = t('Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.@errors', ['@errors' => $error_list]);

I don't really think this way of appending the errors into a translatable string is really correct. Reviewing what the installer is doing here I've realised that the whole TaskException is quite bogus we just need to return the errors to the caller. And in the caller we can add the overall message and the list of errors together.

I've added tests to prove that translating these strings works as expected and reworked the changes to InstallerTestBase to do less in setUpSite().

Given the db drivers are extremely rare I think removing TaskException should be ok but I'll ask @chx to give this a look over in case a db driver maintainer has a different perspective.

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -142,26 +146,15 @@
-      $message = t('Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.@errors', ['@errors' => $error_list]);

+++ b/core/includes/install.core.inc
@@ -1133,14 +1132,24 @@ function install_database_errors($database, $settings_file) {
+        '#type' => 'inline_template',
+        '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',
+        '#context' => [
+          'errors' => [
+            '#theme' => 'item_list',
+            '#items' => $errors,
+          ],
+        ],

Why this change? Gabor was in past issues had some concerns with the http://localize.drupal.org/ being able to parse or having to parse another syntax inline.

May need to get Gabor to review.

alexpott’s picture

@joelpittet inline templates are supported by l.d.o this has been discussed before. Dumping a whole item list on the end of a t() is way worse for translators because they have no idea what @errors is in 'Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.@errors'

joelpittet’s picture

@alexpott that is good to know, I thought that was an outstanding issue that never got resolved regarding l.d.o. Fewf.

Looking again at where the errors placed... it's outside the trans block (read it as the same as the t() implementation the first time). So I'm good with that, thanks for the quick reply.

That's quite the scope change on this patch, just because testing installer is weird. It's a bit beyond me at this point but the inline_template solution is ok with me here. Like you mentioned let's get db maintainer to have a peek.

chx’s picture

Why, yes, MongoDB is a DB driver and the only class that actually does something is the install Tasks class. However, it does not throw TaskException so we are good there.

If you'd like a more generic answer, I absolutely have no problems on a task runner needing to catch its exceptions, it's very early, you can't expect anything from the system at this point.

alexpott’s picture

Issue summary: View changes

Filled out a beta evaluation and the issue summary because the scope has expended.

stefan.r’s picture

Component: theme system » database system
Assigned: alexpott » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs subsystem maintainer review

Changes in #65 look good to me, maybe @Crell can have another look at this as it's similar to what we did earlier in #2529188: Provide better error handling for MySQL client and server utf8mb4 incompatibility, as well as chime in on the API changes

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\SafeMarkup;

Nit: unused use statement

alexpott’s picture

Remove the unused use.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/includes/install.core.inc
    @@ -1133,14 +1132,24 @@ function install_database_errors($database, $settings_file) {
    +    if (count($errors)) {
    +      $error_message = [
    +        '#type' => 'inline_template',
    +        '#template' => '{% trans %}Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.{% endtrans%}{{ errors }}',
    +        '#context' => [
    +          'errors' => [
    +            '#theme' => 'item_list',
    +            '#items' => $errors,
    +          ],
    +        ],
    +      ];
    

    Not 100% comfortable with Twig and #theme here compared to not having it, but also I don't think it's adding a dependency we don't already have.

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerTranslationTest.php
    @@ -46,6 +47,26 @@ protected function setUpLanguage() {
    +    $this->assertRaw('Beheben Sie alle Probleme unten, um die Installation fortzusetzen. Informationen zur Konfiguration der Datenbankserver finden Sie in der <a href="https://www.drupal.org/getting-started/install">Installationshandbuch</a>, oder kontaktieren Sie Ihren Hosting-Anbieter.');
    

    Won't this test fail if the translation is changed on l.d.o?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@catch:

  1. Yep we're using twig and theming in the installer already
  2. Nope the translation is added by the test. - it's not coupled to l.d.o at all.

Tentatively setting back to rtbc.

catch’s picture

Status: Reviewed & tested by the community » Fixed

#1 and #2 both make sense.

Committed/pushed to 8.0.x, thanks!

  • catch committed 7fa232d on 8.0.x
    Issue #2501835 by kgoel, crowdcg, stefan.r, alexpott, joelpittet,...

Status: Fixed » Closed (fixed)

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