file.module - Convert theme_ functions to Twig

Working on this

Attached patch pulls over twig files for the file module from the twig sandbox. Once/if #1898070: file.module - Convert theme_ functions to Twig lands, theme('file_managed_field') and theme('file_formatter_table') can probably be pulled out.

Also, what's the recommended way for commenting preprocess functions for twig. In file.module in the twig sandbox there are several different methods:

/**
 * Default theme implementation to display a help text for file form field.
 *
 * $variables
 *   An associative array containing:
 *   - element: A render element representing the file.
 *
 * @see file-upload-help.html.twig
 */

/**
 * Preprocess variables for a link to a file.
 */

How verbose do we need to be?

Thanks for this, @duellj!

This is #1898070: file.module - Convert theme_ functions to Twig, though :)

We're working out the preprocess doc standards in #1913208: [policy] Standardize template preprocess function documentation.

Thanks Cottser, thought I'd ask, since the function headers are different in this patch. Good to know that preprocess headers are getting figured out, they seem pretty inconsistent.

Oh, whoops :). Now I see what you're saying. Meant to link to #1876712: [meta] Convert all tables in core to new #type 'table' in comment #3.

Started work on reviewing this, just nitpicky documentation tweaks so far so I'll roll a new patch and post in the next day or two :)

Note that this will conflict with #1839068: Implement the new entity field API for the file field type

#8 - Thanks @Berdir, the changes look pretty manageable, we can certainly reroll.

Revised patch to address some minor docs issues as well as convert theme() calls in preprocess to render arrays, thanks to @steveoliver for the help with that.

A couple things I'm not sure about:

1. Not sure why the preprocess versions of the theme_ functions originally in file.field.inc have been moved to file.module:
   • theme_file_widget()
   • theme_file_widget_multiple()
   • theme_file_upload_help()
   • theme_file_formatter_table()
2. * - element_attributes: Remaining HTML attributes for the containing element.
   Should this say "container element" instead? Found in file-widget.html.twig and file-managed-file.html.twig.

Finally, template_preprocess_file_upload_help() might benefit from a similar treatment as being discussed in #1918856: Put each module's help into a separate Twig file.

Added a commit message to the summary as well after looking at git blame and sandbox issues.

Thanks Cottser,

I wasn't sure what the process was for pulling in commits from sandboxes. Feel free to move those other functions back to file.admin.inc. I had moved them because it made sense to me that all modules preprocess functions live in one place, but I can see how keeping them in the files where they are used makes more sense.

Thanks @duellj, that's helpful. I'll reroll to split up the preprocess functions again.

BTW @c4rl put together a great screencast on how to pull in the files and changes from the sandbox, I just added a link to it in the meta issue under Resources: #1757550: [Meta] Convert core theme functions to Twig templates.

Okay, moved things back to file.field.inc and did a bit more documentation cleanup. The interdiff won't be super helpful but I've included it anyway. Better to review the actual patch, since it replaces the theme() functions in place now, resulting in a smaller patch too!

I figured that might happen, will take a look in the next day or two.

Forgot one important detail:

use Drupal\Core\Template\Attribute;

Looking at the latest patch with Dreditor I noticed the docblock in a few of the Twig template files was misaligned.

Needs a reroll now that #1839068: Implement the new entity field API for the file field type is in.

Add commit message to issue summary

- Rerolled against latest HEAD.
- Changed icon_url to src in file-icon.html.twig and removed the @todo
- Removed mention of data types in docblock in file-link.html.twig
- Removed mention of data types in docblock in file-upload-help.html.twig
- Renamed element_attributes to attributes in file-widget.html.twig
- Renamed children to element in file-widget-multiple.html.twig
- Modified file-managed-file.html.twig to be identical to file-widget.html.twig
* Created follow-up issue: #1926610: Consolidate file-managed-file.html.twig and file-widget.html.twig since they are identical

I can't confirm that file-managed-file is even being used. Could whoever tests this patch please confirm, and if so, update the issue summary to include instructions on how that can be tested?

Added how to test

Found a stray dpm(), removing it fixes the test failures locally. No other changes.

One more thought:

+++ b/core/modules/file/file.moduleundefined
@@ -588,29 +588,36 @@ function file_get_content_headers(File $file) {
+    // From file.field.inc.
     'file_widget' => array(
       'render element' => 'element',
+      'template' => 'file-widget',
     ),
     'file_widget_multiple' => array(
       'render element' => 'element',
+      'template' => 'file-widget-multiple'
     ),
     'file_formatter_table' => array(
       'variables' => array('items' => NULL),
+      'template' => 'file-formatter-table',
     ),
     'file_upload_help' => array(
       'variables' => array('description' => NULL, 'upload_validators' => NULL),
+      'template' => 'file-upload-help',
     ),

Should we add 'file' to these while we're at it?

more test

I figured out how to test a managed file (Issue summary updated), and it works! :)

I don't think we need to add a 'file' definition in hook_theme since all these preprocess functions are in file.module, or in file.field.inc, which is included at the top of file.module anyway.

Does changing t() to format_string() we lose translations of those help lines? That doesn't really seem desired.

I don't really see why moving the translation of the upload help moves from the template preprocessing to the template itself. Seems like this means we lose discoverability of those translation strings and it seems like an unnecessary change.

Here's a re-roll without the inadvertent change from t() to format_string(). Agreed, this shouldn't be in here.

And here's a version that also removes theme_file_icon
and brings preprocess docs up to new standards

And just in case anyone did want to be able to override the file icon :)

