Remove SafeMarkup::set() in file_save_upload() and allow render/template code to control a single-item list

Very nice and thank you for the manual testing!

Having the extra space in there with the UL item list doesn't matter in HTML and this is much cleaner.

Thanks @cwells and @joelpittet. +1 for the manual testing.

In this issue I think we should refactor the code flow a bit. In general, rather than using SafeMarkup::format() to stick together two variables that have already been individually sanitized and added to the safe list with the if/else deciding what those variables are, it would be better to have two separate code paths that each assemble the markup in the appropriate place. This both makes it easier to understand what the code is doing (and that it will neither introduce double-escaping nor XSS) and reduces the overhead.

Ideally, we'd do away with the drupal_render() call entirely. But (as @alexpott pointed out), in this case it's being passed to drupal_set_message() which doesn't support render arrays. I'd like to see an issue to discuss whether we should optionally make drupal_set_message() support render arrays by at the minimum using the renderer itself when needed (there are pros and cons to that), or using hashing or suchlike to provide the array keys (pros and cons to that as well) and supporting actual render arrays in a bubbleable-and-best-practice-and-etc. fashion. All that is totally out of scope here, though. So an @todo and a followup for that would be lovely. "Decide how and whether to support render arrays for drupal_set_message()" would probably be the title.

However, even within the function itself, we can simplify it. Regarding this change:

