Convert theme_update_version to update-version.html.twig

As a developer, I prefer #2 (the "for themers" one) overall.

A template with only {{ table }} in it is nearly useless IMO. If that's what we get for the #type table option, I don't know what benefit there is from providing a template there at all.

And thank you for doing this :)

Yep, agreed with Cottser. #2 is also closer to D7's theme function, which means less wrangling for the (uh, 1 ;)) site(s) out there who want to override this template.

I can understand the rationale for wanting code-reuse, and if we were talking about table.twig vs. views-table.twig or something like that, where both were going to be used throughout a site and it would be super annoying to have to make changes in both to keep them consistent, then this would be a tougher choice. But we're talking about a one-off template in a very specialized module that's basically only for user 1.

What might be nice though is a brief comment in the template above 'template' => 'update-version', explaining why we didn't just use #type => table here. And possibly also a normal, nice-to-have follow-up to bring this page generally more inline with other admin tables. For example, if those "download / release notes" were under an "operations" drop button we probably wouldn't need to do such special-casing here. But I don't want to hold a straight conversion issue up on a redesign.

A template with only {{ table }} in it is nearly useless IMO. If that's what we get for the #type table option, I don't know what benefit there is from providing a template there at all.

That's what I've been saying a dozen of times already:

It's pretty much pointless to have a theme function/template for tables. A table is a table. There's hardly a use-case for overriding the markup of a table.

For the same reason, it doesn't make sense to repeat the entire markup for tables in individual templates. That only achieves the opposite: Inconsistency + duplicate code + unmaintainable templates.

The only sensible use-case is to replace the table altogether with markup that is not a table. That's what should lead the architecture and what we should optimize for.

— i.e., hand me the underlying (table) data in a way I can replace the (entire) table output with custom markup of my choice (in a particular place). But don't make me re-invent every single table from scratch.

Given all the points everyone has made so far, the "-themers" version makes the most sense to me:

1. It keeps markup out of preprocess
2. It keeps Twig template useful
3. It allows for changing the format--from table to _whatever_--easily, for a themer.

I would +1 RTBC this if a comment was added to the themers patch as webchick suggested in #5 ("a brief comment in the template above 'template' => 'update-version', explaining why we didn't just use #type => table here").

Waiting on #1898466: update.module - Convert theme_ functions to Twig to reroll patch.

Rerolled patch. Comment added.

May be "keep markup out of preprocess and easier template changes"?

Updated patch.

Looks good. I agree that a solitary {{ table }} is pointless in a template, even this one.

My only comments before considering this RTBC:

1. +++ b/core/modules/update/templates/update-version.html.twig
   @@ -35,4 +35,4 @@
      </tr>
   -</table>
   \ No newline at end of file
   +</table>
   

   Need newline back.

2. +++ b/core/modules/update/templates/update-version.html.twig
   @@ -0,0 +1,38 @@
   +{#
   +/**
   + * @file
   + * Default theme implementation for the version display of a project.
   + *
   + * Available variables:
   + * - title: The title of the project.
   + * - version:  A list of data about the latest released version, containing:
   + *   - version: The version number.
   + *   - date: The date of the release.
   + *   - download_link: The URL for the downloadable file.
   + *   - release_link: The URL for the release notes.
   + * - attributes: HTML attributes for the update versions table.
   + *
   + * @see template_preprocess_update_version()
   + *
   + * @ingroup themeable
   + */
   +#}
   

   If we're not going to presume a table is in this template, maybe genericize the attributes description, like this?:

   - attributes: HTML attributes suitable for a container element.

Thanks for review! Updated the patch.

This one's cooked!

Committed 4f5d858 and pushed to 8.x. Thanks!

diff --git a/core/modules/update/update.module b/core/modules/update/update.module
index f56fa87..ec3e1b4 100644
--- a/core/modules/update/update.module
+++ b/core/modules/update/update.module
@@ -191,9 +191,8 @@ function update_theme() {
     'update_version' => array(
       'variables' => array('version' => NULL, 'title' => NULL, 'attributes' => array()),
       'file' => 'update.report.inc',
-      // We are using template instead of '#type' => 'table' here to
-      // keep markup out of preprocess and allow for easier
-      // changes to markup.
+      // We are using template instead of '#type' => 'table' here to keep markup
+      // out of preprocess and allow for easier changes to markup.
       'template' => 'update-version',
     ),
   );

Reflowed comment on commit to use the full 80 characters.