Nice work, @jenlampton!

Interdiff between #21 and #27.

#27: core-update_file_to_twig-1898070-27.patch queued for re-testing.

#27: core-update_file_to_twig-1898070-27.patch queued for re-testing.

I tested this patch and it works! However, I did spot a few minor issues:

• file.field.inc template_preprocess_file_formatter_table: doc block still missing documentation for $variables['content']; see doxygen coding standards
• file.field.inc template_preprocess_file_widget_multiple: doc block missing documentation for $variables['table'] (see above)
• file-managed-file and file-widget have identical twig templates (except for doc block); any chance we can use the same function for both?

I'm going to leave the status as "needs work" because the doc blocks really should get fleshed out.

Unassigning, haven't had time to work on this issue recently.

@koppie - thanks for the review! We'll definitely fix up the documentation.

Regarding point #3 - I don't think we'll consolidate file-managed-file and file-widget just yet, we're focusing on straight conversion for the time being. Good note though :)

Nice - for the consolidation, @jenlampton already opened an issue: #1926610: Consolidate file-managed-file.html.twig and file-widget.html.twig since they are identical

added how to test managed files

Add #type table conversion issue to related

Add conversion summary table

Tagging as Novice for the docs tweaks brought up in #33, adding variable documentation to a couple of the preprocess functions.

After looking at this patch again, the docs tweaks in #33 do not need to be done. These variables are added in the preprocess functions so they are not @params.

See file_theme().

#27: core-update_file_to_twig-1898070-27.patch queued for re-testing.

Tags.

#27: core-update_file_to_twig-1898070-27.patch queued for re-testing.

This patch will need a reroll since it no longer applies.

Rerolling instructions: http://drupal.org/patch/reroll

Linkify #type table issue

Remove sandbox link

Thanks @bdgreen! Can you please upload those two patches combined together into one .patch file? It also looks like we're missing the added .html.twig template files.

@bdgreen - Thanks! That looks like a reroll of #21, not #27. Can you confirm?

Unless otherwise specified we always reroll the latest patch on the issue. The only reason #27 is red is because I had it re-tested in #41 and it no longer applied. Before that it was green :)

I respect your patience ;) Thanks @Cottser. Redone with #27 patch ...

Thanks for your patience too @bdgreen :) I will take a look and see if I can make sense of the test failures.

@cottser, thanks. Re-rolled again today after full new install of 8.x. But cannot complete a local Testing as for whatever setup after enabling "Testing" module, clicking Configure (i.e. admin/config/development/testing) I'm given then "white screen". Note: for the patch I did uncomment the three twig settings in settings.php and set Stark 8.0-dev as default theme. Patch is again from comment #27.

Should the bullet "2. Find the comment in the issue where the patch applied cleanly ...", from Re-rolling patches page, be updated given your comment "... we always reroll the latest patch ... "- #49, above?

Sending for review, let's see what testbot thinks. Thanks!

I've expanded on and adjusted http://drupal.org/patch/reroll, I think that's a good addition because I've seen it crop up a time or two before. Take a look and let me know if that makes more sense :)

@Cottser: your addition to http://drupal.org/patch/reroll looks good to me ;) And, the patch has passed - yes!

@bdgreen - much better! I think the only discrepancy now with the before-reroll patch (I diffed the two patches) is that the lines building '$output' should be removed:

