Move summary theme output into twig template

+1 RTBC

A recent change broke applying this patch. I have re-rolled it, attached.

Thanks for your efforts!

Here is one more step forward.

The patch contains the next things:

- Made the output identical to the current one
- Fixed coding standards
- Some other improvements

The changes in #7 _look_ good to me (not deployed).

One more small update. I'm going to commit it tomorrow.

In the summary output we have 'microdata' stuff, the problem is that it doesn't acctually work(ported).
In my opinion the best way is to remove it in the followup issue. In the future it can be reintrodused, but for now the module has more important problems.
Any other opinions?

Examples:

/**
 * Implements hook_microdata_suggestions().
 */
function fivestar_microdata_suggestions() {
  $mappings = [];

  // Add the review mapping for Schema.org.
  $mappings['fields']['fivestar']['schema.org'] = [
    '#itemprop' => ['aggregateRating'],
    '#is_item' => TRUE,
    '#itemtype' => ['http://schema.org/AggregateRating'],
    'average_rating' => [
      '#itemprop' => ['ratingValue'],
    ],
    'rating_count' => [
      '#itemprop' => ['ratingCount'],
    ],
  ];

  return $mappings;
}


    if (!empty($variables['microdata']['average_rating']['#attributes'])) {
      $variables['average_rating_microdata'] = new Attribute($variables['microdata']['average_rating']['#attributes']);
    }

    if (!empty($variables['microdata']['rating_count']['#attributes'])) {
      $variables['rating_count_microdata'] = new Attribute($variables['microdata']['rating_count']['#attributes']);
    }

Some review comments for you... hope they are helpful.

1. +++ b/fivestar.module
   @@ -336,32 +336,36 @@
   +    if ($variables['user_rating'] !== 0) {
   

   I'm not convinced about the "!== 0" test here. Could it be rendered as "> 0" instead, for clarity, or is it not always numeric? Testing for "*not* a very specific thing" is often a poor choice in programming, and it seems possible that is true here.

2. +++ b/fivestar.module
   @@ -336,32 +336,36 @@
   +      $variables['user_stars'] = round(($variables['user_rating'] * $variables['stars']) / 100, 1);
   

   Several expressions in this and other code would be easier to read without constant referral to $variables[], e.g. by copying them into local vars.

   This would also help several lines not extend beyond 80 chars.

3. +++ b/fivestar.module
   @@ -336,32 +336,36 @@
   +      $output_type = isset($variables['votes']) ? 'average-count' : 'average';
   

   Could we have a bool defined near the top of the function like:

   $is_voting = isset($variables['votes']);

   and then use that in the other ?: statements?

   I think that would make the code easier to read in general, as well as being shorter and (marginally) faster.

4. +++ b/fivestar.module
   @@ -336,32 +336,36 @@
   +      $variables['average_rating_microdata'] = new Attribute($variables['microdata']['average_rating']['#attributes']);
   

   You asked about the microdata-referencing code. I haven't looked into it but would prefer that whatever progress has been made towards a port was left, unless its incompleteness is breaking things. This module has enough work to do without reverting parts.

   I think the right thing would be to create a new Issue covering the feature, with reference to the known flaws. Possibly, the code could be commented to reference that issue:

   // Incomplete: See [#1234567889] for details.

5. +++ b/templates/fivestar-summary.html.twig
   @@ -8,10 +8,10 @@
   + * - user_rating: The current user rating.
   

   While good to add docs, such comments don't really help much because you can guess that from the variable name. Improve it by being more specific:

   "user_rating: The rating set by the current user on the enclosing entity as an integer 1..100, or NULL if no rating is set".

   Note I stated it's an integer & not a string, what range it has, and exceptional values.

   Also, "the current user rating" is ambiguous, because it could refer to the rating *of* a user themselves, or refer to the rating *by* a user of a thing.

6. +++ b/templates/fivestar-summary.html.twig
   @@ -8,10 +8,10 @@
   + * - output_type: output type indicator.
   

   Same as earlier... try something like:

   output_type: a word indicating the desired output style. One of:
   - "combo" for ...
   - "average" when ...
   - "" ...
   The word is guaranteed to be useful for CSS class names.

7. +++ b/templates/fivestar-summary.html.twig
   @@ -20,31 +20,28 @@
   +      {{ 'Average:'|t }} <span {{ average_rating_microdata }}>{{ average_stars }}</span>
   

   It is a common thing with html attribute twig functions that they supply their own spaces, including an initial one. Is this not true here and could it not be made so?

   [other instances of same thing appear later in file]

I actually ported this myself last year but I never got around to posting an issue and a patch for it, but I still have my code. Comparing the two, they're extremely similar, really differing only in coding style I think. The only functional difference would be in the template, where I did this:

{% set classes = ['fivestar-summary', 'fivestar-summary-' ~ output_type] %}
<div{{ attributes.addClass(classes) }}>

while you did this:
<div class="fivestar-summary fivestar-summary-{{ output_type }}">
My way allows users to add their own class to the wrapper div by specifying #attributes in their fivestar form element.

Notice that the patch passed in #7 but failed in #9 because you moved the "No votes yet" output and didn't get the logic quite right in #9.

I generally agree with everything @rivimey said in #11. We all agree to leave the microdata in for now, and I think that a follow up issue to make it work would be great. I don't want to remove it because if it's gone it will never go back in, but if it's there and not quite working then eventually it will get fixed.

I'm sure there's an issue in this queue about the microdata not fully working ... Oh, here it is, it's for D7: #3152661: Broken schemaorg for aggregate rating (empty values)
I assume the D7 problems still exist in D8.

I agree with @TR that using addClass() is the way to go, for the reasons he suggests..

I did look at the change of {%if%} conditional but presumed the person doing that had got it right. My apologies for not checking further. I would suggest, rather than re-complicating the twig, that something changes in preprocess to make (something close to the current) twig code work.

I have created #3225664: Complete port of, and correct, the handling of microdata to start the process of fixing the microdata. Please update as appropriate.

@rivimey, nice points!

1. done.
2. done.
3. done but in a slightly different way.
4. I'm ok with it:)
5. done.
6. done.
7. done

@TR, good argument about classes - updated.

Also, fixed code that was broke in the last patch. Fortunately, we have tests:)

Patch looks good to me , just a couple of minor doc comments.

1. +++ b/templates/fivestar-summary.html.twig
   @@ -0,0 +1,65 @@
   + * - average_rating: The desired average rating to display out of 100 (i.e. 80 is 4 out of 5 stars),
   ...
   + * - user_rating: The rating set by the user on the enclosing entity as an integer 1..100,
   

   Lines exceed 80 chars. Break between "80" & "is", and before "integer"

2. +++ b/templates/fivestar-summary.html.twig
   @@ -0,0 +1,65 @@
   + *   microdata:
   

   Fill in purpose (or at least put something, if we don't know!)

3. +++ b/templates/fivestar-summary.html.twig
   @@ -0,0 +1,65 @@
   + *   or NULL for not to show average rating. Defaults to NULL.
   ...
   + *   or NULL for not to show user rating. Defaults to NULL.
   

   For my code I would always indent wrapped doc lines another 2 spaces, to make it easier to scan the list. Not sure if this is in the code standards, but good to do, IMO.

I've updated the patch for points 1 and 2.
3 - left as it is, because most core templates use a similar format.

Committed, thanks!