Issue #1898070 by artofeclipse, vlad.dancer, steveoliver, EVIIILJ, joelpittet, elv, c4rl, jenlampton, duellj, Cottser, OpenChimp: Convert file module to Twig.

Task

Use Twig instead of PHPTemplate

Remaining

  • Manual testing

Theme function name/template path Conversion status Needs Markup Comparison Needs Profiling
theme_file_formatter_table converted n/a
theme_file_icon removed! now uses #theme => 'image__file_icon' n/a
theme_file_link converted #161
theme_file_managed_file converted #161
theme_file_upload_help converted #161
theme_file_widget converted #161
theme_file_widget_multiple converted, see also #type table conversion issue: #1938902: Convert file theme tables to table #type #161

To test this code...

1) Add a file field to a content type
2) Create a piece of content, and check that the upload form element is using the file-widget.html.twig
3) attach a file and save
4a) view the node to see if the icon was rendered using file-icon.html.twig (no longer applicable)
4b) view the node to see if the link to the file was rendered using file-link.html.twig
5) edit the node to see if the upload help is rendered using file-upload-help.html.twig
6) edit your content type and make your file field multi-value.
7) edit your node again and confirm that the widget is rendered using file-widget-multiple.html.twig
8) enable the file_module_test module to test managed files.
9) Go to /file/test to confirm that the upload field is rendered using file-managed-file.html.twig

#1757550: [Meta] Convert core theme functions to Twig templates
#1938902: Convert file theme tables to table #type

