Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Issue tags: +Novice
Samvel’s picture

Assigned: Unassigned » Samvel
Samvel’s picture

Status: Active » Needs review
Issue tags: +CodeSprintUA
FileSize
9.93 KB

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
FileSize
7.34 KB

Status: Needs review » Needs work

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

c4rl’s picture

Status: Needs work » Needs review
FileSize
7.35 KB
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
FileSize
9.01 KB
9.87 KB

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
FileSize
9.85 KB

needed a reroll

Status: Needs review » Needs work

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

hussainweb’s picture

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
FileSize
747 bytes
10.57 KB

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.

star-szr’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.

star-szr’s picture

Assigned: c4rl » Unassigned
Issue tags: +Needs reroll

Tagging for reroll.

benjifisher’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.58 KB

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
FileSize
19.93 KB
53.33 KB
19.19 KB
52.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
FileSize
746 bytes
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