Problem/Motivation

$row_classes should be on the context of the row which is being rendered.

Proposed resolution

Move $row_classes to $row['attributes'] with the Attribute class.
This will need to move to move the $row nested further to $row['content']

Remaining Tasks

  • manual testing, with screenshots in #31
  • needs code review

User interface changes

n/a

API changes

$row_classes will no longer be available in views templates. Need to be converted to print $row['attributes'], and in twig {{ row.attributes }}
and $row will become $row['content'] or {{ row.attributes }}

#1843754: Convert views/templates/views-view-list.tpl.php to twig
#1843770: Convert views/templates/views-view-unformatted.tpl.php to twig

Files: 
CommentFileSizeAuthor
#68 1968398-68-views-row-classes-tests-only.patch963 bytesjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 57,988 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#68 1968398-68-views-row-classes.patch4.95 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,923 pass(es).
[ View ]
#65 admin-people-before.png15.64 KBechoz
#65 admin-people-after.png17.08 KBechoz
#63 Screen Shot 2013-08-04 at 11.38.42 PM.png79.81 KBmarkcarver
#62 1968398-62-views-row-classes.patch4 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,886 pass(es).
[ View ]
#62 interdiff.txt954 bytesjoelpittet
#61 1968398-61-views-row-classes.patch3.95 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,564 pass(es).
[ View ]
#61 interdiff.txt4.08 KBjoelpittet
#61 interdiff.txt4.08 KBjoelpittet
#60 Kaleidoscope_–_“_table_class…”___“_table_class…”_2.png250.66 KBjoelpittet
#59 dropbutton-pushed-right.png11.32 KBechoz
#57 table-regression1.png49.61 KBechoz
#57 table-regression2.png49.05 KBechoz
#52 drupal-convert-views-row_classes-1968398-52.patch24.09 KBmarkcarver
PASSED: [[SimpleTest]]: [MySQL] 57,187 pass(es).
[ View ]
#52 interdiff.txt2 KBmarkcarver
#50 interdiff.txt1.41 KBmarkcarver
#49 drupal-convert-views-row_classes-1968398-49.patch23.72 KBmarkcarver
FAILED: [[SimpleTest]]: [MySQL] 57,506 pass(es), 0 fail(s), and 66 exception(s).
[ View ]
#49 interdiff.txt22.97 KBmarkcarver
#47 core-1968398-47-views_row_classes_to_attributes.patch22.43 KBLes Lim
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#45 drupal-convert-views-row_classes-1968398-45.patch22.94 KBmarkcarver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert-views-row_classes-1968398-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 interdiff.txt978 bytesmarkcarver
#42 articles_unformatted_list.jpeg49.77 KBLes Lim
#42 articles_table.jpeg78.32 KBLes Lim
#42 articles_html_list.jpeg95.59 KBLes Lim
#42 articles_grid.jpeg76.02 KBLes Lim
#39 core-1968398-39-views_row_classes_to_attributes.patch22.21 KBLes Lim
PASSED: [[SimpleTest]]: [MySQL] 57,117 pass(es).
[ View ]
#39 interdiff-1968398-37-39.txt559 bytesLes Lim
#37 core-1968398-37-views_row_classes_to_attributes.patch22.23 KBLes Lim
PASSED: [[SimpleTest]]: [MySQL] 57,460 pass(es).
[ View ]
#31 1968398-grid.png137.51 KBR.Hendel
#31 1968398-html.png85.23 KBR.Hendel
#31 1968398-table.png89.9 KBR.Hendel
#31 1968398-unformatted.png114.78 KBR.Hendel
#26 1968398-26-views-row-classes.patch25.93 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1968398-26-views-row-classes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 1968398-23-views-row-classes.patch25.75 KBbrunodbo
PASSED: [[SimpleTest]]: [MySQL] 55,455 pass(es).
[ View ]
#20 1968398-20-views-row-classes.patch26.64 KBbrunodbo
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views.theme.inc.
[ View ]
#17 1968398-17-views-row-classes.patch25.76 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,898 pass(es).
[ View ]
#13 2013-05-22 10.20.55 pm.png238.18 KBintergalactic overlords
#13 2013-05-22 10.35.15 pm.png27.45 KBintergalactic overlords
#13 2013-05-22 10.35.40 pm.png126.48 KBintergalactic overlords
#12 1968398-12-views-row-classes.patch23.14 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 56,069 pass(es).
[ View ]
#12 interdiff.txt1.16 KBjoelpittet
#10 views-classes-to-attributes-1968398-10.patch23.08 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 56,912 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#6 interdiff.txt35.05 KBjoelpittet
#6 views-classes-to-attributes-1968398-6.patch22.93 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,311 pass(es).
[ View ]
#4 views-classes-to-attributes-1968398-4.patch55.55 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,528 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#4 interdiff.txt769 bytesjoelpittet
#1 views-classes-to-attributes-1968398-1.patch55.55 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/templates/views-view-grid.tpl.php.
[ View ]