Files: 
CommentFileSizeAuthor
#178 1898070-twig-theme-file-178.patch15.11 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,359 pass(es).
[ View ]
#176 interdiff.txt2.36 KBjoelpittet
#174 1898070-twig-theme-file-174.patch15.09 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,293 pass(es).
[ View ]
#172 1898070-twig-theme-file-172.patch15.09 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/file.module.
[ View ]
#168 Upload_text_file_rename.png21.84 KBsteinmb
#166 1898070-166-twig-theme-file.patch15.13 KBbenjifisher
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,766 pass(es).
[ View ]
#160 1898070-160-twig-theme-file.patch12.16 KBbenjifisher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,438 pass(es), 1,125 fail(s), and 172 exception(s).
[ View ]
#153 1898070-153-twig-theme-file.patch15.23 KBjjcarrion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,800 pass(es).
[ View ]
#150 1898070-150-twig-theme-file.patch15.32 KBLinL
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,424 pass(es).
[ View ]
#149 1898070-149-twig-theme-file.patch15.32 KBLinL
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,406 pass(es).
[ View ]
#146 1898070-146-twig-theme-file.patch15.32 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 63,164 pass(es).
[ View ]
#123 file_module-twig-1898070-123.patch14.95 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 58,717 pass(es).
[ View ]
#123 interdiff-1898070-120-123.txt1.82 KBoshelach
#120 file_module-twig-1898070-120.patch14.81 KBoshelach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_module-twig-1898070-120.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#120 interdiff-1898070-115-120.txt3.44 KBoshelach
#115 file_module-twig-1898070-115.patch14.29 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]
#115 interdiff-1898070-113-115.txt2.14 KBoshelach
#113 file_module-twig-1898070-113.patch14.29 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 58,239 pass(es).
[ View ]
#113 interdiff-1898070-111-113.txt402 bytesoshelach
#111 file_module-twig-1898070-111.patch14.32 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 58,273 pass(es).
[ View ]
#111 interdiff-1898070-103-111.txt1.34 KBoshelach
#103 file_module-twig-1898070-103.patch14.32 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es).
[ View ]
#103 interdiff-1898070-101-103.txt624 bytesoshelach
#101 file_module-twig-1898070-101.patch14.49 KBoshelach
PASSED: [[SimpleTest]]: [MySQL] 57,926 pass(es).
[ View ]
#101 interdiff-1898070-97-101.txt2.37 KBoshelach
#97 1898070-file-module-twig-97.patch14.68 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]
#97 interdiff.txt5.22 KBtlattimore
#95 1898070-file-to-twig-95.patch14.94 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]
#95 interdiff.txt659 bytestlattimore
#91 1898070-file_module_twig-91.patch14.94 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/file.field.inc.
[ View ]
#85 core-1898070-file_module_to_twig-85.patch14.99 KBLes Lim
PASSED: [[SimpleTest]]: [MySQL] 57,444 pass(es).
[ View ]
#85 interdiff-1898070-84-85.txt1.35 KBLes Lim
#84 core-1898070-file_module_to_twig-84.patch14.51 KBLes Lim
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 0 fail(s), and 439 exception(s).
[ View ]
#79 core-update_file_to_twig-1898070-79.patch15.08 KBsergesemashko
FAILED: [[SimpleTest]]: [MySQL] 57,007 pass(es), 0 fail(s), and 985 exception(s).
[ View ]
#66 1898070-patch-59-nodescription.png11.99 KBjesse.d
#66 1898070-patch-65-withdescription.png15.29 KBjesse.d
#65 core-update_file_to_twig-1898070-65.patch14.4 KBjesse.d
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-update_file_to_twig-1898070-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 interdiff.txt1.13 KBjesse.d
#60 core-update_file_to_twig-1898070-59.patch14.26 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 55,644 pass(es).
[ View ]
#60 core-update_file_to_twig-1898070-53.patch14.27 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 55,561 pass(es).
[ View ]
#60 interdiff.txt861 bytesbdgreen
#60 Before core-update_file_to_twig-1898070-53.patch.png11.09 KBbdgreen
#60 After core-update_file_to_twig-1898070-53.patch.png12.41 KBbdgreen
#59 core-update_file_to_twig-1898070-59.patch14.26 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 55,701 pass(es).
[ View ]
#59 interdiff.txt834 bytesbdgreen
#53 core-update_file_to_twig-1898070-53.patch14.27 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 55,408 pass(es).
[ View ]
#50 core-update_file_to_twig-1898070-50.patch14.03 KBbdgreen
FAILED: [[SimpleTest]]: [MySQL] 55,540 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#47 1898070-46.patch17.33 KBbdgreen
FAILED: [[SimpleTest]]: [MySQL] 55,327 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#44 file.module.patch5.02 KBbdgreen
FAILED: [[SimpleTest]]: [MySQL] 55,064 pass(es), 9 fail(s), and 2,474 exception(s).
[ View ]
#44 file.field_.inc_.patch5.53 KBbdgreen
FAILED: [[SimpleTest]]: [MySQL] 55,399 pass(es), 3 fail(s), and 536 exception(s).
[ View ]
#28 interdiff.txt9.08 KBCottser
#27 core-update_file_to_twig-1898070-27.patch14.82 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-update_file_to_twig-1898070-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 core-update_file_to_twig-1898070-26.patch14.8 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 50,898 pass(es), 90 fail(s), and 26 exception(s).
[ View ]
#25 core-update_file_to_twig-1898070-25.patch15.71 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 49,124 pass(es), 68 fail(s), and 95 exception(s).
[ View ]
#21 1898070-21.patch17.56 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 52,243 pass(es).
[ View ]
#19 core-update_file_to_use_twig-1898070-19.patch17.57 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 51,975 pass(es), 314 fail(s), and 115 exception(s).
[ View ]
#17 1898070-17.patch17.54 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 51,311 pass(es).
[ View ]
#17 interdiff.txt4.69 KBCottser
#16 1898070-16.patch17.48 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 51,314 pass(es).
[ View ]
#16 interdiff.txt369 bytesCottser
#13 1898070-13.patch17.31 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 51,042 pass(es), 333 fail(s), and 201 exception(s).
[ View ]
#13 interdiff.txt17.14 KBCottser
#9 1898070-9.patch23.96 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898070-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 interdiff.txt7.78 KBCottser
#3 1898070-3-twig-file.patch23.5 KBduellj
PASSED: [[SimpleTest]]: [MySQL] 50,435 pass(es).
[ View ]

