theme_table() should not output the no_striping option as an html attribute

Patch works fine for me.

Patch works fine for me as well.

The better solution (for Drupal 8) is probably to separate data/content from settings. Lumping things together in one array is a bad idea -- this patch is a bit of a hack. Shall we fix this in Drupal 8 first?

I can agree with separating different concerns. However I disagree about changing this in D8 first. This patch is about solving, not introducing, an error due to not separating stuff. The changes for D8 would introduce an API change and thus won't be ported back to D7. I would suggest fixing the error in D7 using this patch, and creating a new feature request/task for D8.

Aside: #616240: Make Field UI screens extensible from contrib - part II introduced the no_striping option. Scanning through that one, I can understand that this slipped through.

Per the discussion at #1050616: Figure out backport workflow from Drupal 8 to Drupal 7, current consensus is that quick fixes get applied to both 7.x and 8.x, and then a new 8.x issue is opened for the proper fix. The idea is to prevent Drupal 7 patches being stalled on API changes in 8.x, while also ensuring that 8.x isn't released with regressions compared to Drupal 7. That issue is still open but it's the best we have for a strategy for dealing with this.

#1: 935066_no_striping_1.patch queued for re-testing.

[comment removed: same comment posted twice, next one has the patch attached]

Putting several things together in 1 array is quite normal (at least in an untyped language like PHP). Examples are:
- Form arrays in Drupal: childs and processing data/instructions.
- Render elements in Zend framework: properties (server side) and attributes (client side) are all put in the same array (at the same level).
- Data, properties ('header') and attributes in table cells in Drupal!

This patch is more in line with how function _theme_table_cell (in this same file) handles the same issue. Unsetting server side used elements, remaining elements are attributes.

I can confirm the patch in #12 works against D7.10. I haven't touched D8 yet, so I didn't verify this.

Re-rolled against D8. Works great, the invalid attribute is gone and the striping still works.

#15: remove-no-striping-in-theme_table-935066-15.patch queued for re-testing.

Any news on this? Does the patch need a re-roll?

/me doesn't like invalid markup

Yes it did need a reroll (again). Strangely enough the patch from #10 applied nicely (after adding /core in front of the path names). apparently the white space removal in the patch from #15 was the problem.

Can you review as I can't review patches that (in the basis) are from my self.

