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

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.

  1. Ensure you have curl installed
  2. Apply 2505499-52-testing-do-not-test.patch (or the one in comment #62
  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.

steps to manually test update descriptions

  1. install drupal, log in as admin
  2. enable the aggregator module (with no changes)
  3. apply patch like in #75 (can toggle commenting and uncommenting either the string or the render array)
  4. go to /drupal/update.php
Files: 
CommentFileSizeAuthor
#110 explicitly_support-2505499-110.patch19.72 KBpartyka
#108 explicitly_support-2505499-108.patch16.77 KBpartyka
#107 explicitly_support-2505499-107.patch1.69 KBpartyka
#105 explicitly_support-2505499-105.patch16.79 KBpartyka
#86 interdiff.2505499.81.85.txt5.81 KBYesCT
#86 2505499.85.patch16.79 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,150 pass(es). View
#81 2505499.81.patch19.52 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,113 pass(es). View
#81 interdiff.2505499.69.81.txt4.38 KBYesCT
#76 update-74-do-not-test.patch1.24 KBYesCT
#76 head-render-update-requirements.png141.47 KBYesCT
#74 head-no-update-for-you.png158.45 KBYesCT
#72 update-do-not-test.patch915 bytesYesCT
#69 2505499-68.patch19.05 KBpartyka
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,083 pass(es). View
#69 interdiff-2505499-56-68.txt3.72 KBpartyka
#62 render-try-render-description-do-not-test.patch1011 bytesYesCT
#62 render-try-string-description-do-not-test.patch888 bytesYesCT
#62 before-description-string-requirements.png305.32 KBYesCT
#62 render-array-requirements.png210.46 KBYesCT
#62 string-description-requirements.png285.16 KBYesCT
#56 2505499-56.patch15.82 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,330 pass(es). View
#56 interdiff-52-56.txt1.57 KBstefan.r
#52 2505499-52-testing-do-not-test.patch716 bytespartyka
#52 interdiff-2505499-45-52.txt1.31 KBpartyka
#52 2505499-52.patch15.55 KBpartyka
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,464 pass(es). View
#45 interdiff-43-45.txt8.45 KBxjm
#45 hook-requirements-2505499-45.patch15.64 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,465 pass(es). View
#36 install_profile.png23.41 KBxjm
#34 interdiff-30-34.txt756 bytesxjm
#34 hook-requirements-2505499-34.patch15.23 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,409 pass(es). View
#30 interdiff-25.txt3.52 KBxjm
#30 hook-requirements-2505499-30.patch14.84 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,399 pass(es), 1 fail(s), and 0 exception(s). View
#25 2505499-25-FAIL.patch12.08 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,374 pass(es), 11 fail(s), and 6 exception(s). View
#25 interdiff-19-25.txt632 bytesstefan.r
#19 2505499-19-FAIL.patch12.16 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,218 pass(es), 173 fail(s), and 0 exception(s). View
#19 interdiff-17-19.txt728 bytesstefan.r
#19 2505499-19-FAIL.patch12.16 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,230 pass(es), 170 fail(s), and 0 exception(s). View
#18 less-syntax-fail.txt549 bytesxjm
#18 hook-requirements-2505499-17-FAIL.patch12.05 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,345 pass(es), 11 fail(s), and 6 exception(s). View
#11 interdiff.txt4.22 KBxjm
#11 hook-requirements-2505499-11.patch6.72 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,706 pass(es). View
#7 interdiff.txt3.33 KBxjm
#7 hook-requirements-2505499-7.patch4.15 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,701 pass(es). View
#5 hook-requirements-2505499-5.patch837 bytesxjm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,697 pass(es). View

Comments

xjm’s picture

Issue tags: +SafeMarkup
joelpittet’s picture

Status: Active » Postponed

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()

YesCT’s picture

xjm’s picture

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.

xjm’s picture

Status: Active » Needs review
FileSize
837 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,697 pass(es). View

Patch.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Should add explicit test coverage too.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,701 pass(es). View
3.33 KB

Comme ça.

xjm’s picture

  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.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
13.61 KB
+++ 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

xjm’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.33 KB
6.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,706 pass(es). View
4.22 KB

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:

xjm’s picture

Issue summary: View changes
Status: Needs review » Postponed

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(). :)

joelpittet’s picture

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!

xjm’s picture

Assigned: Unassigned » xjm
Status: Postponed » Needs work

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
12.06 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Module/HookRequirementsTest.php. View

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.

Status: Needs review » Needs work

The last submitted patch, 15: hook-requirements-2505499-15-FAIL.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
5.71 KB
12.16 KB