Comments

c4rl’s picture

Issue tags:+Twig

Tagging

duellj’s picture

Assigned:Unassigned» duellj

Working on this

duellj’s picture

Assigned:duellj» Unassigned
Status:Active» Needs review
StatusFileSize
new23.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,435 pass(es).
[ View ]

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:

<?php
/**
 * 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
 */
?>
<?php
/**
 * Preprocess variables for a link to a file.
 */
?>

How verbose do we need to be?

Cottser’s picture

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.

duellj’s picture

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.

duellj’s picture

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.

Cottser’s picture

Assigned:Unassigned» Cottser

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 :)

Berdir’s picture

Cottser’s picture

StatusFileSize
new7.78 KB
new23.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898070-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#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.

Cottser’s picture

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

duellj’s picture

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.

Cottser’s picture

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.

Cottser’s picture

StatusFileSize
new17.14 KB
new17.31 KB
FAILED: [[SimpleTest]]: [MySQL] 51,042 pass(es), 333 fail(s), and 201 exception(s).
[ View ]

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!

Status:Needs review» Needs work

The last submitted patch, 1898070-13.patch, failed testing.

Cottser’s picture

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

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new369 bytes
new17.48 KB
PASSED: [[SimpleTest]]: [MySQL] 51,314 pass(es).
[ View ]

Forgot one important detail:

<?php
use Drupal\Core\Template\Attribute;
?>
Cottser’s picture

StatusFileSize
new4.69 KB
new17.54 KB
PASSED: [[SimpleTest]]: [MySQL] 51,311 pass(es).
[ View ]

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

Cottser’s picture

Status:Needs review» Needs work
Cottser’s picture

Issue summary:View changes

Add commit message to issue summary

jenlampton’s picture

Status:Needs work» Needs review
StatusFileSize
new17.57 KB
FAILED: [[SimpleTest]]: [MySQL] 51,975 pass(es), 314 fail(s), and 115 exception(s).
[ View ]

- 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?

jenlampton’s picture

Issue summary:View changes

Added how to test

Status:Needs review» Needs work

The last submitted patch, core-update_file_to_use_twig-1898070-19.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new17.56 KB
PASSED: [[SimpleTest]]: [MySQL] 52,243 pass(es).
[ View ]

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?

Cottser’s picture

Issue summary:View changes

more test

jenlampton’s picture

Status:Needs review» Reviewed & tested by the community

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.

Dave Reid’s picture

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

Dave Reid’s picture

Status:Reviewed & tested by the community» Needs work

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.

jenlampton’s picture

Status:Needs work» Needs review
StatusFileSize
new15.71 KB
FAILED: [[SimpleTest]]: [MySQL] 49,124 pass(es), 68 fail(s), and 95 exception(s).
[ View ]

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

jenlampton’s picture

StatusFileSize
new14.8 KB
FAILED: [[SimpleTest]]: [MySQL] 50,898 pass(es), 90 fail(s), and 26 exception(s).
[ View ]

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

jenlampton’s picture

StatusFileSize
new14.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-update_file_to_twig-1898070-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Cottser’s picture

StatusFileSize
new9.08 KB

Nice work, @jenlampton!

Interdiff between #21 and #27.

Status:Needs review» Needs work
Issue tags:-Twig

The last submitted patch, core-update_file_to_twig-1898070-27.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, core-update_file_to_twig-1898070-27.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
Issue tags:+Twig
koppie’s picture

Status:Needs review» Needs work

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.

Cottser’s picture

Assigned:Cottser» Unassigned

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

Cottser’s picture

@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 :)

Cottser’s picture

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

Cottser’s picture

Issue summary:View changes

added how to test managed files

Cottser’s picture

Issue summary:View changes

Add #type table conversion issue to related

Cottser’s picture

Issue summary:View changes

Add conversion summary table

Cottser’s picture

Issue tags:+Novice

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

Cottser’s picture

Status:Needs work» Needs review

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().

Cottser’s picture

Cottser’s picture

Issue tags:-Novice