I'm not sure that we still want to fix this nicely in D8 as it would probably need an API change. Also I still think that combining data and other things is quite common in array driven API's (see my comment in #12). So IMO, Dries' comment from #6 does no longer hold.

If Dries meant something along these lines:

// Old API.
$row = array(
  'data' => array(
    'Cell 1',
    'Cell 2',
  ),
  'no_striping' => TRUE,
  'class' => array('row-class-1', 'row-class-2'),
);

// New API.
$row = array(
  'data' => array(
    'Cell 1',
    'Cell 2',
  ),
  'no_striping' => TRUE,
  'attributes' => array(
    'class' => array('row-class-1', 'row-class-2'),
  ),
);

...then I'm all for it.

I also think it makes more sense to put the no_striping option in the root of the rows array, as opposed to on every row. And if my former suggestion makes sense, then it should also be done for the cells:

<?php
// Old API.
$cell = array(
  'data' => 'This is a header cell',
  'header' => TRUE,
  'class' => array('cell-class-1', 'cell-class-2'),
);

// New API.
$cell = array(
  'data' => 'This is a header cell',
  'header' => TRUE,
  'attributes' => array(
    'class' => array('cell-class-1', 'cell-class-2'),
  ),
);

As such, I think it makes sense to fork an issue directed at a quick D7 fix. If this is agreed, I'll go ahead.

I think we should do this. Forking 7.x as a separate issue: #1959110: theme_table outputs the no_striping option as an HTML attribute (followups)

- Can you give any idea about the impact of this change on core itself (ie: how many uses of theme_table are there)?
- #9 suggests to open a different issue for the proper fix in D8, and use 1 issue for the quick fix in both D8 and D7. So I suggest to either use #1959110: theme_table outputs the no_striping option as an HTML attribute (followups) for the proper fix or start that one with the fix for D8 and leave this one open for the proper fix.

That makes no sense. A quick fix doesn't need all the discussion that has been going on in this one, an issue with impact on the API will. I don't think #9 really intended that this issue specifically is for the quick fix. Does it matter enough to make a fuss about it? The issue summary can be edited to reflect the fact that this will be the "proper 8.x fix" issue...

As for your other point, I think Dries pointing out he wants to separate stuff better is nuff said.

Also consider this example:

$table = array(
  'rows' => array(
    array(
      'data' => array('Cell 1', 'Cell 2'),
      'no_striping' => TRUE,
    ),
    array('Cell 3', 'Cell 4'),
    array('Cell 5', 'Cell 6'),
  ),
);
print theme('table', $table);

// Outputs the following:

<table>
<tbody>
 <tr no_striping="1"><td>Cell 1</td><td>Cell 2</td> </tr>
 <tr class="odd"><td>Cell 3</td><td>Cell 4</td> </tr>
 <tr class="even"><td>Cell 5</td><td>Cell 6</td> </tr>
</tbody>
</table>

What??! Only the first row doesn't have a class for zebra striping, but the others do! I knew it made no sense to have that option on every table row, but this behavior is ridiculous, as it basically forces you to use the complex array structure for every row.

When I look at #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors (and #1942470: Zebra stripping gets messed up when rows are dynamically added/removed.), I think that this issue is going to be obsolete. The former is going to remove the no_striping "attribute" anyway, by replacing it by a separate zebra variable that will be passed to the theme. So I suggest to close this one as duplicate or won't fix and let's all join the discussion in #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors.

I beg to differ. This issue was caused because basically the API doesn't offer enough separation (see #6, #21 and #22). Removing the no_striping option (or moving it elsewhere) would solve this particular issue, but not the more general issue of the API, which wasn't addressed in #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors.
It also looks like the other issue could still take some more time to complete, whereas this one will require a few trivial changes to the theme_table() function. Then we just need to fix the invocations across the codebase. Then again, I'm not sure how much work that would be, but views comes to mind.

Broaden scope

Add some whitespace

Properly add whitespace

Fix error

Clarify

Add some related issues.

I've updated the issue summary to clarify the need for an API change. If anyone with enough permissions sees this, could the title be updated to reflect that?

typo

Looks like #1963340: Change field UI so that adding a field is a separate task will remove the only use case of 'no_striping' that we have in core. Given that, once that issue is done, how about removing it from theme_table() entirely for D8?

Hm, so what will happen in core? Will all tables have striping or none? And what will happen to contrib relying on this (or a similar) option?
I would prefer to either deprecate it or remove it in favor of zebra_striping (or both) but am not sure how I feel about removing it completely.

The original issue here about fixing the bug in the per-row no_striping is now RTBC in #1959110: theme_table outputs the no_striping option as an HTML attribute (followups).

Retitling this one for what the issue summary says, which is something else. Setting to active though, because the latest patch here doesn't do what the issue summary says, and I don't think there's consensus yet on doing that.

Hm, so what will happen in core? Will all tables have striping or none?

I still have hopes for #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors making the striping decision purely a CSS one, in which case, theme_table() just outputs minimal markup and doesn't need any kind of striping related option passed to it. But there's no guarantee of that happening, so not marking this issue a duplicate quite yet.

The above is all just my 2 cents. If I misunderstood the intent of this issue, please correct.

it offends me that someone else can edit my original bug report to make it look like I am proposing ...

Sorry that this offended you; that certainly wasn't my intention, and I don't think it was anyone else's. It is very common for issues to have their scope/approach/etc. change over time, and with Drupal core being a collaborative process, it is appropriate for people to edit summaries and titles to reflect their understandings of the evolving scope. I think only drupal.org webmasters have permission to change the issue author field though. It would be great for d.o. to make it easier to change issue authorship, or make more visible that titles/summaries were updated by multiple people, but that's a topic for a different thread.

the API change needs to be its own issue

I think that's reasonable. @a.ross: can you open one and link to it from here if you'd still like to see that done?

I certainly didn't mean to offend anyone, just trying to get things done here. However to prevent offending mr special I'll make a new issue...

Hmmm, #1939008: Convert theme_table() to Twig still contains this error, set that one back to needs work.

#1965214: Improve the theme_table API

@fietserwin, of we could continue with our issue as a follow-up of that one.

Revert to original bug report.