+++ b/core/modules/file/file.field.incundefined
@@ -638,18 +639,18 @@ function file_field_widget_submit($form, &$form_state) {
   $output = '';
...
   // The "form-managed-file" class is required for proper Ajax functionality.
   $output .= '<div class="file-widget form-managed-file clearfix">';

These lines are not needed anymore, since this is reproduced further down in the function.

Not sure I understand.

Do you mean ...

function theme_file_widget($variables) {
  $element = $variables['element'];
  $output = '';

  // The "form-managed-file" class is required for proper Ajax functionality.
  $output .= '<div class="file-widget form-managed-file clearfix">';

Should read ...

function theme_file_widget($variables) {
  $element = $variables['element'];
  // The "form-managed-file" class is required for proper Ajax functionality.
  $output  = '<div class="file-widget form-managed-file clearfix">';

?

@bdgreen - Sorry for any confusion. Compare these two diffs:

Patch from #27:

+++ b/core/modules/file/file.field.incundefined
@@ -580,40 +581,36 @@ function file_field_widget_submit($form, &$form_state) {
-function theme_file_widget($variables) {
+function template_preprocess_file_widget(&$variables) {
   $element = $variables['element'];
-  $output = '';
-
-  // The "form-managed-file" class is required for proper Ajax functionality.
-  $output .= '<div class="file-widget form-managed-file clearfix">';
   if ($element['fid']['#value'] != 0) {
     // Add the file size after the file name.
-    $element['filename']['#markup'] .= ' <span class="file-size">(' . format_size($element['#file']->filesize) . ')</span> ';
+    $element['filename']['#markup'] .= ' <span ' . new Attribute(array('class' => array('file-size'))) . '>(' . format_size($element['#file']->filesize) . ')</span> ';
   }
-  $output .= drupal_render_children($element);
-  $output .= '</div>';
-
-  return $output;
+  $variables['element'] = drupal_render_children($element);
+  // The "form-managed-file" class is required for proper Ajax functionality.
+  $variables['attributes'] = new Attribute(array('class' => array('file-widget', 'form-managed-file', 'clearfix')));
 }

Patch from #53:

+++ b/core/modules/file/file.field.incundefined
@@ -638,18 +639,18 @@ function file_field_widget_submit($form, &$form_state) {
-function theme_file_widget($variables) {
+function template_preprocess_file_widget(&$variables) {
   $element = $variables['element'];
   $output = '';
-
   // The "form-managed-file" class is required for proper Ajax functionality.
   $output .= '<div class="file-widget form-managed-file clearfix">';
   if (!empty($element['fids']['#value'])) {
@@ -657,22 +658,21 @@ function theme_file_widget($variables) {

@@ -657,22 +658,21 @@ function theme_file_widget($variables) {
     $file = reset($element['#files']);
     $element['file_' . $file->fid]['filename']['#markup'] .= ' <span class="file-size">(' . format_size($file->filesize) . ')</span> ';
   }
-  $output .= drupal_render_children($element);
-  $output .= '</div>';
-
-  return $output;
+  $variables['element'] = drupal_render_children($element);
+  // The "form-managed-file" class is required for proper Ajax functionality.
+  $variables['attributes'] = new Attribute(array('class' => array('file-widget', 'form-managed-file', 'clearfix')));
 }

We don't need to build the $output variable any more - we are converting from a theme function (which returns a string of HTML) to a preprocess function (which returns an array of variables). So the $element lines are fine as is, but any lines building $output should be deleted. Specifically, the three lines I highlighted in my previous comment.

If you can provide an interdiff then we can double check the changes :)

Hope that make a bit more sense. BTW I'm using Dreditor to review these patches and grab the patch snippets for this comment :)

(I noticed we lost the Attribute object in the reroll but I think that's a good thing. I see no point in creating an Attribute object and inserting it into a string immediately.)

@Cottser: made changes as requested, but now "file size" is not now being displayed with attachment ...

Returned to: the "( {file size} )" is lost on applying patch #53

Before patch #53

After patch #53

Latest interfdiff.txt (for #59) & patchs #53 and #59 re-submitted.

Clearing cache ensures display of the file size for patch #59 (and, #53) :$

Awesome, thanks! :)

#9: 1898070-9.patch queued for re-testing.

Template preprocess functions were not running previously. I'm including file.field.inc in the hook_theme definitions to include them.

Attaching screenshots of the file upload field showing the difference between patches from 59 and 65.

profiling this now

=== 8.x..8.x compared (519fe63632755..519fe7b00df53):

ct : 40,580|40,580|0|0.0%
wt : 377,694|378,672|978|0.3%
cpu : 335,787|335,713|-74|-0.0%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%

Profiler output

=== 8.x..1898070 compared (519fe63632755..519fe7e9f08c3):

ct : 40,580|40,580|0|0.0%
wt : 377,694|379,540|1,846|0.5%
cpu : 335,787|336,211|424|0.1%
mu : 50,206,616|50,206,496|-120|-0.0%
pmu : 50,271,728|50,272,040|312|0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self

profiling not run correctly. Needs to be redone

#65: core-update_file_to_twig-1898070-65.patch queued for re-testing.

Updated issue summary.

#65: core-update_file_to_twig-1898070-65.patch queued for re-testing.

Tagging for reroll.

Working on the reroll

Related #2030243: Remove theme_file_upload_help() from the theme system

Unassigning since it's been a while, @rvilar you can reassign if you're still working on this.

Rerolled.

#79: core-update_file_to_twig-1898070-79.patch queued for re-testing.

At the Drupal Camp Twin Cities code sprint - going to re-reroll #65.

Re-rolled #65.

I was seeing an issue where a check_plain() call was getting an object as an argument. Fixed by tweaking #84 to use a File object and its property getters in template_preprocess_file_link().

Unassigning.

And again.

Here is a re-roll of the patch from #85. My Git foo was not good enough to get an interdiff on this one.

removing tag.

Thanks @tlattimore! Interdiffs are not needed for rerolls of patches that don't apply. The only thing I recommend doing is that if you want to make any changes after rerolling, first post the reroll with no changes, then the revised patch with interdiff :)

Retagging for manual testing because it's been a while.

Eh, I messed up the re-roll on #91 and created a syntax error. Here is a patch with that resolved. See interdiff.

Here's a few thing to help cleanup and speed up.

+++ b/core/modules/file/file.field.inc
@@ -7,6 +7,7 @@
+use Drupal\Core\Template\Attribute;

@@ -586,41 +587,37 @@ function file_field_widget_submit($form, &$form_state) {
+  $variables['attributes'] = new Attribute(array('class' => array('file-widget', 'form-managed-file', 'clearfix')));

+++ b/core/modules/file/file.module
@@ -1506,12 +1514,8 @@ function theme_file_managed_file($variables) {
+  $variables['attributes'] = new Attribute($attributes);

@@ -1551,36 +1555,46 @@ function file_managed_file_pre_render($element) {
+    '#attributes' => new Attribute(array('class' => 'file-icon')),

@@ -1589,28 +1603,8 @@ function theme_file_link($variables) {
+  $variables['attributes'] = new Attribute(array('class' => array('file')));

There shouldn't be a need to instantiate the Attribute Class for $variable['attributes'] because it's done for us lazily @see #1982024: Lazy-load Attribute objects later in the rendering process only if needed

+++ b/core/modules/file/file.module
@@ -1506,12 +1514,8 @@ function theme_file_managed_file($variables) {
+  $variables['element'] = drupal_render_children($element);

I could be wrong but I don't believe drupal_render_children() needs to be called.

+++ b/core/modules/file/file.field.inc
@@ -710,34 +708,30 @@ function theme_file_widget_multiple($variables) {
+  $variables['element'] = drupal_render_children($element);

ditto

+++ b/core/modules/file/templates/file-formatter-table.html.twig
@@ -0,0 +1,15 @@
+ * @see template_preprocess()

+++ b/core/modules/file/templates/file-link.html.twig
@@ -0,0 +1,18 @@
+ * @see template_preprocess()

+++ b/core/modules/file/templates/file-managed-file.html.twig
@@ -0,0 +1,18 @@
+ * @see template_preprocess()

+++ b/core/modules/file/templates/file-upload-help.html.twig
@@ -0,0 +1,17 @@
+ * @see template_preprocess()

+++ b/core/modules/file/templates/file-widget-multiple.html.twig
@@ -0,0 +1,17 @@
+ * @see template_preprocess()

+++ b/core/modules/file/templates/file-widget.html.twig
@@ -0,0 +1,18 @@
+ * @see template_preprocess()

All the @see template_preprocess() can be removed.

Here is a patch with the changes that joelpittet requested in #96.

I've been trying to test this and seem to encounter an unrelated issue. After adding a file field to a content type, the upload form element does not appear in the node form. I see this in the current 8.x, in the last commit before the patch, and after applying the patch, both GUI and drush installations. Perhaps I'm repeating an unrelated mistake, but I fail to test.

@oshelach I'd suggest maybe talking through your setup on IRC with drupal office hours on the #drupal channel. There is one in 5 hours: http://drupalmentoring.org/ http://countingdownto.com/countdown/186646

I had no problem adding a field and seeing the form element on the latest d8 commit 6cfcb25b7809322e07b594c117bf7e5ad7e9dff4 with the patch applied.

Some code nitpicks:

@@ -7,6 +7,7 @@
+use Drupal\Core\Template\Attribute;

Not needed for this file.

@@ -586,41 +587,37 @@ function file_field_widget_submit($form, &$form_state) {
+  $variables['element'] = drupal_render_children($element);

drupal_render_children shouldn't be needed either.

@@ -1490,15 +1497,16 @@ function file_managed_file_save_upload($element) {
+ *   - element_attributes: An Array of HTML attributes.

This doesn't exist.

@@ -1510,12 +1518,8 @@ function theme_file_managed_file($variables) {
+  $variables['element'] = drupal_render($element);

Don't think this drupal_render needs to be there. We'll try to avoid rendering things before the template for drillablility.

@@ -0,0 +1,17 @@
+<span class="{{ attributes.class }}"{{ attributes }}>{{ icon }} {{ link }}</span>

Remove the separated class and let attributes render it if it exists, hopefully avoiding an empty class attribute in markup.

@@ -0,0 +1,16 @@
+{% for description in descriptions %}
+  {{ description|t }}<br />
+{% endfor %}

Would like to see this as a join instead of a loop: http://twig.sensiolabs.org/doc/filters/join.html

I'm not sure what "Remove the separated class and let attributes render it if it exists, hopefully avoiding an empty class attribute in markup" means, but I've added that as a todo. Implemented other suggestions.

Thanks for the patch @oshelach

Here's what I meant there.
<span class="{{ attributes.class }}"{{ attributes }}>{{ icon }} {{ link }}</span>
becomes

<span{{ attributes }}>{{ icon }} {{ link }}</span>

And that will avoid any empty 'class' attributes from being rendered, therefore generally cleaner markup. The reason for the other way with it separated out is to allow for easier adding of additional css classes in the twig markup but we decided to do that kind of stuff as example in the bartik theme.

I'm pretty sure the removal of drupal_render_children will work, and you may know this already (as I know I always forget) but if you want testbot to test out the patch, just set the status of the issue to 'Needs Review'.

Cheers!

Thank you, done.
Mind, this leaves several drupal_render() calls in file.field.inc and in file.module. Should they not all be removed?

#103 - the drupal_render() removal lives somewhere else #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(), this is just Twig.

wrong, link! sorry #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead.

+++ b/core/modules/file/file.field.inc
@@ -710,34 +706,30 @@ function theme_file_widget_multiple($variables) {
+  $variables['element'] = drupal_render($element);

+++ b/core/modules/file/file.module
@@ -1521,12 +1528,8 @@ function theme_file_managed_file($variables) {
+  $variables['element'] = drupal_render($element);

However this patch would be adding these two new ones which seems unnecessary, these should probably be $variables['element'] = $element; (needs manual testing to confirm).

ah, yeah, my bad - don't want to introduce new ones. Is that in the preprocess? I thought we were not doing drupal_render() in preprocess functions any more?

So, in fact, the remaining drupal_render() and render() do need to be removed?

no need to touch anything that's already in the file module, but don't add new render() or drupal_render() calls in the patch.

Keep in mind, the Twig template should internally be rendering anything like {{ foo }} so try what Cottser proposed and manually test it to be sure :)

Manual test results seem fine to me.

+++ b/core/modules/file/file.field.inc
@@ -1521,12 +1528,8 @@ function theme_file_managed_file($variables) {
+  $variables['element'] = $element;

Seems unnecessary as $element is not manipulated in the function and manual test seems fine without it.

re #111I agree, if it's not being modified it doesn't need to be reset back like that, the line can just be removed.
+ $variables['element'] = $element;

The only issue we run into sometimes is duplicate rendering of elements sometimes. We just unset() them as they get repurposed from the main $element as a workaround. That's just a head-up if you see duplicate elements in the markup.

Thanks @oshelach, @joelpittet, @thedavidmeister for keeping this one moving :) gave this a review from a code/documentation perspective, notes below.

1. +++ b/core/modules/file/file.field.inc
   @@ -586,41 +586,37 @@ function file_field_widget_submit($form, &$form_state) {
   + *   - element_attributes: An Array of HTML attributes.
   

   This line can be removed, this is not defined in file_theme() because the theme hook uses a 'render element'.

2. +++ b/core/modules/file/file.field.inc
   @@ -710,34 +707,30 @@ function theme_file_widget_multiple($variables) {
      $output = empty($rows) ? '' : drupal_render($build);
   -  $output .= drupal_render_children($element);
   -  return $output;
   +  $variables['element'] = $element;
   

   We missed one $output/$build line here, needs to be removed.

3. +++ b/core/modules/file/file.field.inc
   @@ -710,34 +707,30 @@ function theme_file_widget_multiple($variables) {
   + * Prepares variables for help text templates.
   + *
   + * Default template: file-upload-help.html.twig
   

   Needs a period at the end of .html.twig per http://drupal.org/node/1354#drupal (needs to be a complete sentence).

4. +++ b/core/modules/file/templates/file-link.html.twig
   @@ -0,0 +1,17 @@
   + * Available variables:
   + * - attributes: Remaining HTML attributes of an element.
   + *   - attributes.class: HTML classes.
   

   attributes.class should just be 'class', indenting in a list like this means add a dot/drill down.

5. +++ b/core/modules/file/templates/file-link.html.twig
   @@ -0,0 +1,17 @@
   + * - icon: An icon as produced by file-icon.html.twig.
   

   file-icon.html.twig is no more, this comment needs to be updated.

6. +++ b/core/modules/file/templates/file-upload-help.html.twig
   @@ -0,0 +1,14 @@
   +{{ descriptions|t|join('<br />') }}
   

   Why are we trying to put this through the t() filter? The content has already been put through t() in preprocess and we shouldn't be putting variables through t() anyway.

Follows #114, except for item 5 where I understand the need but lost my way between the documentation guidelines and the actual variable prepared by the preprocess funtion, how deep and detailed does this need to be?

Updated issue summary.

Thanks @oshelach, the changes look great!

For the 'icon' variable documentation, what about something simple like this? We just don't want to refer to a non-existent Twig template.

* - icon: An icon representing the file.

Cool, that's already in the patch.

Um.... removed but not replaced with an explanation of the new variables structure. Coming up.

Combed through this another time and went through the patch locally, here are some more things that can be fixed up. Thanks for working on this one @oshelach :)

1. +++ b/core/modules/file/file.field.inc
   @@ -702,6 +697,7 @@ function theme_file_widget_multiple($variables) {
   +
   
   @@ -710,34 +706,29 @@ function theme_file_widget_multiple($variables) {
   -    '#attributes' => array(
   -      'id' => $table_id,
   -    ),
   +    '#attributes' => array('id' => $table_id),
   

   Please revert these changes, they are not needed…

2. +++ b/core/modules/file/file.module
   @@ -1557,36 +1559,46 @@ function file_managed_file_pre_render($element) {
      $file_icon = array(
   
   @@ -1595,28 +1607,8 @@ function theme_file_link($variables) {
        '#icon_directory' => $variables['icon_directory'],
      );
   

   These $file_icon related lines need to be removed, they are creating an unused variable and use '#theme' => 'file_icon' which is being removed.

3. Related to the above, there is now an instance of '#theme' => 'file_icon' in core/modules/file/lib/Drupal/file/Plugin/views/field/FileMime.php that needs to be converted to image__file_icon.

4. +++ b/core/modules/file/templates/file-link.html.twig
   @@ -0,0 +1,16 @@
   + * - attributes: Remaining HTML attributes of an element.
   + *   - class: HTML classes.
   

   Let's remove the 'class' line here and use the documentation "HTML attributes for the containing element." as in the other templates here and many that are already in core.

5. +++ b/core/modules/file/templates/file-link.html.twig
   @@ -0,0 +1,16 @@
   +<span {{ attributes }}>{{ icon }} {{ link }}</span>
   

   This span needs to be updated to be <span{{ attributes }}> - no space before the attributes. The Attribute object automatically inserts the space if needed, see https://drupal.org/node/1727592.

Hum, looks like this needs another reroll! Thanks for working on this @oshelach :)

Rerolling, excpet the $icon_directory lines (2nd part of 2nd item in #119), which is used a few lines later in the creation of $variables['icon']. Should it really be eliminated?

You're right, $icon_directory is used! Thanks :)

1. +++ b/core/modules/file/file.module
   @@ -566,29 +566,36 @@ function file_get_content_headers(File $file) {
        'file_formatter_table' => array(
          'variables' => array('items' => NULL),
   +      'template' => 'file-formatter-table',
   +      'file' => 'file.field.inc',
        ),
   

   Could we remove file-formatter-table and replace it with a suggestion for #theme=>table? The template is only returning a table in {{ content }}

2. +++ b/core/modules/file/templates/file-link.html.twig
   @@ -0,0 +1,15 @@
   +<span {{attributes}}>{{ icon }} {{ link }}</span>
   

   Needs spaces inbetween the {{}} but not before on attributes.

I can't get the patch to apply. I have tried rolling back to Sept 22 commits with no luck.

It seems like the last patch is erroneously trying to add files that already exist like:

--- /dev/null
+++ b/core/modules/file/templates/file-formatter-table.html.twig
@@ -0,0 +1,14 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a file attachments table.
+ *
+ * Available variables:
+ * - content: File form element HTML.
+ *
+ * @see template_preprocess_file_formatter_table()
+ *
+ * @ingroup themeable
+ */
+#}

Hmm, don't think that's possible:

ls core/modules/file/templates
ls: core/modules/file/templates: No such file or directory

Maybe you have made some commits on 8.x @drupalninja99?

The patch does need a reroll though, looks like one or two hunks have conflicts.

It looks like another ticket has committed changes that are similar to what I am seeing in the last patch. There's seems to be overlap which renders the latest patch irrelevant or at least partially so. I am not sure how to proceed on this.

Here's the re-roll from #123

As I mentioned in #125

Replace file-formatter-table.html.twig template with table suggestion and fixed docs and formatting on file-link

Update testing steps

131: 1898070-131-twig-theme-file.patch queued for re-testing.

Applied the patch comment #131, but its not working locally for me.

@chanderbhushan - can you please provide more information? Did you clear the cache after applying the patch?

131: 1898070-131-twig-theme-file.patch queued for re-testing.

@Cottser I have applied your patch but its not successfully applied.

@chanderbhushan What's the error message you get when you try to apply the patch?

1. Make sure that drupal 8.x head is up-to-date via git pull
2. Check to make sure there are no changes locally with git status and git diff origin/8.x
3. Make sure you apply the patch from within your drupal root.
4. Then curl https://drupal.org/files/1898070-131-twig-theme-file.patch | git apply should work.
5. Check git status after to see it has changes.

@joelpittet it is working fine now thanks

@chanderbhushan great, are you working on manual testing?

@joelpittet Yes, i am doing manual testing.

131: 1898070-131-twig-theme-file.patch queued for re-testing.

Someone did an improper #type=>table conversion with #rows... argh. That is the prompt for the re-roll.

143: 1898070-143-twig-theme-file.patch queued for re-testing.

It no longer seems to apply, and its conflicts are beyond a simple re-roll.

Simple reroll of patch in #143.

Tagging for reroll. Thanks for the last one @idflood! After reroll we could use some manual testing and profiling on this one :)

Quick reroll of #146.

Oops, an extra space crept in. Here it is again.

Is there a way to cancel the test in #149?

Tagging for reroll.

reroll made

Works fine, also tested here #2265127: Add back file theming from 7.x-3.x

Let's get some evidence of manual testing and it'd be nice to a some profiling too as per #148

Tagging for sprints

I am working on profiling.

Easy re-roll: files have moved from lib/Drupal/file/Plugin/ to src/Plugin/.

I profiled all the elements for this issue. The only one that gets close to a 1% difference is file-widget-multiple.html. Here are the gory details:

Profiling file-widget.html.twig and file-upload-help.html.twig

=== 8.x..8.x compared (5391fc871e91d..5391fcf6ab927):

ct  : 80,383|80,383|0|0.0%
wt  : 539,783|538,139|-1,644|-0.3%
cpu : 486,835|485,336|-1,499|-0.3%
mu  : 13,042,796|13,042,796|0|0.0%
pmu : 13,141,928|13,141,928|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5391fc871e91d&...

=== 8.x..1898070-160-twig-theme-file compared (5391fc871e91d..5391fd5a43e70):

ct  : 80,383|80,495|112|0.1%
wt  : 539,783|538,114|-1,669|-0.3%
cpu : 486,835|487,235|400|0.1%
mu  : 13,042,796|13,067,740|24,944|0.2%
pmu : 13,141,928|13,166,960|25,032|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5391fc871e91d&...

file-link.html.twig

=== 8.x..8.x compared (539201f8938f4..539203c57074d):

ct  : 71,681|71,681|0|0.0%
wt  : 478,633|478,880|247|0.1%
cpu : 433,641|433,152|-489|-0.1%
mu  : 12,830,060|12,830,060|0|0.0%
pmu : 12,945,524|12,945,524|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=539201f8938f4&...

=== 8.x..1898070-160-twig-theme-file compared (539201f8938f4..5392041970f3f):

ct  : 71,681|71,912|231|0.3%
wt  : 478,633|480,098|1,465|0.3%
cpu : 433,641|436,329|2,688|0.6%
mu  : 12,830,060|12,888,300|58,240|0.5%
pmu : 12,945,524|13,004,932|59,408|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=539201f8938f4&...

Profiling file-widget-multiple.html.twig

=== 8.x..8.x compared (539205a48c548..539206513e72e):

ct  : 89,936|89,936|0|0.0%
wt  : 594,227|595,138|911|0.2%
cpu : 540,163|539,815|-348|-0.1%
mu  : 13,474,668|13,474,668|0|0.0%
pmu : 13,579,136|13,579,136|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=539205a48c548&...

=== 8.x..1898070-160-twig-theme-file compared (539205a48c548..539206aca7ae8):

ct  : 89,936|90,753|817|0.9%
wt  : 594,227|598,833|4,606|0.8%
cpu : 540,163|543,931|3,768|0.7%
mu  : 13,474,668|13,568,852|94,184|0.7%
pmu : 13,579,136|13,675,896|96,760|0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=539205a48c548&...

Profiling file-managed-file.html.twig

=== 8.x..8.x compared (53920b950c201..53920bd97d62d):

ct  : 54,670|54,670|0|0.0%
wt  : 385,188|384,732|-456|-0.1%
cpu : 343,319|343,758|439|0.1%
mu  : 11,209,656|11,209,656|0|0.0%
pmu : 11,303,172|11,303,172|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53920b950c201&...

Run 53920b950c201 uploaded successfully for drupal-perf-drupalcon.
Run 53920c043e98b uploaded successfully for drupal-perf-drupalcon.

=== 8.x..1898070-160-twig-theme-file compared (53920b950c201..53920c043e98b):

ct  : 54,670|54,713|43|0.1%
wt  : 385,188|385,546|358|0.1%
cpu : 343,319|343,132|-187|-0.1%
mu  : 11,209,656|11,220,072|10,416|0.1%
pmu : 11,303,172|11,313,564|10,392|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53920b950c201&...

Tests test

Confirm that this patch does not fly:

Uncaught PHP Exception Twig_Error_Loader: "Unable to find template "core/modules/file/templates/file-upload-help.html.twig" (looked into: /Users/steinmb/apache/htdocs/d8/drupal)." at /Users/steinmb/apache/htdocs/d8/drupal/core/vendor/twig/twig/lib/Twig/Loader/Filesystem.php line 202

@steinmb: Thanks for catching that. It looks like I messed up when creating the patch. Let me try again.

Manual testing. Followed steps found in issue summary.
File uploaded: v1.2.text.txt renamed to v1.2.text_.txt

file-widget.html.twig

<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->
<div class="file-widget form-managed-file clearfix">
  <div id="edit-field-foo-0-upload-ajax-wrapper"><input type="file" class="form-file fileValidate-processed auto-file-upload-processed" size="22" name="files[field_foo_0]" id="edit-field-foo-0-upload"><input type="submit" value="Upload" name="field_foo_0_upload_button" id="edit-field-foo-0-upload-button" formnovalidate="formnovalidate" class="js-hide button form-submit drupal-ajax-processed"><input type="hidden" value="" name="field_foo[0][fids]"><input type="hidden" value="1" name="field_foo[0][display]"></div>
</div>

<!-- END OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->

file-link.html.twig:

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_link') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-link.html.twig' -->
<span class="file">

<!-- THEME DEBUG -->
<!-- CALL: _theme('image__file_icon') -->
<!-- FILE NAME SUGGESTIONS:
   * image--file-icon.html.twig
   x image.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/image.html.twig' -->
<img typeof="foaf:Image" title="v1.2.text_.txt" alt="" src="/core/modules/file/icons/text-plain.png" class="file-icon">

<!-- END OUTPUT from 'core/modules/system/templates/image.html.twig' -->

 <a type="text/plain; length=988" href="http://d8.dev/sites/default/files/v1.2.text_.txt">v1.2.text_.txt</a></span>

<!-- END OUTPUT from 'core/modules/file/templates/file-link.html.twig' -->

file-upload-help.html.twig

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_upload_help') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-upload-help.html.twig' -->
One file only.<br>1.95 GB limit.<br>Allowed types: txt.

<!-- END OUTPUT from 'core/modules/file/templates/file-upload-help.html.twig' -->

file-widget-multiple.html.twig

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_widget_multiple') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-widget-multiple.html.twig' -->


<!-- THEME DEBUG -->
<!-- CALL: _theme('table') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/table.html.twig' -->
<div class="tabledrag-toggle-weight-wrapper"><button class="link tabledrag-toggle-weight" type="button" title="Re-order rows by numerical weight instead of dragging.">Show row weights</button></div><div class="tableresponsive-toggle-columns"><button class="link tableresponsive-toggle" type="button" title="Show table cells that were hidden to make the table fit within a small screen." style="display: none;">Hide unimportant columns</button></div><table class="responsive-enabled tabledrag-processed tableresponsive-processed" id="edit-field-foo-table">
  
  
      <thead>
      <tr>
                  <th>File information</th>
                  <th class="tabledrag-hide" style="display: none;">Weight</th>
                  <th>Operations</th>
              </tr>
    </thead>
  
      <tbody>
              <tr class="draggable odd">
                      <td><a class="tabledrag-handle" href="#" title="Drag to re-order"><div class="handle">&nbsp;</div></a>

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_widget') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->
<div class="file-widget form-managed-file clearfix">
  <input type="hidden" value="2" name="field_foo[0][fids]">

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_link') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-link.html.twig' -->
<span class="file">

<!-- THEME DEBUG -->
<!-- CALL: _theme('image__file_icon') -->
<!-- FILE NAME SUGGESTIONS:
   * image--file-icon.html.twig
   x image.html.twig
-->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/image.html.twig' -->
<img typeof="foaf:Image" title="v1.2.text_.txt" alt="" src="/core/modules/file/icons/text-plain.png" class="file-icon">

<!-- END OUTPUT from 'core/modules/system/templates/image.html.twig' -->

 <a type="text/plain; length=988" href="http://d8.dev/sites/default/files/v1.2.text_.txt" class="menu-item__link">v1.2.text_.txt</a></span>

<!-- END OUTPUT from 'core/modules/file/templates/file-link.html.twig' -->

 <span class="file-size">(988 bytes)</span> <input type="hidden" value="1" name="field_foo[0][display]">
</div>

<!-- END OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->

</td>
                      <td class="tabledrag-hide" style="display: none;">

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->
<div class="form-item form-type-select form-item-field-foo-0--weight">
      <label for="edit-field-foo-0-weight" class="visually-hidden">Weight for new file</label>
      

<!-- THEME DEBUG -->
<!-- CALL: _theme('select') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/select.html.twig' -->
<select name="field_foo[0][_weight]" id="edit-field-foo-0-weight" class="edit-field-foo-weight form-select"><option value="-2">-2</option><option value="-1">-1</option><option selected="selected" value="0">0</option><option value="1">1</option><option value="2">2</option></select>

<!-- END OUTPUT from 'core/modules/system/templates/select.html.twig' -->


      </div>

<!-- END OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->

</td>
                      <td><input type="submit" class="button form-submit drupal-ajax-processed" value="Remove" name="field_foo_0_remove_button" id="edit-field-foo-0-remove-button" formnovalidate="formnovalidate"></td>
                  </tr>
          </tbody>
  </table>

<!-- END OUTPUT from 'core/modules/system/templates/table.html.twig' -->


<div id="edit-field-foo-ajax-wrapper">

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->
<div class="form-item form-type-managed-file form-item-field-foo-1">
      <label for="edit-field-foo-1-upload">Add a new file</label>
      

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_widget') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->
<div class="file-widget form-managed-file clearfix">
  <input type="file" class="form-file fileValidate-processed auto-file-upload-processed" size="22" name="files[field_foo_1][]" id="edit-field-foo-1-upload" multiple="multiple"><input type="submit" value="Upload" name="field_foo_1_upload_button" id="edit-field-foo-1-upload-button" formnovalidate="formnovalidate" class="js-hide button form-submit drupal-ajax-processed"><input type="hidden" value="1" name="field_foo[1][_weight]"><input type="hidden" value="" name="field_foo[1][fids]"><input type="hidden" value="1" name="field_foo[1][display]">
</div>

<!-- END OUTPUT from 'core/modules/file/templates/file-widget.html.twig' -->


          <div id="edit-field-foo-1-upload--description" class="description">
      

<!-- THEME DEBUG -->
<!-- CALL: _theme('file_upload_help') -->
<!-- BEGIN OUTPUT from 'core/modules/file/templates/file-upload-help.html.twig' -->
Unlimited number of files can be uploaded to this field.<br>1.95 GB limit.<br>Allowed types: txt.

<!-- END OUTPUT from 'core/modules/file/templates/file-upload-help.html.twig' -->


    </div>
  </div>

<!-- END OUTPUT from 'core/modules/system/templates/form-element.html.twig' -->

</div>

<!-- END OUTPUT from 'core/modules/file/templates/file-widget-multiple.html.twig' -->

This failed:

Running the file_module_test —module.

Uncaught PHP Exception InvalidArgumentException: "Class "\Drupal\file_module_test\Form\FileModuleTestForm" does not exist." at /Users/steinmb/apache/htdocs/d8/drupal/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php line 29

Prob. not related but. Uploading the file v1.2.text.txt it got renamed to v1.2.text_.txt and the UI created two messages

I'll take that back, the exception found in #167 was my fault. I'm happy, the issue happy.

Awesome work @benjifisher and @steinmb!

Some very nitpicky points but I think this could use just a tiny bit more love before it passes the finish line.

1. +++ b/core/modules/file/file.field.inc
   @@ -126,18 +121,19 @@ function theme_file_widget_multiple($variables) {
   +
   ...
   -      'id' => $table_id,
   +      'id' => $table_id
   

   I think these two hunks should be reverted, the first one is just adding a newline out of taste from what I can see and the second one is removing a coding-standards appropriate comma: https://drupal.org/coding-standards#array

2. +++ b/core/modules/file/file.field.inc
   @@ -148,25 +144,22 @@ function theme_file_widget_multiple($variables) {
   + * Prepares variables for help text templates.
   + *
   + * Default template: file-upload-help.html.twig.
   

   "file upload help text templates" maybe would be better here?

3. +++ b/core/modules/file/file.module
   @@ -1537,15 +1539,15 @@ function file_managed_file_save_upload($element, array &$form_state) {
      $attributes = array();
   
   @@ -1557,12 +1559,7 @@ function theme_file_managed_file($variables) {
      $attributes['class'][] = 'form-managed-file';
   ...
   +  $variables['attributes'] = $attributes;
   

   I think it would be better if we just added things to $variables['attributes'], this looks like we're currently clobbering everything coming in. I think that might be in scope to be added here but as long as that gets taken care of at some point I'm happy :) [in other words, if someone can create a followup that would be great <3]

4. +++ b/core/modules/file/file.module
   @@ -1601,24 +1598,36 @@ function file_managed_file_pre_render($element) {
   - *     files. Defaults to the value of the "icon.directory"
   - *     variable.
   + *     files. Defaults to the value of the "icon.directory" variable.
   

   Out of scope for this issue so I would lean towards leaving these lines alone…

5. +++ b/core/modules/file/file.module
   @@ -1601,24 +1598,36 @@ function file_managed_file_pre_render($element) {
     *
     * @ingroup themeable
     */
   -function theme_file_link($variables) {
   +function template_preprocess_file_link(&$variables) {
   

   Remove @ingroup themeable from this because it's a preprocess function.

Thanks @Cottser. I did 1,2,3, and 5 of the nitpicks in #171.

I am ok with #4 the minor docblock change because we are already cleaning up the docblock title by removing @ingroup themable so any conflict happens it would likely already happen due to the other changes surrounding that.

typo, back to rtbc.

Works for me, no interdiff though? :P

Oh sorry, I forgot, here's that interdiff between #166 and #174

I'm really sorry, but this no longer applies due to #2089331: [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain(). Should be a quick re-roll, I hope.

Re-rolled.

Great work, all!

Committed and pushed to 8.x. Thanks!