Convert Views $row_classes to $row['attributes']

Mh okay. At the moment I would prefer $row->foo over $row['foo'] but yeah if this will changed during twig, that's okay.

This needs seriously manual testing :)

I cannot apply patch #6 to latest head.

The views table, html list and unformatted list are all fine.

There are some problems with the summaries and with the grid view though:

• The grid view shows a big heap of notices. (see screenshot)
• Both the unformatted summary and the list summary have something really wrong. It's as if only the page.tpl.php is shown, without the html.tpl.php wrapping it. (see screenshots)

see #13

+++ b/core/modules/views/templates/views-view-summary-unformatted.tpl.phpundefined
@@ -9,12 +9,12 @@
+    <?php if (!empty($row['separator'])): ?><? print $row['separator']; ?><?php endif; ?>

Afaik we need <?php instead of <? here as it breaks on the testbot (if there is a test (see the glossary issue))

+++ b/core/modules/views/templates/views-view-table.tpl.phpundefined
@@ -6,15 +6,10 @@
  * - $header: An array of header labels keyed by field id.

Just in general: If we change the actual content of these variables (for example attributes to them) these documentations should be adapted as well.

+++ b/core/modules/views/templates/views-view-summary-unformatted.tpl.phpundefined
@@ -9,12 +9,12 @@
+    <?php if (!empty($row['separator'])): ?><? print $row['separator']; ?><?php endif; ?>

Afaik we need <?php instead of <? here as it breaks on the testbot (if there is a test (see the glossary issue))

+++ b/core/modules/views/templates/views-view-table.tpl.phpundefined
@@ -6,15 +6,10 @@
  * - $header: An array of header labels keyed by field id.

Just in general: If we change the actual content of these variables (for example attributes to them) these documentations should be adapted as well.

Reroll.

Woops, bad reroll. New one incoming.

New reroll.

Go testbot.

Need reroll

Yesterday I have tested #23. I created a view with for formats:
- grid
- html
- table
- unformatted

I compared the outcome before and after patching.

Everything looks fine to me besides html. I got following php-errors:

    User error: "0" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc).
    User error: "1" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc).
    User error: "0" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc).
    User error: "0" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc).
    User error: "1" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc).

Will have to see if this happens also with latest patch...

Might be worth while to take a look at #1903746-44: Replace the views grid table template with one using divs.

I changed the Grid style to use the new Attribute system as well.