fixing that fatal error, $description_array was declared twice

xjm’s picture

FileSize
12.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,345 pass(es), 11 fail(s), and 6 exception(s). View
549 bytes

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

stefan.r’s picture

FileSize
12.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,230 pass(es), 170 fail(s), and 0 exception(s). View
728 bytes
12.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,218 pass(es), 173 fail(s), and 0 exception(s). View

Seems we had the same thought there :)

The last submitted patch, 18: hook-requirements-2505499-17-FAIL.patch, failed testing.

The last submitted patch, 19: 2505499-19-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2505499-19-FAIL.patch, failed testing.

xjm’s picture

Hm, those still aren't the right failures.

stefan.r’s picture

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

s/testing/Testing?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
632 bytes
12.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,374 pass(es), 11 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 25: 2505499-25-FAIL.patch, failed testing.

xjm’s picture

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

Those are the right fails.

xjm’s picture

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.

xjm’s picture

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

xjm’s picture

Assigned: xjm » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
14.84 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,399 pass(es), 1 fail(s), and 0 exception(s). View
3.52 KB

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.
stefan.r’s picture

Issue tags: +Needs tests
xjm’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 30: hook-requirements-2505499-30.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
15.23 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,409 pass(es). View
756 bytes
xjm’s picture

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.

xjm’s picture

Issue summary: View changes
FileSize
23.41 KB

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.

xjm’s picture

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.

xjm’s picture

xjm’s picture

Issue summary: View changes
xjm’s picture

@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.

xjm’s picture

Issue tags: +Needs followup

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

stefan.r’s picture

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

Wim Leers’s picture

Status: Needs review » Needs work
  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.

xjm’s picture

Assigned: Unassigned » xjm

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.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs followup +Needs manual testing
FileSize
15.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,465 pass(es). View
8.45 KB

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.

xjm’s picture

Issue summary: View changes

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

xjm’s picture

Issue summary: View changes
Wim Leers’s picture

#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?

Besides the manual testing and preventing problems in the edge case outlined below, I think this patch is ready :)
+++ 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.

partyka’s picture

Assigned: Unassigned » partyka

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

YesCT’s picture

I'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

YesCT’s picture

just 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

partyka’s picture

Assigned: partyka » Unassigned
Issue summary: View changes
FileSize
15.55 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,464 pass(es). View
1.31 KB
716 bytes

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.
YesCT’s picture

I'm going to review this. and check on the manual testing.

Wim Leers’s picture

+++ 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.

alexpott’s picture

Status: Needs review » Needs work
+++ 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?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
15.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,330 pass(es). View
Wim Leers’s picture

#56: yes! Perfect.

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

partyka’s picture

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.

Wim Leers’s picture

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

YesCT’s picture

maybe we should have a test then, that we can show fails for #52, and asserts the description is first.

YesCT’s picture

  1. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -8,24 +8,226 @@
    +    // If the requirements description was a render array, check that it was
    +    // rendered correctly.
    +    if ($this->descriptionArray) {
    +      $this->assertRaw('Requirements 1 render array with <em>markup!</em>');
    +      $this->assertText('Requirements 1 first item');
    +      $this->assertText('Requirements 1 second item');
    +    }
    

    can we check here that it is first?

  2. +++ b/core/modules/system/tests/modules/requirements1_test/requirements1_test.install
    @@ -2,18 +2,53 @@
    +  // Use a render array or a string for the description based on the value set
    +  // in the test.
    +  if (\Drupal::state()->get('requirements1_test.description_array')) {
    +    $requirement['description'] = [
    +      '#theme' => 'item_list',
    +      '#title' => t('Requirements 1 render array with <em>markup!</em>'),
    +      '#items' => ['Requirements 1 first item', 'Requirements 1 second item'],
    +    ];
    +  }
    

    and here set a weight that would cause it to be in the wrong order (for the earlier patch).

YesCT’s picture

manual 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

partyka’s picture

@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

justAChris’s picture

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

partyka’s picture

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).

partyka’s picture

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

partyka’s picture

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.

YesCT’s picture

Issue summary: View changes