Tags.

Cottser’s picture

Issue tags:-Twig

Status:Needs review» Needs work
Issue tags:+Twig

The last submitted patch, core-update_file_to_twig-1898070-27.patch, failed testing.

Cottser’s picture

Issue tags:+Novice, +Needs reroll

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

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

Cottser’s picture

Issue summary:View changes

Linkify #type table issue

Cottser’s picture

Issue summary:View changes

Remove sandbox link

bdgreen’s picture

Status:Needs work» Needs review
StatusFileSize
new5.53 KB
FAILED: [[SimpleTest]]: [MySQL] 55,399 pass(es), 3 fail(s), and 536 exception(s).
[ View ]
new5.02 KB
FAILED: [[SimpleTest]]: [MySQL] 55,064 pass(es), 9 fail(s), and 2,474 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, file.field_.inc_.patch, failed testing.

Cottser’s picture

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’s picture

StatusFileSize
new17.33 KB
FAILED: [[SimpleTest]]: [MySQL] 55,327 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
bdgreen’s picture

Status:Needs work» Needs review
Cottser’s picture

Status:Needs review» Needs work

@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 :)

bdgreen’s picture

Status:Needs work» Needs review
StatusFileSize
new14.03 KB
FAILED: [[SimpleTest]]: [MySQL] 55,540 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, core-update_file_to_twig-1898070-50.patch, failed testing.

Cottser’s picture

Assigned:Unassigned» Cottser
Issue tags:-Novice, -Needs reroll

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

bdgreen’s picture

StatusFileSize
new14.27 KB
PASSED: [[SimpleTest]]: [MySQL] 55,408 pass(es).
[ View ]

@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?

Cottser’s picture

Status:Needs work» Needs review

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 :)

bdgreen’s picture

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

Cottser’s picture

Assigned:Cottser» Unassigned
Status:Needs review» Needs work

@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.

bdgreen’s picture

Status:Needs work» Needs review

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">';

?

Cottser’s picture

Status:Needs review» Needs work

@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.)

bdgreen’s picture

StatusFileSize
new834 bytes
new14.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,701 pass(es).
[ View ]

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

bdgreen’s picture

Status:Needs work» Needs review
StatusFileSize
new12.41 KB
new11.09 KB
new861 bytes
new14.27 KB
PASSED: [[SimpleTest]]: [MySQL] 55,561 pass(es).
[ View ]
new14.26 KB
PASSED: [[SimpleTest]]: [MySQL] 55,644 pass(es).
[ View ]

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

Before patch #53

Before core-update_file_to_twig-1898070-53.patch.png

After patch #53

After core-update_file_to_twig-1898070-53.patch.png

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

bdgreen’s picture

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

Cottser’s picture

Awesome, thanks! :)

c4rl’s picture

Title:Convert file module to Twig» file.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

hanpersand’s picture

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

jesse.d’s picture

StatusFileSize
new1.13 KB
new14.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-update_file_to_twig-1898070-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

jesse.d’s picture

StatusFileSize
new15.29 KB
new11.99 KB

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

OpenChimp’s picture

profiling this now

OpenChimp’s picture

=== 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

OpenChimp’s picture

Issue tags:+Needs profiling

profiling not run correctly. Needs to be redone

Status:Needs review» Needs work
Issue tags:-Needs profiling, -Twig

The last submitted patch, core-update_file_to_twig-1898070-65.patch, failed testing.

tlattimore’s picture

Status:Needs work» Needs review
tlattimore’s picture

Issue summary:View changes

Updated issue summary.

joelpittet’s picture

Status:Needs review» Needs work
Issue tags:+Needs profiling, +Twig

The last submitted patch, core-update_file_to_twig-1898070-65.patch, failed testing.

Cottser’s picture

Issue tags:+Needs reroll

Tagging for reroll.

rvilar’s picture

Assigned:Unassigned» rvilar

Working on the reroll

thedavidmeister’s picture

Cottser’s picture

Assigned:rvilar» Unassigned

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

sergesemashko’s picture