I have tested latest patch #26 at simplytest.me.
There also happend that php error User error: "0" is an invalid render array key in element_children() (line 5271 of core/includes/common.inc)..
But error messages were displayed at views backend this time. So this seems nothing to do for me with a special format. I'm not sure if this has something to do with that issue at all - but I'm not a dev... :-(

I have tested patch #26 at simplytest.me like I have done #27. I created a view with for formats:
- grid
- html
- table
- unformatted

This time everything worked fine and I got no error warnings, see screenshots:

grid:

html:

table:

unformatted:

The code looks great in general but I have some fear that these preprocess code gets more and more basically impossible to read/maintain. An array of 6 levels feels like madness.

I updated the issue summary with a remaining tasks section.
The manual testing is awesome.

Still needs a code review before it can be rtbc.

Also needs to be re-rolled now that $vars is $variables everywhere. Working on it.

Re-rolled #26.

The grid-related changes committed in #1903746: Replace the views grid table template with one using divs have made the corresponding parts of this patch obsolete.

Patch looks good! Just one tiny nitpick that I'm aware of:

+++ b/core/modules/views/views.theme.inc
@@ -927,7 +929,7 @@ function template_preprocess_views_view_list(&$variables) {
+    $variables['attributes'] = new Attribute(array('class' => $wrapper_class));

Don't need to instantiate a new Attribute class here, it's now lazy loaded in (see: #1982024: Lazy-load Attribute objects later in the rendering process only if needed). Change it so it's just an array.

After that, if patch is green and manual testing is done, this looks to be an RTBC issue!

New patch reflecting #38.

Reviewed revisions, patch is good. RTBC once manual testing is done and test comes back green.

It would feel better if someone could do another great manual testing like #31

Tested a view with 4 displays, each with a custom row class set to "foo-[counter]". Hacked Bartik to add a yellow background style to "foo-2". Everything looks good.

The yellow background doesn't show in the grid display because of #2048897: Views Grid style: tokens not replaced in custom row/column classes, but that's a separate issue. The row classes are otherwise correct.

Looks good to me.

+++ b/core/modules/views/templates/views-view-table.html.twigundefined
@@ -7,21 +7,21 @@
+ *   - column: @todo.
+ *     - attributes: HTML classes to apply to each row.
+ *     - content: The header column content.

I think this @todo is unnecessary

Ok, I updated the doc for this template (I think it just got overlooked). There were a few other things that needed to be changed too.

Re-rolled #45.

Here's the patch that actually fixes the two $vars that were added by #864006-81: Improve table semantics by adding scope or headers/id attributes and needed refactoring anyway.

Here's the real interdiff from #47 -> #49.

Errors were because the row was continuing on empty rows. This caused the attributes array from being converted into a new Attribute object. Tests should pass now.

Looks good!

Agreed

similar work was done in #2036369: Cleanup views.theme.inc

Committed 0e6c03d and pushed to 8.x. Thanks!

I believe this commit caused this visual regression to afaict just the people view. Reverting it locally returns the view to it's expected state. There are 2 hidden cells using space and pushing the dropbutton out. These are views-field-edit-node and views-field-translation-link. Possibly unrelated, in the people view, Translation link has broken/missing handler.

field edit node td

field translation link td

The identified classes in dev tools are just highlighting in GREEN the 2 blank cells. This screenshot shows how the dropbutton is pushed 2 cells to the right, where it is supposed to be under the th "Operations". In the previous screenshots, my browser window was narrower so the dropbutton was squished and less noticble. Aren't other's getting this with the People view (admin/people)?

Patch in #62 is good. Tested the patch in simplytest.me:

Good visually here too. I know not necessary, but I had a look, so did screenshots too.

Before patch #62

After patch #62

+++ b/core/modules/views/views.theme.incundefined
@@ -616,27 +616,38 @@ function template_preprocess_views_view_table(&$variables) {
+      // Skip building the attributes and content if the field is to be excluded
+      // from the display.
+      if (!empty($fields[$field]->options['exclude'])) {
+        continue;
+      }

This is testable so we should test it - therefore we can ensure we don't break this in the same way again.

Also the priority of this is critical because things are unusable the way they are.

Test only patch failed as expected and new complete patch passed. Code looks good. Setting back to RTBC.

Committed 4962369 pushed to 8.x. Thanks for the test @joelpittet! nice work

Hi all and sorry to post into this issue but I guess it is definitely related and we should perhaps change or clarify things. Of course we can and should create a follow-up issue but I think it makes sense to link the problem here and have a short discussion perhaps...

The root of our "logical" problem can be seen in #27 already but doesn't seem to have been addressed finally in a straight way:

Have a look at the following comparison in views core module twig files:

views-view.html.twig:

...
  {{ exposed }}
  {{ attachment_before }}

  {{ rows }}
  {{ empty }}
  {{ pager }}
...

Renders "{{ rows }}"

while

views-view-unformatted.html.twig

{% for row in rows %}
  {%
    set row_classes = [
      default_row_class ? 'views-row',
    ]
  %}
  <div{{ row.attributes.addClass(row_classes) }}>
    {{- row.content -}}
  </div>
{% endfor %}

loops through rows.

So good and so clear...

What you might think now is that the longer variant from ...-unformatted.html.twig is just an optional bonus to add the row class wrappers and {{ rows }} as shorthand SHOULD also work in -unformatted.html.twig files, if you don't need it.

But NO, this leads to a watchdog full of

User error: "attributes" is an invalid render array key in Drupal\Core\Render\Element::children() (Zeile 97 in /core/lib/Drupal/Core/Render/Element.php).

So my suggestion would be to ensure that {{ rows }} always works as expected or we find a different solution to tell themers that this is a completely different structure in certain cases and needs different handling... I guess we're not the only ones who ran into this problem...

Does this only happen in -unformatted.html.twig or also in other views-view.html.twig overrides?

Any ideas? Thank you very much for your help!