Double escaping of install errors

The uninstall page is being dealt with at #2305831: Double escaping on /admin/modules/uninstall

I added in the parent issue for this.

Fixed double escaped error messages during installation. Addressed comment #4. Screenshot, after fix.

Please take a look at #2311123: New inline_template render element for HTML code in PHP. You should try using one of the first three options. Since String::format() is calling SafeMarkup::set() directly, it's considered as option four which is seriously discouraged.

Try changing the file permission of sites/default/files to chmod -R 777 sites/default/files.

@sabert00th3693 The point isn't to avoid the error; the point is to correct the double escaped HTML when the error comes up.

Addressed comment #7 and used inline template.

Could we try checking if this works without the drupal_render?

It is possible that might be necessary, but ideally we want to leave that message as a render element as long as possible. Once the message is rendered as a string, it can't be altered by anything else. If that is necessary, ok, but leaving as render element is preferred.

@mdrummond I tried to fix it without using drupal_render, however it didn't work.

Okay, sounds good then. Probably want some manual testing before we RTBC. May also want a test to check output.

@mdrummond I am a new contributor, would you please let me know what needs to be done next at my end. Thanks!

Looking into this, I think there is a better fix. This string is meant to be translatable, but lost its translatability way back in #349508: Require UTF8 database encoding, and it seems to be by accident.
So, we make it translatable, and the double-escape issue is fixed at the same time.

I'm trying to remember how t() works with auto-escaping and failing. I'm concerned that's similar to a SafeMarkup approach, but I can't remember.

If anybody remembers, feel free to pipe in.

I don't think we need to test every one of the double-escaping fixes, maybe we can do that in a followup? Especially because this one is in the installer which I (maybe incorrectly) assume will be harder to test.

I manually tested this, here are before/after screenshots:

Before

After

I am not sure this is the correct fix. The message should be translatable, but the <p> tags should not be in the translatable content (nor, probably, the href URL), and adding !message to the end feels like a hack.

I will post another patch soon.
There seems to be more cases of double-escape during install.

During the requirements check, there are 3 places where can have double-escaped html. One was reported by the OP (in file system check). The other two are in the memory limit check, and required php extensions check.

Before
Memory limit

PHP extensions

After

File system

Memory limit

PHP extensions

Re #18: There are ~500 t() calls in core that include html, and 4 of them start with a <p> tag (find using grep -rn "[^a-zA-Z]t('<p"). Also, the !message is already properly escaped when it is generated (inside runTestQuery()):

    $this->fail(t($fail, array('%query' => $query, '%error' => $e->getMessage(), '%name' => $this->name())));

So, I don't think there is a problem with the t() call. But, you were right about the href URL, fixed that in the last patch.

@herom,
Don't you think that instead of having so much in inline_template we could use a proper twig template? it will be simple and much easier to manage and good for readability.
What are your thoughts on these?

@ivanjaros, I don't get your thought, sorry! But don't we have {% trans %}? Please correct me if I'm wrong. Why we do require to create 200 templates?

1. +++ b/core/modules/system/system.install
   @@ -107,15 +107,19 @@ function system_requirements($phase) {
        $item_list = array(
          '#theme' => 'item_list',
          '#items' => $missing_extensions,
        );
   -    $description .= drupal_render($item_list);
   

   I don't understand why we have to merge the list and the preceding paragraph into a single template.

   Can't we simply use a simple t() string in #prefix in this case?

   Or alternatively, two separate render element keys, first #markup, second #theme item_list.

2. +++ b/core/modules/system/system.install
   @@ -188,26 +192,32 @@ function system_requirements($phase) {
   +      $description = array(
   +          '#type' => 'inline_template',
   +          '#template' => "{% if phase == 'install' %}
   
   @@ -396,39 +407,64 @@ function system_requirements($phase) {
   +    $requirements['file system']['description'] = array(
   +      '#type' => 'inline_template',
   +      '#template' => "{% for directory in directories %}
   

   This looks pretty horrible to my eyes... What's the reason for using an inline template here?

   We're still able to concatenate translated strings, no?

@aneek, I don't know. Should I move them into core/modules/system/templates/status-report.html.twig (where they are currently printed), or create separate templates (status-report-memory-limit.html.twig, status-report-file-system.html.twig)?

@sun,
1. moved into #prefix.
2. If the t() calls generate html (%variables do), we can't concatenate them without another SafeMarkup::set() call. But, I don't know if there is a better way than using template?

@herom,
I think it will be good to place those in separate template files. Lets ask othes too.

@ivanjaros, it's okay man. Happens :-)

@sun, do you think it will be a good idea to clean these inline_template and placing those as twig templates in system module?

I'd recommend to ditch the template based approach altogether, primarily because it duplicates the same logic into two spots.

If Drupal is no longer able to concatenate strings (…really?), then I'd simply recompose the individual strings into separate #markup elements of a render array. It looks like most of them are exclusive/substitutes for a single 'error' anyway, optionally followed by a 'help'.

@sun, then we can wait for #2324371: Fix common HTML escaped render #key values due to Twig autoescape to get into core. This will eventually solve this issue, if we are using #markup elements.

Closed #2341607: HTML tags in installer error message as a duplicate of this issue.

@Cottser, I'm posting patch for #2317281: Double escaping of install errors for review/incorporated.

Are the bits of html I see in the Field UI part of this same issue or something else?

Patch doesn't apply anymore. Working on it :)

I fixed database settings form errors in #2346249: Fix wrong escaped errors in Installer - Database settings. Working on the rest errors.

Also fixed "Requirements" errors in #2346287: Installer requirements errors escaped HTML in variables..

@yoroy I think that #33 is refereing to field_prefix/field_suffix... which I'm looking at fixing here #2324371: Fix common HTML escaped render #key values due to Twig autoescape though I stalled on that one because I feel a bit uneasy about the solution direction.

@rteijeiro is splitting this patch up a bit because it's easier to review/test and commit that way in chunks that can be individually confirmed that the strings are coming from a safe source or an alternative solution to their escaping is provided. Thanks you for that and please continue, we already got the first one in on this so it can be rolled out of this patch.

I have a bias toward the simplest effective solution here, so here's the one line patch.

#2346287: Installer requirements errors escaped HTML in variables. This one got committed, not sure which one is right, but at least it's fixed in some regard.
Closing this as a duplicate, I do like the solution here too. May be worth opening a follow-up to clean that up a bit if possible.

#markup although is roughly equivalent to SafeMarkup::set in terms of security concerns and also because it will call that in the end anyway. Though the source of variables in question are safe.