Assigned:Unassigned» sergesemashko
sergesemashko’s picture

Assigned:sergesemashko» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new15.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,007 pass(es), 0 fail(s), and 985 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work
Issue tags:-Needs profiling, -Twig

The last submitted patch, core-update_file_to_twig-1898070-79.patch, failed testing.

drupalninja99’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs profiling, +Twig

The last submitted patch, core-update_file_to_twig-1898070-79.patch, failed testing.

Les Lim’s picture

Assigned:Unassigned» Les Lim

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

Les Lim’s picture

Status:Needs work» Needs review
StatusFileSize
new14.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,105 pass(es), 0 fail(s), and 439 exception(s).
[ View ]

Re-rolled #65.

Les Lim’s picture

StatusFileSize
new1.35 KB
new14.99 KB
PASSED: [[SimpleTest]]: [MySQL] 57,444 pass(es).
[ View ]

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().

Les Lim’s picture

Assigned:Les Lim» Unassigned

Unassigning.

xjm’s picture

Issue tags:+Needs reroll

And again.

Cottser’s picture

Status:Needs review» Needs work
sergesemashko’s picture

Assigned:Unassigned» sergesemashko
sergesemashko’s picture

Assigned:sergesemashko» Unassigned
tlattimore’s picture

Status:Needs work» Needs review
StatusFileSize
new14.94 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/file.field.inc.
[ View ]

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

tlattimore’s picture

Issue tags:-Needs reroll

removing tag.

Cottser’s picture

Issue tags:+Needs manual testing

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.

Status:Needs review» Needs work

The last submitted patch, 1898070-file_module_twig-91.patch, failed testing.

tlattimore’s picture

Status:Needs work» Needs review
StatusFileSize
new659 bytes
new14.94 KB
PASSED: [[SimpleTest]]: [MySQL] 57,963 pass(es).
[ View ]

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

joelpittet’s picture

Status:Needs review» Needs work

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.

tlattimore’s picture

Status:Needs work» Needs review
StatusFileSize
new5.22 KB
new14.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]

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

oshelach’s picture

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.

joelpittet’s picture

@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.

joelpittet’s picture

Status:Needs review» Needs work

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

oshelach’s picture

StatusFileSize
new2.37 KB
new14.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,926 pass(es).
[ View ]

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.

joelpittet’s picture

Status:Needs work» Needs review

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!

oshelach’s picture

StatusFileSize
new624 bytes
new14.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,991 pass(es).
[ View ]

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

thedavidmeister’s picture

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

thedavidmeister’s picture

Cottser’s picture

+++ 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).

thedavidmeister’s picture

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?

oshelach’s picture

Assigned:oshelach» Unassigned
Status:Needs work» Needs review

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

thedavidmeister’s picture

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 :)

oshelach’s picture

Assigned:Unassigned» oshelach
Status:Needs review» Needs work
oshelach’s picture

Assigned:Unassigned» oshelach
StatusFileSize
new1.34 KB
new14.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,273 pass(es).
[ View ]

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.

joelpittet’s picture

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.

oshelach’s picture

StatusFileSize
new402 bytes
new14.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,239 pass(es).
[ View ]
Cottser’s picture

Status:Needs review» Needs work

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.

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new2.14 KB
new14.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]

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?

oshelach’s picture

Issue summary:View changes

Updated issue summary.

Cottser’s picture

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.

oshelach’s picture

Cool, that's already in the patch.

oshelach’s picture

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

Cottser’s picture

Status:Needs review» Needs work

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.

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.44 KB
new14.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_module-twig-1898070-120.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, file_module-twig-1898070-120.patch, failed testing.

Cottser’s picture

Issue tags:+Needs reroll

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

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
new14.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,717 pass(es).
[ View ]

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?

Cottser’s picture

Issue tags:-Needs reroll

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

joelpittet’s picture

Status:Needs review» Needs work
  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.

drupalninja99’s picture

Issue tags:+Needs reroll

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

drupalninja99’s picture

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
+ */
+#}
Cottser’s picture

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.

