Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
To Test:
1. Add file field to article
2. Create an article upload a file

Files: 
CommentFileSizeAuthor
#36 drupal-file-2009014-36.patch746 bytesStephaneQ
PASSED: [[SimpleTest]]: [MySQL] 57,488 pass(es).
[ View ]
#31 After Edit Field.jpg52.8 KBadamcowboy
#31 After Field Icon.jpg19.19 KBadamcowboy
#31 Before Edit Field.jpg53.33 KBadamcowboy
#31 Before Field Icon.jpg19.93 KBadamcowboy
#30 drupal-file-2009014-30.patch10.58 KBbenjifisher
PASSED: [[SimpleTest]]: [MySQL] 57,276 pass(es).
[ View ]
#23 drupal-file-2009014-23.patch10.57 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file-2009014-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 drupal-file-2009014-interdiff-20-23.txt747 byteshussainweb
#20 drupal-file-2009014-20.patch9.84 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 56,739 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#20 drupal-file-2009014-interdiff-18-20.txt1.34 KBhussainweb
#18 drupal-file-2009014-18.patch9.85 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 55,564 pass(es), 101 fail(s), and 156 exception(s).
[ View ]
#15 drupal-2009014-15.patch9.87 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 58,101 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#15 drupal-2009014-15.patch-interdiff.txt9.01 KBc4rl
#12 drupal-2009014-12.patch7.35 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 0 fail(s), and 235 exception(s).
[ View ]
#10 drupal-2009014-10.patch7.34 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.php.
[ View ]
#3 replace-theme-with-render-2009014-3.patch9.93 KBSamvel
PASSED: [[SimpleTest]]: [MySQL] 56,081 pass(es).
[ View ]

Comments

thedavidmeister’s picture

Issue tags:+Novice
Samvel’s picture

Assigned:Unassigned» Samvel
Samvel’s picture

Status:Active» Needs review
Issue tags:+CodeSprintUA
StatusFileSize
new9.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,081 pass(es).
[ View ]

attached

podarok’s picture

Status:Needs review» Reviewed & tested by the community

#3 works for me
rtbc

thedavidmeister’s picture

Status:Reviewed & tested by the community» Needs work

In theme_file_formatter_table() I don't think we need to drupal_render() all the table cells here If we structure $table correctly.

IIRC, in _theme_table_cell() it looks like if we use the 'data' syntax for individual table cells theme_table() will call drupal_render() for us.

instead of:

    $table['#rows'][] = array(
      drupal_render($file_link),
      format_size($item['filesize']),
    );

could we try:

$table['#rows'][] = array(array('data' => $file_link), array('data' => format_size($item['filesize']));
Samvel’s picture

Are you sure? Because seems i try this, and it not workink, because drupal_render(which will render $table) will not render anythinh in #rows.

thedavidmeister’s picture

give me a minute, I'll do a test to double check.

thedavidmeister’s picture

  $table = array('#theme' => 'table', '#header' => array(t('Attachment'), t('Size')));
  $table['#rows'] = array();
  foreach ($variables['items'] as $delta => $item) {
    $file_link = array('#theme' => 'file_link', '#file' => (object) $item);
    $table['#rows'][] = array(
      array('data' => $file_link),
      format_size($item['filesize']),
    );
  }

Works for me, you have to set 'data' for the actual cell in the row. That said, I could see the table when I printed $table rendered but not when it was returned? not sure if I was doing something wrong there.

thedavidmeister’s picture

Assigned:Samvel» Unassigned
c4rl’s picture

Status:Needs work» Needs review
StatusFileSize
new7.34 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.php.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-2009014-10.patch, failed testing.

c4rl’s picture

Status:Needs work» Needs review
StatusFileSize
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 0 fail(s), and 235 exception(s).
[ View ]
thedavidmeister’s picture

Status:Needs review» Needs work
Issue tags:+Needs manual testing
-    return $response->addCommand(new ReplaceCommand(NULL, theme('status_messages')));
+    $build = array('#theme' => 'status_messages');
+    return $response->addCommand(new ReplaceCommand(NULL, $build));

missed a drupal_render() here?

-          '#title' => theme('file_link', array('file' => $file)) . ' ',
+          '#title' => array(
+            '#file_link',
+            '#file' => $file
+          ),
         );
       }
       else {
         $element['file_' . $delta]['filename'] = array(
-          '#type' => 'markup',
-          '#markup' => theme('file_link', array('file' => $file)) . ' ',
+          '#type' => 'file_link',
+          '#file' => $file,

The changes here mean we'll have to manually test the output, with particular attention to the trailing whitespace that was in #markup.

+  $icon_build = array(
+    '#theme' => 'file_icon',
+    '#file' => $file,
+    '#icon_directory' => $icon_directory,
+  );

Was there another variable in this scope called $file_icon? if not, can we call $icon_build $file_icon?

-      $elements['#file_upload_description'] = theme('file_upload_help', array('description' => '', 'upload_validators' => $elements[0]['#upload_validators'], 'cardinality' => $cardinality));
+      $elements['#file_upload_description'] = array(
+        '#theme' => 'file_upload_help',
+        '#description' => '',
+        'upload_validators' => $elements[0]['#upload_validators'],
+        'cardinality' => $cardinality
+      );

needs manual testing.

-      $element['#description'] = theme('file_upload_help', array('description' => $element['#description'], 'upload_validators' => $element['#upload_validators'], 'cardinality' => $cardinality));
+      $element['#description'] = array(
+        '#theme' => 'file_upload_help',
+        '#description' => $element['#description'],
+        '#upload_validators' => $element['#upload_validators'],
+        '#cardinality' => $cardinality,
+      );

also needs manual testing.

-      $data = theme('file_icon', array('file' => $fake_file));
+      $data = array(
+        '#theme' => 'file_icon',
+        '#file' => $fake_file
+      );

also needs manual testing.

c4rl’s picture

Yeah, I shouldn't write patches at 1:00 at night. :) I'll have another look at this tomorrow.

c4rl’s picture

Status:Needs work» Needs review
StatusFileSize
new9.01 KB
new9.87 KB
FAILED: [[SimpleTest]]: [MySQL] 58,101 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

Some improvements and fixes now that I am awake.

Status:Needs review» Needs work

The last submitted patch, drupal-2009014-15.patch, failed testing.

c4rl’s picture

Assigned:Unassigned» c4rl
jenlampton’s picture

Status:Needs work» Needs review
StatusFileSize
new9.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,564 pass(es), 101 fail(s), and 156 exception(s).
[ View ]

needed a reroll

Status:Needs review» Needs work

The last submitted patch, drupal-file-2009014-18.patch, failed testing.

hussainweb’s picture

StatusFileSize
new1.34 KB
new9.84 KB
FAILED: [[SimpleTest]]: [MySQL] 56,739 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

I see one of the problems here:

@@ -957,16 +960,17 @@ function file_managed_file_process($element, &$form_state, $form) {
   if (!empty($fids) && $element['#files']) {
     foreach ($element['#files'] as $delta => $file) {
       if ($element['#multiple']) {
+        $file_link = array(
+          '#theme' => 'file_link',
+          '#file' => $file,
+        );
         $element['file_' . $delta]['selected'] = array(
           '#type' => 'checkbox',
-          '#title' => theme('file_link', array('file' => $file)) . ' ',
+          '#title' => drupal_render($file_link),
         );
       }
       else {
-        $element['file_' . $delta]['filename'] = array(
-          '#markup' => theme('file_link', array('file' => $file)) . ' ',
-          '#weight' => -10,
-        );
+        $element['file_' . $delta]['filename'] = $file_link += array('#weight' => -10);
       }
     }
   }

I also have doubts about this part:

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/FileMime.php
@@ -36,8 +36,11 @@ public function buildOptionsForm(&$form, &$form_state) {
   function render($values) {
     $data = $values->{$this->field_alias};
     if (!empty($this->options['filemime_image']) && $data !== NULL && $data !== '') {
-      $fake_file = (object) array('filemime' => $data);
-      $data = theme('file_icon', array('file' => $fake_file));
+      $file_icon = array(
+        '#theme' => 'file_icon',
+        '#file' => $values->_entity,
+      );
+      $data = drupal_render($file_icon);
     }

But I am letting this part be as it seems it was working fine in #15 (at least tests passed).

hussainweb’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-file-2009014-20.patch, failed testing.

hussainweb’s picture

Status:Needs work» Needs review
StatusFileSize
new747 bytes
new10.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-file-2009014-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I am repeating the following change for theme_image_widget() which should fix those exceptions.

+++ b/core/modules/file/file.field.inc
@@ -596,7 +596,7 @@ function theme_file_widget($variables) {
   if (!empty($element['fids']['#value'])) {
     // Add the file size after the file name.
     $file = reset($element['#files']);
-    $element['file_' . $file->id()]['filename']['#markup'] .= ' <span class="file-size">(' . format_size($file->getSize()) . ')</span> ';
+    $element['file_' . $file->id()]['filename']['#suffix'] = ' <span class="file-size">(' . format_size($file->getSize()) . ')</span> ';
   }
   $output .= drupal_render_children($element);

Interdiff is attached for clarity.

ErikV’s picture

Status:Needs review» Needs work

I think this needs a reroll

benjifisher’s picture

Status:Needs work» Needs review

@ErikV:

Why do you say it needs a reroll? The patch applies cleanly locally. I will ask the testbot for its opinion.

benjifisher’s picture

#23: drupal-file-2009014-23.patch queued for re-testing.

Cottser’s picture

#23: drupal-file-2009014-23.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +Needs manual testing, +CodeSprintUA

The last submitted patch, drupal-file-2009014-23.patch, failed testing.

Cottser’s picture

Assigned:c4rl» Unassigned
Issue tags:+Needs reroll

Tagging for reroll.

benjifisher’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new10.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,276 pass(es).
[ View ]

It looks as though #2036087: Add public identifier to all render methods in all views plugins in core (commit 31efff2) conflicts. I feel like living dangerously, so I will manually edit the patch, adding "public" in the one context line that matches "function render". My patch applies cleanly, but what will the testbot say?

adamcowboy’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new19.93 KB
new53.33 KB
new19.19 KB
new52.8 KB

I applied the patch and tested it. It worked well.

I also manually tested.

Attached are screen shots of Field Icon and File Upload Field.

adamcowboy’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing, -CodeSprintUA
adamcowboy’s picture

Status:Needs review» Reviewed & tested by the community

RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed bc65847 and pushed to 8.x. Thanks!

thedavidmeister’s picture

Status:Fixed» Needs work

There is a theme() call in file_save_upload() still

StephaneQ’s picture

Status:Needs work» Needs review
StatusFileSize
new746 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,488 pass(es).
[ View ]
sbudker1’s picture

Status:Needs review» Reviewed & tested by the community

The patch worked! There were no apparent issues when I uploaded a file in an article.

xjm’s picture

#36: drupal-file-2009014-36.patch queued for re-testing.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

alexpott’s picture

Status:Fixed» Reviewed & tested by the community

Had to revert this commit because the commit contained changes from #2052995: Add a route to the frontpage see f4aead7

Committed 9614f73 and pushed to 8.x.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 8b7bc0c and pushed to 8.x. Thanks!

webchick’s picture

D'oh. Sorry about that!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Instructions how to test the patch