going to start working on manually testing the update phase. (mentioned in #45 6.)

partyka’s picture

FileSize
3.72 KB
19.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,083 pass(es). View

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.

partyka’s picture

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

stefan.r’s picture

...and the value field as well

YesCT’s picture

FileSize
915 bytes

attaching changes, showing my attempt (which does not make aggregator show up on /admin/modules/update)
why? hm.

partyka’s picture

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

YesCT’s picture

Issue summary: View changes
FileSize
158.45 KB

so. have to go to /drupal/update.php

putting some steps to test in the issue summary.

next testing with the patch and render arrays.

alexpott’s picture

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

YesCT’s picture

render array works in head for update requirement description.

markup:

      </tr>
            <tr class="system-status-report__entry color-error">
            <td class="system-status-report__status-icon system-status-report__status-icon--error">
          <div title="Error">
            <span class="visually-hidden">Error</span>
          </div>
        </td>
        <td class="system-status-report__status-title"></td>
        <td>
          
                      <div class="description"><div class="item-list"><h3>Requirements 1 render array with <em>markup!</em></h3><ul><li>Requirements 1 first item</li><li>Requirements 1 second item</li></ul></div></div>
                  </td>
      </tr>

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

YesCT’s picture

  1. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -89,6 +89,73 @@ function testHookRequirements() {
    +   * Tests the result hook_requirements() to make sure that the message
    +   * that lists "value" requirements comes after the description.
    

    https://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."

  2. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -89,6 +89,73 @@ function testHookRequirements() {
    +   * @see https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.api.php/function/hook_requirements/8
    

    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.

  3. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -89,6 +89,73 @@ function testHookRequirements() {
    +    // First, let's make sure the test module is not installed.
    

    I'll simplify this to be "Make sure..."

  4. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -89,6 +89,73 @@ function testHookRequirements() {
    +    // Configure the module's hook_requirements(). Testing with
    +    // render array off and use value off.
    ...
    +    parent::assertNoText('second item (Currently using');
    +    parent::assertNoText('markup!. (Currently using');
    

    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.

YesCT’s picture

regarding #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

 * @return
 *   An associative array where the keys are arbitrary but must be unique (it
 *   is suggested to use the module short name as a prefix) and the values are
 *   themselves associative arrays with the following elements:
 *   - title: The name of the requirement.
 *   - value: The current value (e.g., version, time, level, etc). During
 *     install phase, this should only be used for version numbers, do not set
 *     it if not applicable.
 *   - description: The description of the requirement/status.
 *   - severity: The requirement's result/severity level, one of:
 *     - REQUIREMENT_INFO: For info only.
 *     - REQUIREMENT_OK: The requirement is satisfied.
 *     - REQUIREMENT_WARNING: The requirement failed with a warning.
 *     - REQUIREMENT_ERROR: The requirement failed with an error.
joelpittet’s picture

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’s picture

+++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
@@ -8,24 +8,293 @@
+    $this->container->get('module_installer')->uninstall(['requirements1_test']);
+    $this->assertModules(['requirements1_test'], FALSE);

why uninstall? it should have not installed, right?

YesCT’s picture

FileSize
4.38 KB
19.52 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,113 pass(es). View

and... 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

YesCT’s picture

I'm done for the day if anyone else wants to pick up the review.

+++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
@@ -8,24 +8,283 @@
+   * Tests hook_requirements() message lists "value" after the description.

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"

partyka’s picture

@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.

partyka’s picture

@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?

YesCT’s picture

Status: Needs review » Needs work

that will work. doing this now.

oops. and #81 had my manual testing code in it. I'll take that out.

YesCT’s picture

Status: Needs work » Needs review
FileSize
16.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,150 pass(es). View
5.81 KB

took out my test code.

tweaked a few comments.

took out the 'can use a render array' per @joelpittet in #79

and

+++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
@@ -8,24 +8,283 @@
+    parent::assertNoText('second item (Currently using');
+    parent::assertNoText('markup!. (Currently using');

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.

justAChris’s picture

Assigned: Unassigned » justAChris

Reviewing

justAChris’s picture

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

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

partyka’s picture

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

stefan.r’s picture

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

partyka’s picture

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.

stefan.r’s picture

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?

stefan.r’s picture

+++ 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?

alexpott’s picture

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.

stefan.r’s picture

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.

partyka’s picture

Per IRC with stefan.r:

Issue #2501835 may also have example of broken installer.

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs tests
xjm’s picture

@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.

partyka’s picture

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

joelpittet’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Needs work » Postponed

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.

lauriii’s picture

Version: 9.x-dev » 8.1.x-dev
Priority: Major » Normal

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue summary: View changes
Status: Postponed » Needs work
Issue tags: +Needs change record

@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.

partyka’s picture

@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.

partyka’s picture

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.

partyka’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

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

partyka’s picture

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

The last submitted patch, 107: explicitly_support-2505499-107.patch, failed testing.

partyka’s picture

@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?

Status: Needs review » Needs work

The last submitted patch, 110: explicitly_support-2505499-110.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.