drupalninja99’s picture

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.

joelpittet’s picture

Assigned:oshelach» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new14.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,467 pass(es).
[ View ]

Here's the re-roll from #123

joelpittet’s picture

StatusFileSize
new4.77 KB
new15.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898070-131-twig-theme-file.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

As I mentioned in #125

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

joelpittet’s picture

Issue summary:View changes

Update testing steps

joelpittet’s picture

chanderbhushan’s picture

Issue summary:View changes
Status:Needs review» Needs work

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

Cottser’s picture

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

chanderbhushan’s picture

Status:Needs work» Needs review
chanderbhushan’s picture

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

joelpittet’s picture

@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.
chanderbhushan’s picture

@joelpittet it is working fine now thanks

joelpittet’s picture

@chanderbhushan great, are you working on manual testing?

chanderbhushan’s picture

@joelpittet Yes, i am doing manual testing.

LinL’s picture

Status:Needs review» Needs work

The last submitted patch, 131: 1898070-131-twig-theme-file.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new15.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898070-143-twig-theme-file.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

mikl’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 143: 1898070-143-twig-theme-file.patch, failed testing.

idflood’s picture

Status:Needs work» Needs review
StatusFileSize
new15.32 KB
PASSED: [[SimpleTest]]: [MySQL] 63,164 pass(es).
[ View ]

Simple reroll of patch in #143.

Cottser’s picture

Cottser’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

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

LinL’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new15.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,406 pass(es).
[ View ]

Quick reroll of #146.

LinL’s picture

StatusFileSize
new15.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,424 pass(es).
[ View ]

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

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

Cottser’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Tagging for reroll.

jjcarrion’s picture

Assigned:Unassigned» jjcarrion
jjcarrion’s picture

Assigned:jjcarrion» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new15.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,800 pass(es).
[ View ]

reroll made

m1r1k’s picture

Status:Needs review» Reviewed & tested by the community

Works fine, also tested here #2265127: Update file theming

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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

joelpittet’s picture

Issue tags:+frontend
joelpittet’s picture

Issue tags:+Novice

Tagging for sprints

joelpittet’s picture

Issue summary:View changes
benjifisher’s picture

Assigned:Unassigned» benjifisher

I am working on profiling.

benjifisher’s picture

StatusFileSize
new12.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,438 pass(es), 1,125 fail(s), and 172 exception(s).
[ View ]

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

benjifisher’s picture

Assigned:benjifisher» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs profiling, -Novice

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&...

steinmb’s picture

Assigned:Unassigned» steinmb

Tests test

Status:Needs review» Needs work

The last submitted patch, 160: 1898070-160-twig-theme-file.patch, failed testing.

steinmb’s picture

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’s picture

Assigned:steinmb» Unassigned
benjifisher’s picture

Status:Needs work» Needs review
StatusFileSize
new15.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,766 pass(es).
[ View ]

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

steinmb’s picture

Status:Needs review» Needs work

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
steinmb’s picture

StatusFileSize
new21.84 KB

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

two messages

steinmb’s picture

Status:Needs work» Reviewed & tested by the community

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

joelpittet’s picture

Issue tags:-Needs manual testing

Awesome work @benjifisher and @steinmb!

Cottser’s picture

Status:Reviewed & tested by the community» Needs work

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.

joelpittet’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new15.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/file.module.
[ View ]

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 172: 1898070-twig-theme-file-172.patch, failed testing.

joelpittet’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new15.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,293 pass(es).
[ View ]

typo, back to rtbc.

Cottser’s picture

Works for me, no interdiff though? :P

joelpittet’s picture

StatusFileSize
new2.36 KB

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

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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.

joelpittet’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new15.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,359 pass(es).
[ View ]

Re-rolled.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Great work, all!

Committed and pushed to 8.x. Thanks!

  • Commit a4a887a on 8.x by webchick:
    Issue #1898070 by benjifisher, jjcarrion, LinL, idflood, joelpittet,...

Status:Fixed» Closed (fixed)

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