Comments

joelpittet’s picture

Status:Active» Needs review
StatusFileSize
new55.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/templates/views-view-grid.tpl.php.
[ View ]

Rough cut. Let me know if anything needs to change. Also converted Object references to Arrays for consistency and used all my powers to avoid doing any 'extra' cleanup.

dawehner’s picture

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

Status:Needs review» Needs work

The last submitted patch, views-classes-to-attributes-1968398-1.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new769 bytes
new55.55 KB
FAILED: [[SimpleTest]]: [MySQL] 54,528 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

@dawehner I agree, I prefer that too, it makes sense like in JS to have rows[{key:val},{key:val}], but it seems arrays are king, and PHP's version of object literals is not so cool( new stdClass()). So for now I was instructed to make things the same.

Here's a patch with that typo I missed in local testing.

Status:Needs review» Needs work

The last submitted patch, views-classes-to-attributes-1968398-4.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new22.93 KB
PASSED: [[SimpleTest]]: [MySQL] 54,311 pass(es).
[ View ]
new35.05 KB

Undid some of my whitespace fixes and talked to some chx and merlinofchaos on IRC #drupal-vdc to confirm my "fix" for field content always being empty was indeed a mistake. So removed that. Also a random file snuck in there:S

Interdiff is against #1

dawehner’s picture

Issue tags:+Needs manual testing

This needs seriously manual testing :)

joelpittet’s picture

Issue tags:+Twig
clemens.tolboom’s picture

Status:Needs review» Needs work

I cannot apply patch #6 to latest head.

joelpittet’s picture

Assigned:joelpittet» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.08 KB
FAILED: [[SimpleTest]]: [MySQL] 56,912 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Thanks @clemens.tolboom needed a re-roll cus it was older.

Status:Needs review» Needs work

The last submitted patch, views-classes-to-attributes-1968398-10.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new23.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,069 pass(es).
[ View ]

fixing that rebase bit for views table.

intergalactic overlords’s picture

StatusFileSize
new126.48 KB
new27.45 KB
new238.18 KB

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)
intergalactic overlords’s picture

Status:Needs review» Needs work

see #13

dawehner’s picture

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

dawehner’s picture

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

joelpittet’s picture

StatusFileSize
new25.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,898 pass(es).
[ View ]

#15 and #16 no longer exist as they are now twig templates. Thanks a lot guys for reviewing. I spotted my bugs in grid, thanks @intergalactic o...

Here's a re-roll, with fixes.

joelpittet’s picture

Status:Needs work» Needs review
joelpittet’s picture

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

Tagging, needs re-roll due to other fixes regarding VDC renaming.

brunodbo’s picture

Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll
StatusFileSize
new26.64 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views.theme.inc.
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, 1968398-20-views-row-classes.patch, failed testing.

brunodbo’s picture

Woops, bad reroll. New one incoming.

brunodbo’s picture

StatusFileSize
new25.75 KB
PASSED: [[SimpleTest]]: [MySQL] 55,455 pass(es).
[ View ]

New reroll.

brunodbo’s picture

Status:Needs work» Needs review

Go testbot.

adamcowboy’s picture

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

Need reroll

joelpittet’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new25.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1968398-26-views-row-classes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled

R.Hendel’s picture

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

markcarver’s picture

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.

R.Hendel’s picture

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

joelpittet’s picture

#26: 1968398-26-views-row-classes.patch queued for re-testing.

R.Hendel’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new114.78 KB
new89.9 KB
new85.23 KB
new137.51 KB

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:

1968398-grid.pnghtml:

1968398-html.pngtable:

1968398-table.pngunformatted:

1968398-unformatted.png

dawehner’s picture

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.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review

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.

joelpittet’s picture

#26: 1968398-26-views-row-classes.patch queued for re-testing.

joelpittet’s picture

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

This will need a re-roll now that grid is all gussied up.

#1903746: Replace the views grid table template with one using divs

Les Lim’s picture

Assigned:Unassigned» Les Lim

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

Les Lim’s picture

Status:Needs work» Needs review
StatusFileSize
new22.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,460 pass(es).
[ View ]

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.

markcarver’s picture

Issue tags:-Needs reroll

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!

Les Lim’s picture

StatusFileSize
new559 bytes
new22.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,117 pass(es).
[ View ]

