Installer requirements errors escaped HTML in variables.

+++ b/core/modules/system/system.install
@@ -385,7 +385,6 @@ function system_requirements($phase) {
-  $error = '';

@@ -397,26 +396,26 @@ function system_requirements($phase) {
+      $description = [];

The error presumably get's built up separately from the description for some reason. There is a code path where the error could be added to but not trigger the description to be displayed.

Maybe go with $error = [];
And build that up separately and prepend it on to your $description?

Above patch uses "SafeMarkupSet" which is to be avoided based on the discussion on [#2311123]

Attached patch solves double escaping for:

• php extensions (issue 2346509)
• memory limit
• writable filesystem

It doesn't use SafeMarkup but uses 'inline_template'.

Patch was created together with Zekvyrin (https://www.drupal.org/u/zekvyrin) at Drupalcon Amsterdam.

Confirming that #7 patch solved the issues

Variable names need to follow the coding standards. I would also use an array, $description[] with keys (ex $description['phase'], $description['memory'] ) rather than $desc_first and $desc_second.

Good point, makes sense.

Attached patch is properly naming variables :)

Before patch

After

Good catch! Attaching new patch, don't really know how to make an interdiff :|

I'll do my best to fix it. I think I'm on a good track!

update: I was at the hackerspace, I don't see you online atm so I'll ping tomorrow (unless I find the answer myself first)

Tested and working as expected.
I was trying to fix this issue. And it looks like you fixed it in a lot better way than i had. I think this is a good patch

Marked #2348431: Requirements review box showing code instead of rendering text as a duplicate

@akoe and me just encountred this one as well. Specifically the missing files/ directory one is also showing up in the status report page.

We approached this with SafeMarkup as well, before stumpling on this issue.

We tested and reviewed this patch and it works fine on install as well as status page.

It was hard to find this issue, though I have not better title proposal.

Can we add a test to test for double escaped html?

While I was working on the patch, Zekvyrin created an installation environment that would fail on all requirements (!) so as to test all situations.

I can't really tell if we can add a test

so we would need a add new environment to the test we can do that. I was thinking can we test for double escaped looking at the output for the tests?
We have it all over D8

like on error on the module page

+++ b/core/modules/system/system.install
@@ -115,7 +115,17 @@ function system_requirements($phase) {
-    $description .= drupal_render($item_list);
...
+        'var' => drupal_render($item_list),
...
+    $description = drupal_render($description);

@@ -190,24 +200,37 @@ function system_requirements($phase) {
+      $description = drupal_render($description);

@@ -234,7 +257,17 @@ function system_requirements($phase) {
+        $description = drupal_render($description);

@@ -400,22 +433,31 @@ function system_requirements($phase) {
+        $description = drupal_render($description);

There is no need to do the rendering here - we are returning one big render array. Which means we can do something like the patch attached.

I can confirm that alexpott's modification works perfectly to my enviroment:

Some nitpicks a but some possible caveats still to take care of as well in this iteration:

Thanks for keeping on top of this issue!

1. +++ b/core/modules/system/system.install
   @@ -112,11 +112,18 @@ function system_requirements($phase) {
   +    // We use twig inline_template to avoid double escaping.
   
   @@ -231,11 +250,17 @@ function system_requirements($phase) {
   +        // We use twig inline_template to avoid double escaping.
   

   We are avoiding "escaping", not "double escaping".

2. +++ b/core/modules/system/system.install
   @@ -191,24 +198,36 @@ function system_requirements($phase) {
        $description = '';
   ...
   +      $description['phase'] = t('Consider increasing your PHP memory limit to %memory_minimum_limit to help prevent errors in the installation process.', array('%memory_minimum_limit' => DRUPAL_MINIMUM_PHP_MEMORY_LIMIT));
   

   Description is being treated as an array and probably should be initialized as one.

3. +++ b/core/modules/system/system.install
   @@ -401,22 +426,30 @@ function system_requirements($phase) {
   -        $error .= t('The directory %directory does not exist.', array('%directory' => $directory)) . ' ';
   +        $error = t('The directory %directory does not exist.', array('%directory' => $directory));
   ...
   -        $error .= t('The directory %directory is not writable.', array('%directory' => $directory)) . ' ';
   

   Where is the $error initialized before for this concatenation, we may need more work here...

4. +++ b/core/modules/system/system.install
   @@ -401,22 +426,30 @@ function system_requirements($phase) {
   +        $description = array(
   +         '#type' => 'inline_template',
   +         '#template' => '{{ error }} {{ description }}',
   +         '#context' => array(
   +           'error' => $error,
   +           'description' => $description,
   +         ),
   +        );
   

   just needs one more space of indent to make it 2 spaces.

Fixed 1,2 & 4 from comment #26.

About 3 ($error's concat), I don't think that this should change.
After $error is initialized, and might or might not be overridden once. It's no longer concatenated with description the way is was.

Good Work :)
I agree with your comments, description cannot be initialized as an array as it was treated as string in the previous statements.

Its working.

Tested #30 working fine refer screenshot #32

Tested #30 working fine refer screenshot #32

Thanks @Zekvyrin for the fixes and I agree with you on the #27.3 regarding the $error variable. It's weird because it looks like it could be collecting the $errors but the likelihood of that ever happening is 0 I believe because the $phase won't change in the loop.

RTBC++

Is it worth an actual template for this rather than an inline one?

@catch what do you mean by actual template? None of the patches introduced a template file.

@bserem yes that's the question - would it be better to introduce an actual template file.

Understood now.

I can't really answer that. The issue seems resolved with the patches provided, and it is not a bad solution either I believe.
A template file would definitely postpone this more, not that we are in a hurry :)
Plus I don't get what the benefits of a template file would be (I really don't, I'm not that up to date with D8 yet).
Can you please share some intel on the subject?

@catch: I don't think we can answer that properly in this issue. Maybe a more 'global' answer is needed.
We thought this issue is in "small snippets of HTML" category, so we implemented 2nd option and not 1st from instructions:
https://www.drupal.org/node/2311123

As I saw, parent issue #2297711: Fix HTML escaping due to Twig autoescape is postponed for now.. so I guess this one has to wait.
I guess other issues like #2273925: Ensure #markup is XSS escaped in Renderer::doRender() should be resolved first and then we'll review these issues.

This one doesn't need to wait. The parent was postponed while all the children issues are gone through. Which is what we are doing here.

catch, fixes of #2297711: Fix HTML escaping due to Twig autoescape have tended to be in the "getting things fixed" category. Not introducing new functionality. To convert to a template, let's do that in a follow-up.

Ok then, I'm setting it to RTBC as it was before questioning about making it a template.

And we might revisit this issue as a follow up.

This patch works for me. The html tags are displayed correctly.

However, I noticed several other issues: the links should be https instead http, we are not using the phrase "handbook" anymore but "online documentation", and the link should rather go to the installation guide and not to info about webhosting.

Since these issues are also relevant for some of the other messages in the system.install file: Should I wait until this patch is committed, and then make a new patch for all of them?

The issues you are mentioning are not exactly relevant to what we are trying to solve here.

Valid as they are, they should probably be solved on another issue.
Just my opinion :)

OK thanks for the answers.

@ifrik well spotted - please open a new issue for the things you found!

Committed/pushed to 8.0.x, thanks!