+++ b/core/modules/file/file.module
@@ -856,11 +856,12 @@ function file_save_upload($form_field_name, $validators = array(), $destination
-        $message = SafeMarkup::set($message . drupal_render($item_list));
+        $error_text = drupal_render($item_list);

In HEAD, that is taking one string, and appending it to a rendered render array. It'd be better to just put the string in the render array first (since we already have one), and then do the drupal_render() on the whole thing.

I also question why we have this if/else in the first place. Looking at the whole of file_save_upload(), it's there solely to make it an item list if there's more than one item, but a single string if it's not. That seems wrong to me -- a list of only one item is still a list, and it should be up to the themeable output to decide what to do with a list that has only one item in it, not a particular CRUD operation. So I'd consider removing that else condition with the array_pop() entirely (we'd compare screenshots for a single item) and then with the suggestion above we just have one render array with one drupal_render() call only, and no extra SafeMarkup anything.

follow-up: #2505497: Support render arrays for drupal_set_message()

I took the approach as suggested in #11. $message is an array with error and item_list arrays and adding render on drupal_set_message, got rid of if and else statement so now it displays the item list regardless of number of items. I have added test module which I created to test the error message on the node edit/add page.

Added screenshot here

I dont see what in the patch is making this happen...

but just uploading a txt file, does not cause the list.

doing a different test,
changing the article content type field setting for image to have a max allowed size,
and uploading a png greater than that,
then I *do* get the list,
but... I get it twice.

I dont know why.

Actually, the double error message is in head also. So I do not think this issue needs to fix that. ... maybe it is already covered by a child of the follow-up to the inline error issue? #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

[edit: Maybe it is #2482783: File upload errors not set or shown correctly ?]

here is the screenshot of head.

I dont see what in the patch is making this happen...

but just uploading a txt file, does not cause the list.

If you try to upload a txt file for an image field in article content type, you get the error which is coming from file.js line 124

var error = Drupal.t("The selected file %filename cannot be uploaded. Only files with the following extensions are allowed: %extensions.",

soo... there is no safeMarkup::set() in the js.

So I think the patch might be ok.

+++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
@@ -298,7 +298,8 @@ public function testDefaultImages() {
-    $this->assertText(t('The specified file text-0.txt could not be uploaded. Only files with the following extensions are allowed: png gif jpg jpeg.'), 'Non-image file cannot be used as default image.');
+    $this->assertText('The specified file text-0.txt could not be uploaded.', 'Non-image file cannot be used as default image.');
+    $this->assertText('Only files with the following extensions are allowed: png gif jpg jpeg.', 'Non-image file cannot be used as default image.');

I wonder if this change is not necessary. Since that error comes from the js and is not reformatted in this issue.

[edit]
oops. I see it was an actual fail in #13.. I will look into why, if the error is coming from the js which is not changed.

To get the list without the module, set a small (100) maximum file size, and a large (20000 x 20000) minimal resolution, and select an image that is .. medium say: 2M and 1440 × 960 or so.

------------

Another problem I noticed that already exists in head, which most likely would only happen to people testing this,
is that if the node/add/article page is reloaded (so no error is present)
and the maximum file size is set, and the minimal resolution,
and try and upload an image,
see the list of the two errors,
in another window, change the article content type image field to have a giant maximum file size,
back in the node add window, try to upload the image again...
and instead of only seeing the minimal resolution error, both errors are still there.
but, if reload the node/add/article page
then only the one appropriate error is shown.

again, this occurs in head, so not the problem of this issue.

answering my own question from #21

simpletest does not have javascript. :) so the error is displayed as a list.

updated issue summary.

I think all the things @xjm asked for have been done in the patch,
and the concerns I found are not really to be solved in this issue.

rtbc.

1. +++ b/core/modules/file/file.module
   @@ -850,18 +850,18 @@ function file_save_upload($form_field_name, $validators = array(), $destination
   +      $message = array(
   +        'error' => array(
   +          '#markup' => t('The specified file %name could not be uploaded.', array('%name' => $file->getFilename())),
   +        ),
   +        'item_list' => array(
   

   Nice, this is very clear now.

2. +++ b/core/modules/file/file.module
   @@ -850,18 +850,18 @@ function file_save_upload($form_field_name, $validators = array(), $destination
   +      // @todo Add support render_arrays for drupal_set_message. See
   +      //  https://www.drupal.org/node/2505497
   

   Minor: I don't think we need the underscore for "render arrays". Also drupal_set_message() should have parentheses at the end, and I guess we need a period at the end of the sentence.

3. +++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
   @@ -298,7 +298,8 @@ public function testDefaultImages() {
   +    $this->assertText('The specified file text-0.txt could not be uploaded.', 'Non-image file cannot be used as default image.');
   +    $this->assertText('Only files with the following extensions are allowed: png gif jpg jpeg.', 'Non-image file cannot be used as default image.');
   
   +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
   @@ -45,7 +45,8 @@ function testResolution() {
   +    $this->assertRaw(t('The specified file %name could not be uploaded.', array('%name' => $image_that_is_too_small->filename)), 'Node save failed when minimum image resolution was not met.');
   +    $this->assertRaw(t('The image is too small; the minimum dimensions are %dimensions pixels.', array('%dimensions' => '50x50')), 'Node save failed when minimum image resolution was not met.');
   

   So it makes sense for these to be two separate assertion messages now. However, having two separate assertions with the same assertion message adds difficulty for debugging.

   I'd actually just remove the assertion message from all four of these assertions now. assertText() has an okay default for it.

Looking at this now.

Made changes per 2 and 3 above.

Looking good!

Before RTBCing, can we get input on whether removing the special case for one entry in the item list is out of scope or not desirable?

If we think so, then we should still use a render array, but the else condition would use it as well.

Based on reading through the issue and installing/testing the patch locally, as well as talking with @davidhernandez IRL, I think it is desirable to keep this for consistency. However, we should get usability feedback whether or not it is ok to leave the one bullet point.

Accidentally changed status, setting it back.

If this had to have a new patch made again, someone could fix the space on the indent of the second line of the @todo to be indented two spaces.
https://www.drupal.org/node/1354#todo

but that is very not blocking.

Updated issue summary to reflect that this issue needs usability review, as well as provided additional screenshots of more than one error. Lastly, this issue does not require automated test coverage for the sanitization of the string, since it was only previously calling SafeMarkup::set(), which is consistent with other child issues of the meta #2280965: [meta] Remove every SafeMarkup::set() call.

I am not a big fan of single bullet point errors (bullets are visually, much more sensible when there are several). However it is by large a non-issue from a usability perspective. So I think its OK, to move forward with this direction.

In this case we lead with "upload is failing" not a specific error. If we were to lead with a specific error, it would be weird to not have this in a bullet.

Regarding #29, we should always avoid changes that are visually different from the semantic intent - unless it has big UX benefits.

Need to add another assertText for 'Image exceeding max resolution was properly resized.', since that text was taken out since it's on another line.

oops - mis-read the change. I thought it was like the concatenated string above in the patch. That was just removing the assertion message to actually show the text being searched for.

Retitling to include the scope of the item list simplification.

Nice work! More and more, render arrays seem like a sound solution. @alexpott also looked this over and was +1.

I like also that we removed some unneeded theme-babysitting, if you will.

As a required part of a critical issue, this issue can be completed any time during the beta phase per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Congratulations @jcloys on your first Drupal 8 commit credit!

I made a small docs fix on commit:

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index 912e9b9..9f0074f 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -859,7 +859,7 @@ function file_save_upload($form_field_name, $validators = array(), $destination
           '#items' => $errors,
         ),
       );
-      // @todo Add support render arrays for drupal_set_message(). See
+      // @todo Add support for render arrays in drupal_set_message()? See
       //  https://www.drupal.org/node/2505497.
       drupal_set_message(\Drupal::service('renderer')->render($message), 'error');
       $files[$i] = FALSE;