New patch reflecting #38.

markcarver’s picture

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

dawehner’s picture

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

Les Lim’s picture

Issue tags:-Needs manual testing
StatusFileSize
new76.02 KB
new95.59 KB
new78.32 KB
new49.77 KB

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.

unformatted list view, after patchtable view, after patchhtml list view, after patchgrid view, after patch

markcarver’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

markcarver’s picture

Status:Needs work» Needs review
StatusFileSize
new978 bytes
new22.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert-views-row_classes-1968398-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-convert-views-row_classes-1968398-45.patch, failed testing.

Les Lim’s picture

Status:Needs work» Needs review
StatusFileSize
new22.43 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-rolled #45.

Status:Needs review» Needs work

The last submitted patch, core-1968398-47-views_row_classes_to_attributes.patch, failed testing.

markcarver’s picture

Status:Needs work» Needs review
StatusFileSize
new22.97 KB
new23.72 KB
FAILED: [[SimpleTest]]: [MySQL] 57,506 pass(es), 0 fail(s), and 66 exception(s).
[ View ]

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.

markcarver’s picture

StatusFileSize
new1.41 KB

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

Status:Needs review» Needs work

The last submitted patch, drupal-convert-views-row_classes-1968398-49.patch, failed testing.

markcarver’s picture

Status:Needs work» Needs review
StatusFileSize
new2 KB
new24.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,187 pass(es).
[ View ]

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.

Fabianx’s picture

Looks good!

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Agreed

bdone’s picture

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

echoz’s picture

Status:Fixed» Needs work
StatusFileSize
new49.05 KB
new49.61 KB

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

table-regression1.png

field translation link td

table-regression2.png

joelpittet’s picture

Assigned:Les Lim» Unassigned

@echoz re #57 From your screenshots I can't identify the visual regression you are speaking of and the firebug output in there has identical classes and markup. Can you show the before and after visual change and more helpfully the markup that has produced the visual change?

Thanks for looking into this!

echoz’s picture

StatusFileSize
new11.32 KB

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

dropbutton-pushed-right.png

joelpittet’s picture

I see what you're getting at, here's a markup comparison on that page. I'll see if I can't find where this is going wrong.

Kaleidoscope_–_“_table_class…”___“_table_class…”_2.png

FYI, this comparison is done against HEAD with the patch #52 reversed, if anybody cares to try before HEAD moves;)

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
new4.08 KB
new3.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,564 pass(es).
[ View ]

Thanks @echoz I think I found the issue. I forgot that we moved the attributes directly on the field being built instead of it's own variables. So what was happening is that the attributes that the hidden/excluded fields would have gotten if they weren't excluded was building the column unintentionally.

I moved the exclude check higher up which should do two things:

  • Stop rendering columns that should be excluded.
  • Stop needlessly calling "new Attribute()" for columns that aren't shown which should help a bit with performance.

I also moved the long column key reference to a reference variable so that less mistakes are made reading that path and copying it, because I noticed another mistake like that in the code. See below:

+++ b/core/modules/views/views.theme.inc
@@ -642,22 +643,21 @@ function template_preprocess_views_view_table(&$variables) {
+          if (!empty($variables['rows'][$num][$column]['content'])) {

missing 'columns'

joelpittet’s picture

StatusFileSize
new954 bytes
new4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,886 pass(es).
[ View ]

Ha, thing I said I fixed I forgot... so here's that.

markcarver’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new79.81 KB

Patch in #62 is good. Tested the patch in simplytest.me:
Screen Shot 2013-08-04 at 11.38.42 PM.png

joelpittet’s picture

This would be nice if it had a test to check that the container doesn't get rendered maybe if it's excluded. There is exclude tests in views already but it doesn't check the container... not totally sure if this test is needed but putting it out there...

echoz’s picture

StatusFileSize
new17.08 KB
new15.64 KB

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

Before patch #62

admin-people-before.png

After patch #62

admin-people-after.png

alexpott’s picture

Priority:Normal» Critical
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests
+++ 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.

joelpittet’s picture

Assigned:Unassigned» joelpittet

Not sure where the tests should go or if I should try to augment the FieldUnitTest.php where it already tests the exclude, so I'll start with that and if anybody has some hints please send.

I'm assigning this to myself, in the meantime.

joelpittet’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new4.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,923 pass(es).
[ View ]
new963 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,988 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

First patch should fail and second should pass. Let me know if the tests will fit the bill?

markcarver’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

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

Anonymous’s picture

Issue summary:View changes

added remaining tasks

joelpittet’s picture

Assigned:joelpittet» Unassigned
Issue summary:View changes