Follow-up to #2296929: Remove system_requirements() SafeMarkup::set() use
Problem/Motivation
hook_requirements()
messages currently work with render arrays for runtime requirements messages. However, the documentation does not explicitly state that render arrays are allowed. Additionally, install phase requirements do not support render arrays because in drupal_check_module()
they are passed to drupal_set_message()
, and also to a placeholder in a t()
following #2501639: Remove SafeMarkup::set in drupal_check_module().
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.
Proposed resolution
Explicitly support render arrays in hook_requirements().
Remaining tasks
File followup for the expected behavior of 'value' and 'title'.Done: #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that APIAdd markup to the render arrays to ensure they're not escaped unexpectedly.DoneAdd a test for XSS.Not done; way too contrived in ahook_requirements()
message.Test profiles and themes too.Not supported for the install phase and/or broken. See #2549043: hook_requirements() is not reliable during Drupal installation.- (done in #62) Manually test a requirements error on module install.
- (done in #75) Manually test a requirements error on module update.
- (done in #86)
Add a test to ensure that description has a lower weight and comes first - needs a test making sure markup arrays work in the installer and the updater See #89 - #96
- Followup to ensure Drush compatibility: https://github.com/drush-ops/drush/issues/1314
User interface changes
before for install
cannot use a render array for the description
after for install
can use a render array for the description
before for update
after for update
with the patch they are exactly the same (and the markup is the same)
Steps to manually test
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.
- Ensure you have curl installed
- Apply 2505499-52-testing-do-not-test.patch (or the one in comment #62
- Enable the aggregator module
- 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.
steps to manually test update descriptions
- install drupal, log in as admin
- enable the aggregator module (with no changes)
- apply patch like in #75 (can toggle commenting and uncommenting either the string or the render array)
- go to /drupal/update.php
Comment | File | Size | Author |
---|---|---|---|
#110 | explicitly_support-2505499-110.patch | 19.72 KB | partyka |
#108 | explicitly_support-2505499-108.patch | 16.77 KB | partyka |
#107 | explicitly_support-2505499-107.patch | 1.69 KB | partyka |
#105 | explicitly_support-2505499-105.patch | 16.79 KB | partyka |
#86 | interdiff.2505499.81.85.txt | 5.81 KB | YesCT |
Comments
Comment #1
xjmComment #2
joelpittetThe '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()
Comment #3
YesCT CreditAttribution: YesCT commentedadding an @todo to #2501683: Remove SafeMarkup::set in _update_message_text() so making it related here.
Comment #4
xjmI 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 thandrupal_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.Comment #5
xjmPatch.
Comment #6
xjmShould add explicit test coverage too.
Comment #7
xjmComme ça.
Comment #8
xjmNote.
Missing space there.
Comment #9
xjmComment #10
xjmAlso, 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():
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
Comment #11
xjmUpdating 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:
Comment #12
xjmMmm, 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(). :)
Comment #13
joelpittetThat'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!
Comment #14
xjmAlright, 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.
Comment #15
xjmSo 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.
Comment #17
stefan.r CreditAttribution: stefan.r commentedfixing that fatal error, $description_array was declared twice
Comment #18
xjmNot the right failures. :P Protip: don't add patch documentation while on sleep meds.
Comment #19
stefan.r CreditAttribution: stefan.r commentedSeems we had the same thought there :)
Comment #23
xjmHm, those still aren't the right failures.
Comment #24
stefan.r CreditAttribution: stefan.r commenteds/testing/Testing?
Comment #25
stefan.r CreditAttribution: stefan.r commentedComment #27
xjmNice catch @stefan.r! That explains why it was passing locally but not on the bots.
Those are the right fails.
Comment #28
xjmTwo 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 ofhook_requirements()
are not that. So we need to fixdrupal_check_module()
instead to make progress.Comment #29
xjm(BTW, issue still assigned to me, and working on the patch.)
Comment #30
xjmAttached 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:
Comment #31
stefan.r CreditAttribution: stefan.r commentedComment #32
xjmComment #34
xjmComment #35
xjmPinged @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 improvingdrupal_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.Comment #36
xjmI just tested and
hook_requirements()
is apparently supported for install profiles at runtime, but not during installation. I added this:It had no effect on the installation of Standard, but did register a message at runtime:
So crossing out that item in the summary.
Comment #37
xjmOkay... something funky is actually going on with
hook_requirements()
during the installer. Implementing this for taxonomy prevents the installation of Standard:However, adding this to text.install instead has no effect and installation of the profile proceeds:
Looks like a nasty bug if I haven't screwed something up.
Comment #38
xjmFiled #2549043: hook_requirements() is not reliable during Drupal installation for that.
Comment #39
xjmComment #40
xjm@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 theis_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.
Comment #41
xjmSo that I don't forget about the followup for the value key weirdness.
Comment #42
stefan.r CreditAttribution: stefan.r commented@xjm other than filing the followup the remaining tasks from #30 are done and this is ready for final review?
Comment #43
Wim LeersSo$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.The suffix of a single space should be moved to this prefix AFAICT.
Indentation error.
Extraneous
\n
.80 cols violation.
80 cols.
Ah, so this answers #42: this seems to be the last remaining bit to be done.
Again extraneous
\n
.Another extraneous
\n
.Again.
80 cols.
And again extraneous
\n
.Comment #44
xjmOur 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.
Comment #45
xjmThanks @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.
Comment #46
xjmOh and @stefan.r the remaining tasks were in the summary and up to date before the last patch. :) Updated them now.
Comment #47
xjmComment #48
Wim Leers#45:
Besides the manual testing and preventing problems in the edge case outlined below, I think this patch is ready :)
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.Comment #49
partyka CreditAttribution: partyka commentedWorking on a new patch that will address the weight issue.
Comment #50
YesCT CreditAttribution: YesCT commentedI'm going to do the manual testing.. by
hacking a test module so its version looks like it needs an update.
making it so it requires something that isn't there
and testing if a render array works in the message
Comment #51
YesCT CreditAttribution: YesCT commentedjust noting that in the interdiff in #45 the followup is correct in the comment in the code and is: #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set
it was just mis-copy-pasted in the issue comment
Comment #52
partyka CreditAttribution: partyka commentedI 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.
Comment #53
YesCT CreditAttribution: YesCT commentedI'm going to review this. and check on the manual testing.
Comment #54
Wim LeersThis still means the
#weight
in$requirement['description']
— if it's a render array — will still override that zero.Comment #55
alexpott$message = \Drupal::service('renderer')->renderPlain($message);
no?Comment #56
stefan.r CreditAttribution: stefan.r commentedComment #57
Wim Leers#56: yes! Perfect.
So this now only needs manual testing, which YesCT seems to have begun in #53.
Comment #58
partyka CreditAttribution: partyka commentedWhy 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.
Comment #59
Wim Leers@partyka: Because we need to make sure the description always comes first. So it must have a lower weight.
Comment #60
YesCT CreditAttribution: YesCT commentedmaybe we should have a test then, that we can show fails for #52, and asserts the description is first.
Comment #61
YesCT CreditAttribution: YesCT commentedcan we check here that it is first?
and here set a weight that would cause it to be in the wrong order (for the earlier patch).
Comment #62
YesCT CreditAttribution: YesCT commentedmanual testing. using both a string and a render array. the do-not-test patch shows the render array.
setting the weight and testing patch in #52 confirms that the order of the message and the "(Currently using cURL version Enabled)" is different than in head. and with #57 the order is the same as in head.
before:
after:
with render array (alignment issue? with the 'x' icon?)
with just description string like in head.
---------
updated issue summary
Comment #63
partyka CreditAttribution: partyka commented@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
Comment #64
justAChris CreditAttribution: justAChris as a volunteer commentedOpened issue for drupal set message spacing in seven theme described in comment #62: #2550925: Header style in seven theme with drupal set message
Comment #65
partyka CreditAttribution: partyka commentedI 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
When attempting to run it, I get Killed
FATAL Drupal\system\Tests\Module\HookRequirementsTest: test runner returned a non-zero error code (137).
Comment #66
partyka CreditAttribution: partyka commentedObviously, the
$this->verbose("<pre>" . print_r($this, TRUE) . "</pre>");
is just me trying to work past the debugging/testing issues.Comment #67
partyka CreditAttribution: partyka commentedI 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.
Comment #68
YesCT CreditAttribution: YesCT commentedgoing to start working on manually testing the update phase. (mentioned in #45 6.)
Comment #69
partyka CreditAttribution: partyka commentedAdding 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:
When running as sudo (as it's a vagrant box)
Using the browser-based testrunner, running only HookRequirementsTest:
So, I'm not sure why the numbers don't even match up.
Comment #70
partyka CreditAttribution: partyka commentedI 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
Comment #71
stefan.r CreditAttribution: stefan.r commented...and the value field as well
Comment #72
YesCT CreditAttribution: YesCT commentedattaching changes, showing my attempt (which does not make aggregator show up on /admin/modules/update)
why? hm.
Comment #73
partyka CreditAttribution: partyka commentedAre there any specific tests that we can do for the value field as well?
Comment #74
YesCT CreditAttribution: YesCT commentedso. have to go to /drupal/update.php
putting some steps to test in the issue summary.
next testing with the patch and render arrays.
Comment #75
alexpottFYI: Drush has an existing PR - https://github.com/drush-ops/drush/pull/1315
Comment #76
YesCT CreditAttribution: YesCT commentedrender array works in head for update requirement description.
markup:
and that is exactly the same in head and with the patch
removing needs manual testing tag since that is done.
=====
next I'll review the whole patch
Comment #77
YesCT CreditAttribution: YesCT commentedhttps://www.drupal.org/node/1354#drupal
"All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc."
we can @see the function and api.drupal.org will make a link to it, we dont have to link to api.drupal.org in the code.
I'll simplify this to be "Make sure..."
for this one, the test is making sure that the description is not printed, but this test is about the *order* ... so I'm not sure we need this here if there is no order to check.
I'm going to make some changes.
Comment #78
YesCT CreditAttribution: YesCT commentedregarding #70 and #71
the api page gets its information from the docs in the code, so to make a change, we would do it in
function hook_requirements()
in
module.api.php
Comment #79
joelpittetThe 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 arender array
,object with __toString()
and just astring
.Drush doesn't currently support this.
I don't think we should be adding that to the docs because every template supports this...
Comment #80
YesCT CreditAttribution: YesCT commentedwhy uninstall? it should have not installed, right?
Comment #81
YesCT CreditAttribution: YesCT commentedand... wrapped comments to as close to 80 chars as possible per
"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below)."
https://www.drupal.org/node/1354#drupal
Comment #82
YesCT CreditAttribution: YesCT commentedI'm done for the day if anyone else wants to pick up the review.
noting this for the next person (or for me tomorrow). That I think this is not actually what is happening, it is appending the "(Currently using ..." but I'm not sure that is "value"
Comment #83
partyka CreditAttribution: partyka commented@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.
Comment #84
partyka CreditAttribution: partyka commented@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:
What do you think?
Comment #85
YesCT CreditAttribution: YesCT commentedthat will work. doing this now.
oops. and #81 had my manual testing code in it. I'll take that out.
Comment #86
YesCT CreditAttribution: YesCT commentedtook out my test code.
tweaked a few comments.
took out the 'can use a render array' per @joelpittet in #79
and
the "(Currently using" string is only put where the value is. so ... we dont have to check the order when it isn't there...
I think this is ready.
Comment #87
justAChris CreditAttribution: justAChris as a volunteer commentedReviewing
Comment #88
justAChris CreditAttribution: justAChris as a volunteer commentedAgree 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.
Comment #89
alexpottWe 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.
Comment #90
partyka CreditAttribution: partyka commented@alexpott: I'm sorry but I don't see a connection. How is the support for render arrays related to the issue in #2156401?
Comment #91
stefan.r CreditAttribution: stefan.r commented@partyka I think that issue is just an example of how to make the installer break in a test
Comment #92
partyka CreditAttribution: partyka commentedAre 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.
Comment #93
stefan.r CreditAttribution: stefan.r commentedAh 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?
Comment #94
stefan.r CreditAttribution: stefan.r commentedActually 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?
Comment #95
alexpottSo 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.Comment #96
stefan.r CreditAttribution: stefan.r commentedMy 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:This doesn't make sense considering how it's implemented in some of contrib and core;
VALUE
is almost never a version andTITLE
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.
Comment #97
partyka CreditAttribution: partyka commentedPer IRC with stefan.r:
Issue #2501835 may also have example of broken installer.
Comment #98
YesCT CreditAttribution: YesCT commentedComment #99
xjm@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
.Comment #100
partyka CreditAttribution: partyka commentedSo, I'm confused here - is an installer test needed, or not?
Comment #101
joelpittetSince 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.
Comment #102
lauriiiInstead of postponing this to 9.0.x, postponing for 8.1.x. Also demoting this to normal based on #101
Comment #104
xjm@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.
Comment #105
partyka CreditAttribution: partyka commented@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.
Comment #106
partyka CreditAttribution: partyka commentedSo, 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.
Comment #107
partyka CreditAttribution: partyka commentedUploading new patch for testing. Patch is done against re-roll as well as incorporating verbage suggested in #96
Comment #108
partyka CreditAttribution: partyka commentedPatch in #107 done against wrong branch.. please ignore
Comment #110
partyka CreditAttribution: partyka commented@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?
Comment #117
RumyanaRuseva CreditAttribution: RumyanaRuseva at FFW commentedI'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.