Comments

joelpittet’s picture

Issue summary: View changes
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
Related issues: +#1898466: update.module - Convert theme_ functions to Twig
FileSize
6.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,697 pass(es). View
3.9 KB
6.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,678 pass(es). View

I've posted two patches here. I want to bring to attention the over abstraction in the theme system and who get's the shaft.

The first one is for developers.

It uses #theme => link and #type => table but due to it's limitations you must hardcode some markup in the preprocess function to get the desired markup.

The interdiff shows the markup differences between the #type table version and the all in the template version.

The second patch is for themers.

There is no process, nor is there a need. All the variables provided to the template are enough to mark them as intended. The template becomes vastly more useful to a themer because they don't have to hunt form markup in some preprocess file and end up having to modify the preprocess and the template files to get the intended effect. That doesn't negate the preprocess's usefulness and if needed they can still have that flexibility but it's cruft. But for every merit there is a drawback. If you plan to change all tables in the system, this one wouldn't get that effect because it's not of #type => table nor is it's list of #theme => list and those global changes will have no effect on the theming of these.

I come from a background of both sides as I'm sure many of you are the same. You can also see that if we went hard and fast on either way completely we will shoot ourselves(as themers or as developers) in the foot.

I've been thinking about this for the last year but opportunity again came to bring this back to light. The last attempt I did with this concept was less show both sides and just show the themers side but I don't think that was enough. So here I've done the work twice on a fairly simple template. And even if you decide "that shouldn't be a table anyway, why is he complaining, just change it to a div and call it a day". Think a bit down the road for when those types of decisions need to be made and how much effort will it be to change a few tags in a template vs restructure a renderable array in preprocess, and which audience should be doing that work?

joelpittet’s picture

Status: Active » Needs review
Cottser’s picture

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.

Cottser’s picture

And thank you for doing this :)

webchick’s picture

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.

sun’s picture

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.

joelpittet’s picture

@sun have a gander at the preprocess function that generates the #type=>table.

If you were to use just #type=>table you'd have to build markup in PHP for the cell contents, which I'm attempting to avoid at all costs:\ See #prefix/#suffix below:

+++ b/core/modules/update/update.report.inc
@@ -346,41 +346,54 @@ function theme_update_status_label($variables) {
+          '#prefix' => ' <span class="version-date">',
+          '#markup' => format_date($version['date'], 'custom', 'Y-M-d'),
+          '#suffix' => '</span>',

AND

Without the theme function for this {{ table }} you'd have to either duplicate all that preprocess work in the controller/buildForm/etc which's create this specific#type=>table or maybe hack a 'table--update-version.html.twig' together which would be likely gross. Or (this doesn't work from what I understand, yet) do that table preprocess suggestion with ala template_preprocess_table__update_version() function.

I'd prefer the latter custom preprocess suggestion, if it worked and if that was the philosophy we'd all like to adhere to but the first point is still a big problem in my eyes (themer needs to extend that preprocess suggestion in their theme to change an HTML class of "version-date").

steveoliver’s picture

Status: Needs review » Needs work

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

l0ke’s picture

Assigned: Unassigned » l0ke
l0ke’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,993 pass(es). View

Rerolled patch. Comment added.

joelpittet’s picture

Status: Needs review » Needs work

Thanks @lokeoke here are a couple changes.

  1. +++ b/core/modules/update/templates/update-version.html.twig
    @@ -0,0 +1,38 @@
    \ No newline at end of file
    

    Need to add a newline to the end of the twig template.

  2. +++ b/core/modules/update/update.module
    @@ -189,8 +189,11 @@ function update_theme() {
    +      // keep markup out of preprocess and allow to change format easily.
    

    Maybe this would be better written as...
    from: allow to change format easily.
    to: allow for easier template changes.

    Thoughts?

l0ke’s picture

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

joelpittet’s picture

Better but strange in context:

+      // We are using template instead of '#type' => 'table' here to
+      // keep markup out of preprocess and easier template changes.

How about:

+      // We are using a template instead of '#type' => 'table' to
+      // keep markup out of preprocess and allow for easier  
+      // changes to markup.
l0ke’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
4.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,000 pass(es). View

Updated patch.

steveoliver’s picture

Status: Needs review » Needs work

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.

joelpittet’s picture

@steveoliver item #15.1 is done the newline was fixed in #14. The interdiff will getcha like that, it tricked me for a second. too;)

#15.2 I'm good with that attributes description change too, maybe move it to the top of the list as well for consistency with other templates?

l0ke’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,993 pass(es). View
886 bytes

Thanks for review! Updated the patch.

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Novice

This one's cooked!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 4f5d858 on 8.0.x
    Issue #2264833 by lokeoke, joelpittet: Convert theme_update_version to...
m1r1k’s picture

Issue tags: +#ams2014contest

Status: Fixed » Closed (fixed)

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