Explicitly support render arrays in hook_requirements()

The 'description' key that I think is mostly in question of wanting a render array is the one that seems to get passed to email notification and drupal_set_message() so I'm going to postpone this on #2505497: Support render arrays for drupal_set_message()

I actually don't think this should be postponed on #2505497: Support render arrays for drupal_set_message() because hook_requirements() is actually much more straightforward than drupal_set_message(), and already works with render arrays. We just need to document it. I think we should go ahead and do so because following #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), #2506581: Remove SafeMarkup::set() from Renderer::doRender, and #2506195: Remove SafeMarkup::set() from Xss::filter(), it should be a best practice to use render arrays for building markup together with translated strings, especially for admin use, because we've done a lot of work so that #markup minimizes overhead and side effects while ensuring safe output. The only case where we should avoid it is when we explicitly want something to be escaped rather than filtered.

Patch.

Should add explicit test coverage too.

Comme ça.

1. +++ b/core/modules/system/src/Tests/Module/HookRequirementsOutputTest.php
   @@ -0,0 +1,50 @@
   +   * @todo We should possibly test both the install and runtime phases.
   

   Note.

2. +++ b/core/modules/system/tests/modules/requirements3_test/requirements3_test.install
   @@ -0,0 +1,35 @@
   +  else{
   

   Missing space there.

+++ b/core/modules/system/tests/modules/requirements3_test/requirements3_test.install
@@ -0,0 +1,35 @@
+  // Always fails requirements.

Also, that's obviously a bad copypaste.

However, actually, it looks like the render arrays also don't work for the install phase:

This is related to #2505497: Support render arrays for drupal_set_message(), but not actually 100% the same issue. In drupal_check_module():

          $message = t('@requirements_message (Currently using @item version @version)', array('@requirements_message' => $requirement ['description'], '@item' => $requirement ['title'], '@version' => $requirement ['value']));

See also #2501639: Remove SafeMarkup::set in drupal_check_module(), where we cleverly did this because it was simple and gave translators more context. :P

Updating the summary to reflect #10.

We could document that render arrays are only supported for the install phase, but that wouldn't be great DX.

The attached is a workaround (not that I'm thrilled with it). It also adds test coverage for that case, although it'd probably be better not to double up the test coverage in that way. Also, screenshot of an item list in action:

Mmm, so yeah, I guess @joelpittet was right in the first place and this should be postponed on #2505497: Support render arrays for drupal_set_message(). :)

That's unpossible! @see https://twitter.com/joelpittet/status/627559394209742848

Thanks for giving a go though, it's nice to have a second pair of eyes on the problem!

Alright, I'm increasingly unsure whether #2505497: Support render arrays for drupal_set_message() is going to be fixed or not, and it's gotten sidetracked into some stuff about error handling. So for now I'm going to roll the patch here on top of that one, and if #2505497: Support render arrays for drupal_set_message() doesn't land, then we can reuse the same tests but just add a hunk here.

So after I broke it a different way I got frustrated with the paucity of test coverage for what is actually a whole lot of complexity in how the hook gets used. I also found an assertion in the existing test that I think may have been a false positive.

So the attached tests every darn permutation, to expose which ones currently fail in HEAD. Spoiler alert: it's not just render arrays.

fixing that fatal error, $description_array was declared twice

Not the right failures. :P Protip: don't add patch documentation while on sleep meds.

Seems we had the same thought there :)

Hm, those still aren't the right failures.

+++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
@@ -8,24 +8,224 @@
+    $edit['modules[testing][requirements1_test][enable]'] = 'requirements1_test';

s/testing/Testing?

Nice catch @stefan.r! That explains why it was passing locally but not on the bots.

Those are the right fails.

Two framework managers have essentially said #2505497: Support render arrays for drupal_set_message() is wontfix. While I disagree strongly with encouraging developers to use renderPlain() in their own code, at least the tangled internals of hook_requirements() are not that. So we need to fix drupal_check_module() instead to make progress.

(BTW, issue still assigned to me, and working on the patch.)

Attached fixes the tests, fixes drupal_check_module(), and adds a @todo for a to-be-filed followup for the weirdness around the 'title' and 'value' keys in the installer which I have yet to file. (TLDR, 'value' means different things all over core.)

Remaining tasks:

• File followup for the expected behavior of 'value' and 'title'.
• Add markup to the render arrays to ensure they're not escaped unexpectedly.
• Add a test for XSS.
• Test profiles and themes too.

Pinged @catch to look at this issue and weigh in on whether he is okay with it before proceeding further since he had concerns about #2505497: Support render arrays for drupal_set_message(). I'm hoping that it'll be determined that since render arrays already work in (I think) 22 of 24 possible cases for hook_requirements(), it's worth improving drupal_check_module() so that the remaining two can be supported as well. There are three other children of the critical that will be easily solved with this.

I just tested and hook_requirements() is apparently supported for install profiles at runtime, but not during installation. I added this:

diff --git a/core/profiles/standard/standard.install b/core/profiles/standard/standard.install
index 4d4416a..3b27fa7 100644
--- a/core/profiles/standard/standard.install
+++ b/core/profiles/standard/standard.install
@@ -63,3 +63,12 @@ function standard_install() {
   // Enable the admin theme.
   \Drupal::configFactory()->getEditable('node.settings')->set('use_admin_theme', TRUE)->save(TRUE);
 }
+
+function standard_requirements() {
+  $requirements['standard_fail'] = [
+    'title' => 'Standard! Fail!',
+    'severity' => REQUIREMENT_ERROR,
+    'description' => 'YOU CANNOT HAS',
+  ];
+  return $requirements;
+}

It had no effect on the installation of Standard, but did register a message at runtime:

So crossing out that item in the summary.

Okay... something funky is actually going on with hook_requirements() during the installer. Implementing this for taxonomy prevents the installation of Standard:

function taxonomy_requirements() {

return [
  'taxonomy_cow' =>
  [
    'severity' => REQUIREMENT_ERROR,
    'title' => 'TAXONOMY FAIL!',
    'description' => 'Dude. Bro.',
  ],
];
}

However, adding this to text.install instead has no effect and installation of the profile proceeds:

function text_requirements() {
  $requirements['text_cow'] =
    [
      'severity' => REQUIREMENT_ERROR,
      'title' => 'TEXT FAIL!',
      'description' => 'Dude. Bro.',
    ];

  return $requirements;
}

Looks like a nasty bug if I haven't screwed something up.

Filed #2549043: hook_requirements() is not reliable during Drupal installation for that.

@catch said in IRC that he'd be inclined to not support render arrays here if it didn't already work, but since it does already work in most cases, it'd be an API change at this point to remove support.

I also think the change to drupal_check_module() is appropriate regardless, actually. The only thing that would be different is the is_array() hunk.

And hey the test coverage is a win in any case. If we decide to remove render array support at a later time, it's just a diff of a few lines to remove that from the tests.

So that I don't forget about the followup for the value key weirdness.

@xjm other than filing the followup the remaining tasks from #30 are done and this is ready for final review?

1. +++ b/core/includes/install.inc
   @@ -979,10 +979,24 @@ function drupal_check_module($module) {
   +          $message['description'] = [
   ...
   -          $message = t('@requirements_message (Currently using @item version @version)', array('@requirements_message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']));
   +          $message['version'] = [
   

   So $message['description'] + $message['version'] supersede $message the string.

   Ok. But there is no guarantee that they will be rendered in this order. IMO it's better to make that ordering explicit and specify a #weight.

   Finally, it seems… odd… to use the render system to generate a plain string. I don't see any HTML, so I'm confused why this is necessary.

   After having read the test code, I understand why. But this really needs some docs. Probably on the if is_array($requirement['description']) check: it could say it does a straight assignment in case the description already is a render array.

2. +++ b/core/includes/install.inc
   @@ -979,10 +979,24 @@ function drupal_check_module($module) {
   +            '#suffix' => ' ',
   ...
   +            '#prefix' => '(',
   

   The suffix of a single space should be moved to this prefix AFAICT.

3. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -104,14 +104,18 @@ function testEnableRequirementsFailureDependency() {
   +      // Attempt to install both modules at the same time.
   

   Indentation error.

4. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +  function testHookRequirements() {
   +
   +    // Test installing the module with all possible combinations of values
   +    // in the hook_requirements() hook.
   

   Extraneous \n.

   80 cols violation.

5. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +    // Test the status report with all possible combinations of values
   +    // in the hook_requirements() hook.
   

   80 cols.

6. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +    // @todo Test update phase separately.
   

   Ah, so this answers #42: this seems to be the last remaining bit to be done.

7. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +  protected function doInstallTest() {
   +
   +    // Only install phase requirement errors should fail installation.
   

   Again extraneous \n.

8. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +    }
   +
   +  }
   

   Another extraneous \n.

9. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
   @@ -8,24 +8,232 @@
   +  protected function doRuntimeTest() {
   +
   +    // Configure the module's hook_requirements().
   

   Again.

10. +++ b/core/modules/system/tests/modules/requirements1_test/requirements1_test.install
    @@ -2,18 +2,53 @@
    + * - requirements1_test.phase: Whether to add the message for 'install', 'update', or 'runtime' phase.
    

    80 cols.

11. +++ b/core/modules/system/tests/modules/requirements1_test/requirements1_test.install
    @@ -2,18 +2,53 @@
       return $requirements;
    +
     }
    

    And again extraneous \n.

Our coding standards don't dictate one way or the other whether there can be a blank line at the beginning or end of a method, which means I would not give that feedback, but I don't care one way or the other so I'll change it.

Thanks @Wim Leers. Re: #43:

1. Sure. Added a couple inline comments and also added a #weight.
2. Sure, fixed.
3. Fixed.
4, 5, 7, 8, 9, 11. Meh, but fixed.
6. Actually I decided to remove the todo instead since adding full test coverage for that wouldn't work in that test anyway and it's probably not in scope. However, we should at least test it manually, so tagging for that.
10. Fixed.

I filed #2505499: Explicitly support render arrays in hook_requirements() for the followup; still drafting that summary. Attached also takes care of the outstanding work to add test coverage for markup in the messages to ensure it's not double-escaped.

I considered also adding a test for XSS, but decided against it because any scenario for that would be really contrived: Not only would the module need to put user input in the requirements message (Choose Your Own Requirements!) but the developer would furthermore have to not use t() properly for the message. So I didn't add test coverage for that.

Oh and @stefan.r the remaining tasks were in the summary and up to date before the last patch. :) Updated them now.

#45:

• the issue you link to, as being the one that you filed for the follow-up… is this very issue :) Can you fix that?
• what sort/level of manual testing do you think is necessary?

+++ b/core/includes/install.inc
@@ -979,10 +979,30 @@ function drupal_check_module($module) {
+        // If the requirement description is already a render array, add it to
+        // the message array.
+        if (is_array($requirement['description'])) {
+          $message['description'] = $requirement['description'];
+        }
+        // Otherwise, add a #markup element for the description string.
+        else {
+          $message['description'] = [
+            '#markup' => $requirement['description'],
+            '#weight' => 0,
+          ];
+        }

This means it's still possible for $requirement['description'] to have a non-zero #weight. We should explicitly set it to zero outside the if/else.

Working on a new patch that will address the weight issue.

I have moved the else statement out of the if-else block so that the weight is always set, along with the '#markup' variable. In the next section, if it is determined that the $requirement['description'] is actually a render array, the value of $message['description'] that is set in the previous code-block is wiped away and replaced with the render array.

To test it - it's a bit of a hack, but I altered the aggregator module so that it reported that curl was not installed even though it was. This is in 2505499-52-testing-do-not-test.patch.

1. Ensure you have curl installed
2. Apply 2505499-52-testing-do-not-test.patch
3. Enable the aggregator module
4. You should see the message "The Aggregator module could not be installed because the PHP cURL library is not available. (Currently using cURL version Enabled)" even though cURL is actually installed.

+++ b/core/includes/install.inc
@@ -980,18 +980,18 @@ function drupal_check_module($module) {
+        $message['description'] = [
+          '#markup' => $requirement['description'],
+          '#weight' => 0,
+        ];
+
+        // If the requirement description is actually a render array, correct
+        // the message array to recognize this.
         if (is_array($requirement['description'])) {
           $message['description'] = $requirement['description'];
         }

This still means the #weight in $requirement['description'] — if it's a render array — will still override that zero.

+++ b/core/includes/install.inc
@@ -979,10 +979,30 @@ function drupal_check_module($module) {
+        \Drupal::service('renderer')->renderPlain($message);

$message = \Drupal::service('renderer')->renderPlain($message); no?

#56: yes! Perfect.

So this now only needs manual testing, which YesCT seems to have begun in #53.

Why is it doing it like so:

+ // Set the description weight to 0. If the description was previously
+ // a render array, we overwrite the existing weight here.
+ $message['description']['#weight'] = 0;

I don't understand why we would want to change the weight if it is already set.

@partyka: Because we need to make sure the description always comes first. So it must have a lower weight.

@Wim Leers: I understand, that makes sense.

I am going to work on a test to insure that the description comes before the "Currently using..." string

Opened issue for drupal set message spacing in seven theme described in comment #62: #2550925: Header style in seven theme with drupal set message

I haven't gotten around yet to creating a diff or even know if this is somewhat correct code due to some difficulties with my testing environment, but while talking with @xjm we came up the following method in Drupal\system\Tests\Module\HookRequirementsTest

  function testVersionRequirements() {
    // Ensure that the module is not currently installed.
    print "HI";
    $this->assertModules(['requirements1_test'], FALSE);

    // Configure the module's hook_requirements().
    \Drupal::state()->set('requirements1_test.phase', "install");
    \Drupal::state()->set('requirements1_test.severity', REQUIREMENT_ERROR);
    \Drupal::state()->set('requirements1_test.description_array', TRUE);
    \Drupal::state()->set('requirements1_test.use_value', TRUE);

    // Attempt to install the module.
    $edit = [];
    $edit['modules[Testing][requirements1_test][enable]'] = 'requirements1_test';
    $this->drupalPostForm('admin/modules', $edit, t('Install'));

    $this->verbose("<pre>" . print_r($this, TRUE) . "</pre>");
    parent::assertText('Currently using');

  }

When attempting to run it, I get Killed
FATAL Drupal\system\Tests\Module\HookRequirementsTest: test runner returned a non-zero error code (137).

Obviously, the $this->verbose("<pre>" . print_r($this, TRUE) . "</pre>"); is just me trying to work past the debugging/testing issues.

I took out the verbose statement and the issues seem to have gone away. Which, in retrospect, makes sense as the object is probably really large when dumped out. Patch coming later.

Adding new tests to test for the proper placement of the value text after the description. What's odd is that is that passes ok when I use the browser-based test runner, but I am having issues on the CLI. I think that may be due to some sort of CLI configuration.

Here is the patch and interdiff.

----------------------
CLI issues:
php ./core/scripts/run-tests.sh --color --url http://drupalvm.dev/ --verbose --class 'Drupal\system\Tests\Module\HookRequirementsTest'

when running as 'vagrant' user:

Test summary
------------

Drupal\system\Tests\Module\HookRequirementsTest                0 passes             6 exceptions             

Test run duration: 1 sec

When running as sudo (as it's a vagrant box)

Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\Module\HookRequirementsTest

Test run started:
  Friday, August 14, 2015 - 18:03

Test summary
------------

Drupal\system\Tests\Module\HookRequirementsTest              738 passes 277 fails                            

Test run duration: 2 min 36 sec

Using the browser-based testrunner, running only HookRequirementsTest:

960 passes, 0 fails, 0 exceptions, 154 debug messages

So, I'm not sure why the numbers don't even match up.

I think the page at https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.... also needs to be updated to state that the description field supports render arrays

...and the value field as well

Are there any specific tests that we can do for the value field as well?

FYI: Drush has an existing PR - https://github.com/drush-ops/drush/pull/1315

The only reason description supports render arrays is because they get printed in status-report.html.twig and any variable printed in a twig template can be a render array, object with __toString() and just a string.

Drush doesn't currently support this.

I don't think we should be adding that to the docs because every template supports this...

@YesCT: Regarding #80, yes, it should not be installed. That's just a bit of a leftover.. I embarrassed I left it in there, I feel like I made extra work for you. sorry about that.

@YesCT: Re #82:
Maybe it might be better to say that the "version requirements" come after the description. The requirements do contain the 'value' element. See core/includes/install.inc, function drupal_check_module near line 1000:

        if (isset($requirement['value']) && $requirement['value']) {
          $message['version'] = [
            '#prefix' => ' (',
            '#markup' => t('Currently using @item version @version', array('@item' => $requirement['title'], '@version' => $requirement['value'])),
            '#suffix' => ')',
            '#weight' => 1,
          ];
        }

What do you think?

Reviewing

Agree this is ready. Manual testing has been added in #62, #74 & #76.
Automated test for ensuring description has a lower weight and and comes first was completed in #86. There is no longer any extraneous test code there and don't see any additional style issues.

+++ b/core/includes/install.inc
@@ -979,11 +979,32 @@ function drupal_check_module($module) {
     foreach ($requirements as $requirement) {
       if (isset($requirement['severity']) && $requirement['severity'] == REQUIREMENT_ERROR) {
-        $message = $requirement['description'];
+        $message = [];
+
+        // If the requirement description is already a render array, add it to
+        // the message array.
+        if (is_array($requirement['description'])) {
+          $message['description'] = $requirement['description'];
+        }
+        // Otherwise, add a #markup element for the description string.
+        else {
+          $message['description'] = ['#markup' => $requirement['description']];
+        }
+        // Make sure the description is first, even if the description render
+        // array had an existing weight.
+        $message['description']['#weight'] = 0;
+
+        // If a required value was provided for the item, append it to the error
+        // message.
         if (isset($requirement['value']) && $requirement['value']) {
-          $message = t('@requirements_message (Currently using @item version @version)', array('@requirements_message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']));
+          $message['version'] = [
+            '#prefix' => ' (',
+            '#markup' => t('Currently using @item version @version', array('@item' => $requirement['title'], '@version' => $requirement['value'])),
+            '#suffix' => ')',
+            '#weight' => 1,
+          ];
         }
-        drupal_set_message($message, 'error');
+        drupal_set_message(\Drupal::service('renderer')->renderPlain($message), 'error');
       }
     }

We have no automated test coverage of this change. It is possible to test the broken installer using InstallerTestBase. It's not that easy - see #2156401: Write install_profile value to configuration and only to settings.php if it is writeable. Given that this is the main change of the patch I think we should add test coverage here. I debated with myself about asking for a followup but considering that this is the main code change it should be tested.

@alexpott: I'm sorry but I don't see a connection. How is the support for render arrays related to the issue in #2156401?

@partyka I think that issue is just an example of how to make the installer break in a test

Are we talking about the Drupal installer or the requirements hook? If we're just testing the requirements hook, why do we need to break the installer? I would expect (but have not confirmed) that the installer test would check to see if all the necessary modules were enabled or not, and that would be sufficient. I don't think the installer test should care why or how a module install failed.

Ah you're right, that code is only used in /admin/modules, looks like we got a bit confused by the filename (install.core.inc is about the installer, install.inc is about installing extensions).

But I guess it could still use test coverage regardless?

+++ b/core/includes/install.inc
@@ -979,11 +979,32 @@ function drupal_check_module($module) {
+        // If a required value was provided for the item, append it to the error
+        // message.
         if (isset($requirement['value']) && $requirement['value']) {
-          $message = t('@requirements_message (Currently using @item version @version)', array('@requirements_message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']));
+          $message['version'] = [
+            '#prefix' => ' (',
+            '#markup' => t('Currently using @item version @version', array('@item' => $requirement['title'], '@version' => $requirement['value'])),
+            '#suffix' => ')',
+            '#weight' => 1,
+          ];
         }

Actually this error message is completely nonsensical, considering how people use hook_requirements(): http://www.drupalcontrib.org/api/drupal/drupal!modules!system!system.api...

This should probably be removed in a followup?

So yep my bad - drupal_check_module() is not used from the installer but both the installer and update.php display requirements - we need to check that render arrays work in both places. Also re #94 the $message['version'] is being added to $message['description'] so it does make sense.

My point was the message that's being added to the description doesn't make sense, when it says DESCRIPTION. Currently using TITLE version VALUE, just looking through some random contrib modules the last bit will output things like:

• Currently using Flag version Translation version helpers module not found.
• Currently using Extensions not available version Either the memcache or memcached extensions must be installed in order to use memcache integration.
• Currently using version Client error
• Currently using XML sitemap user version Anonymous access to user profiles

This doesn't make sense considering how it's implemented in some of contrib and core; VALUE is almost never a version and TITLE is often not something we're "currently using". So yes this should be in the error string, but this could rather say something like "TITLE: VALUE<br />DESCRIPTION" in the dsm.

edit: I'll cross-post this to #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set as I don't want to derail this issue... So for now this just needs a test making sure markup arrays work in the installer and the updater.

Per IRC with stefan.r:

Issue #2501835 may also have example of broken installer.

@stefan.r yep, that's what that stub issue was for, thanks!

#2549043: hook_requirements() is not reliable during Drupal installation was why I did not add tests for the installer. @alexpott suggested a workaround of adding the test module to the testing profile explicitly in its dependencies in info.yml.

So, I'm confused here - is an installer test needed, or not?

Since we are in RC, I'm postponing to 9 as it's unlikely this will go into 8.1.x. If someone finds this necessary for an 8.1.x feature, feel free to move it to 8.1.x and we can unpostpone after 8.0.0 is released.

Instead of postponing this to 9.0.x, postponing for 8.1.x. Also demoting this to normal based on #101

@partyka and I discussed this issue today.

@alexpott and I agreed that this would be an acceptable API "addition" (sort of) for a minor release, so I am unpostponing it. We agree it is just normal priority now. There is some disruption for Drush (and other CLI tools) that currently expect a string, but for Drush there is an existing issue: https://github.com/drush-ops/drush/issues/1314

@partyka and I determined that the patch on the issue needs to be rerolled. Looking through the patch, I also don't see any update for the hook_requirements() documentation for this change (edit: see the original patch on the issue), so we should add that too, as well as a draft change record. So marking Needs work for those things. (I did not review the patch in full at this point.)

This issue reminded me of #2662552: Early use of render service in installer is problematic, but that problem was for the installation of core, not for the installation of modules. So we are OK there.

@xjm I have rerolled the patch and ran the testing script via the GUI testing. I'm getting an error when I run the tests, so i'm just posting the patch as a re-roll point. Not expected to pass/work.

So, the error I was getting when I was running the patch was simply due to a misconfiguration in my local stack. I was getting a timeout error in fcgi. Changed it to mod_php and it worked. Is this an issue that the test takes long? It's probably just because it's running locally that it took really long.

Uploading new patch for testing. Patch is done against re-roll as well as incorporating verbage suggested in #96

Patch in #107 done against wrong branch.. please ignore

@alexpott and I have decided to see if creating a test using InstallerTestBase will resolve the issue mentioned in comment #89. I was hoping that by using an install profile we could work around some of the issues. I created a new class to do so - InstallerRequirementsTest - and based it on DistributionProfileTest. DistributionProfileTest attempts to install a new testing module that is designed to explicitly fail to print the result to the screen. This testing module is requirements_installer_test.

I've placed a breakpoint inside of requirements_installer_test_requirements and it's never reached when i am running a test. Not sure why it appears the profile created by InstallerRequirementsTest doesn't make the test module of requirements_installer_test install.

@xjm, @alexpott: Where should I update the documentation for hook_requirements()? Sounds to me like you're referring to the documentation that is automatically generated from the function comments. Is that right?

I'm not sure if it should be part of this issue, or a separate one:
When installing Drupal I see a warning `Array to string conversion install.core.inc:2286`. That happens in install_display_requirements() because hook_requirements() in modules/system/system.install returns an Array for description instead of string.

I believe install_display_requirements() should also be changed to support the render arrays.