This issue is forked from #935066: theme_table() should not output the no_striping option as an html attribute, because I think Drupal 8 needs an API change. This issue will focus on a quick fix, so the discussion in the other issue can continue undisturbed.

Related issues that touch the same code/functionality and that will, sooner or later, obsolete this quick fix code:
- #1942470: Zebra stripping gets messed up when rows are dynamically added/removed.
- #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors

Steps to reproduce

  • Enable the PHP input filter.
  • Add content and choose the PHP input filter.
  • Enter the following code in the "body" field (including PHP tags):
    <?php
    print theme('table', array('rows' => array(array('data' => array('This is a table cell.'), 'no_striping' => TRUE))));
    ?>
  • Save and view the content.
  • Inspect the "this is a table cell" text and see that it is wrapped in a <tr> element which has an invalid "no_striping" attribute.
  • Apply the patch in #1959110-21: theme_table outputs the no_striping option as an HTML attribute (followups) and view the content again (reload the page)
  • Invalid attribute gone.

Description of the original issue report

This shows up at admin/structure/types/manage/article/display. 'no_striping'=TRUE is supposed to prevent even/odd classes from being added to rows, which it is, but it's also getting lumped in as if it were a regular attribute:

<tr class="region-message region-visible-message region-populated" no_striping="1"><td colspan="7">No field is displayed.</td> </tr>

The problem is in theme.inc. In d6, $row only contained 'data' and attributes, but in d7 'no_striping' may be in $row as well:

      if (isset($row['data'])) {
        foreach ($row as $key => $value) {
          if ($key == 'data') {
            $cells = $value;
          }
          else {
            $attributes[$key] = $value;
          }
        }
      }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

a.ross’s picture

Status: Active » Needs review
FileSize
1.33 KB

Attaching patch (in less lines of code than the original!™) :)

fietserwin’s picture

Not resetting the status to await the test results (not sure if they will be interrupted by a status change).

Needs work:
- do give $no_striping a value in the else branch as well.

Status: Needs review » Needs work

The last submitted patch, theme_table-no_striping-invalid-html-attribute_1959110-1.patch, failed testing.

a.ross’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
markabur’s picture

ignore

a.ross’s picture

Not related to 7.x. Please post in #935066: theme_table() should not output the no_striping option as an html attribute, as that one is about 8.x

marcingy’s picture

Status: Needs review » Closed (duplicate)

Issues are fixed in d8 and then backported so this is a dup of the other issue referenced.

a.ross’s picture

Status: Closed (duplicate) » Needs review

See description...

marcingy’s picture

Status: Needs review » Closed (duplicate)

Yes but it is a duplicate as issue has be fixed in head first and then backported so this fix makes a regression in d8 - see the comment from catch http://drupal.org/node/935066#comment-4448824. This is a duplicate. So you should push this quick fix being applied to d8, get it backported to d7 and then d8 can work on a fuller fix.

a.ross’s picture

Status: Closed (duplicate) » Needs review

#935066-9: theme_table() should not output the no_striping option as an html attribute

We can do the quick fix on D8 and then backport, but in the other issue a proper fix is being discussed, so IMO that should be used for the proper fix, as per the backports policy.

Edit: Crossposted, but why shouldn't the other issue be used for the proper fix?

marcingy’s picture

Well this needs to be set to d8 first - what ever happens something needs to go into d8 first and then be backported. I fully agree with you that d8 should get the simple fix as it makes no sense as per what catch said on d7 being stalled on waiting for an api change. Maybe the best way to do deal with this is...leave this open get agreement on d7 version, then move to d8 directly port the d7 version to d8. Commit to d8 and then commit the exact same fix to d7.

a.ross’s picture

Version: 7.x-dev » 8.x-dev

I already have a patch for 8.x ready, so that wouldn't be a problem as this is relatively trivial stuff. Though I admit that I overlooked the part where the quick fix had to be against 8.x first :) thanks for pointing that out. Will switch this to 8.x and upload the patch as soon as the test results come in.

Edit: oh well, already switched it by accident. I'll just leave it at that.

a.ross’s picture

Gah, patch is actually incorrect in that it changes current, albeit horrible and unpredictable, behavior. For more info on that see the other issue.
Anyways, here's a correct D8 patch.

fietserwin’s picture

Marking this issue as the quick fix.

+++ b/core/includes/theme.incundefined
@@ -2203,25 +2203,24 @@ function theme_table($variables) {
+        $no_striping = isset($attributes['no_striping']) ? $attributes['no_striping'] : FALSE;

Your patch is indeed shorter and still readable. We can further simplify by using a variable that expresses something positive (general programming rule: do not use no or not in a variable name).:

        $striping = empty($attributes['no_striping'] );

Of course, the other use of $no_striping need also to be refactored (and become more readable as well).

a.ross’s picture

I agree with that, but as the API already uses no_striping it's more consistent to use $no_striping IMO.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

It's a quick fix that's presumed to disappear anyway in D8. So marking as RTBC. will update issue summary to refer to other issues that will make this code for D8 obsolete.

fietserwin’s picture

Issue summary: View changes

Clarification.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks @a.ross and @fietserwin!

This needs an automated test that will fail when the bug is present, and pass when combined with the fix (see http://drupal.org/core-gates#testing). So, let's add that test next. Instructions: http://drupal.org/node/1468170

xjm’s picture

Issue tags: +Needs manual testing

Let's also make sure to test this patch manually and confirm that it has the intended result and that nothing else is affected. (Documenting specific steps to reproduce the issue in the summary will make it easier for others to test the patch.)

a.ross’s picture

Assigned: Unassigned » a.ross
Status: Needs work » Needs review

I'm on it

a.ross’s picture

Status: Needs review » Needs work

Oops crosspost

a.ross’s picture

a.ross’s picture

Issue summary: View changes

Added related issues

a.ross’s picture

Issue summary: View changes

Added steps to reproduce

a.ross’s picture

Issue summary: View changes

correction

fietserwin’s picture

If we have to add tests (to quick fixes) to test that something does not appear in the output, we will have to add another zillion tests. You simply can't test anything. A test that checks that there is no odd/even class is OK, but is not what this issue is about (it doesn't fail in the test only patch). So, IMO the assertion $this->assertNoRaw('no_striping', ... is ridiculous and should be removed.

a.ross’s picture

Hm, I don't have a strong opinion on that one... I can only refer to this: http://drupal.org/core-gates#testing
It says there that an explanation should be provided why a test isn't necessary, before marking RTBC. In any case, IMO more tests cannot be a bad thing.
Oh, and lets not stall this issue any longer, I want valid HTML :)

xjm’s picture

Thanks @a.ross and @fietserwin! I think I misunderstood the issue. Does the no_striping attribute still work before the patch, and simply just print the bogus attribute in addition?

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.phpundefined
@@ -66,4 +66,19 @@ function testThemeTableWithEmptyMessage() {
+    $this->assertNoRaw('no_striping', 'No invalid no_striping HTML attribute was printed.');

@fierstwin is probably correct inasmuch as this assertion is probably not needed. Let's keep the rest of the test, though, since it is missing coverage and it does demonstrate that @a.ross's fix does not cause a regression. I guess the only way to add test coverage for the bug if the even/odd classes are already properly omitted would be to validate the output markup, and that's way outside the scope of this issue. ;)

Also, despite the fact that it's not needed, that last assertion does demonstrate the bug is fixed. So let's remove that second assertion, and then I think this is RTBC.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

#24: The patch has been applied successfully more than once, but one can find that only in comments in the original issue [3930566] . I only realized that, after I had written my previous reaction. So it is easy to overlook that. BTW: the patch in this issue is not really different from the versions tested over there. So, for me, manual testing already has been done.

The error is indeed only about the fact that additional but incorrect markup is generated. The option it itself works (i.e: odd/even classes are not generated when this option is passed to theme_table).

Moreover, the "ridiculous" test may not be so ridiculous under the assumption that people are currently working on large impact features in separate branches and that, eventually, these branches get merged into head. Merge conflicts then get solved manually and the error may be reintroduced if this is not done very carefully. So adding an otherwise illogical test to prevent future (merge-)regressions may be not so bad after all.

So we leave the patch as is.

xjm’s picture

The patch has been applied successfully more than once

The patch is applied successfully every time it is uploaded, because testbot tests every patch, starting with applying it. What "manual testing" means is visiting a table using the no_striping option in the user interface, and inspecting it for regressions and changes, and usually documenting the before/after in a screenshot.

I actually don't at all understand how this issue is different from #935066: theme_table() should not output the no_striping option as an html attribute. Is it that this one is intended to offer a backportable solution, and the other is not? Usually we wouldn't do two separate issues for that. If the other issue's scope has changed, its title should be updated to indicate that it has a broader scope.

a.ross’s picture

Yeah, it was the intention to change the title and summary of that issue to indicate a broader scope, I haven't gotten around to it yet. But you can see some discussion going on there about an API change, see #6, #21, #22 and #27.

effulgentsia’s picture

I agree with the RTBC of this issue, including both test assertions. The second because that's the bug being fixed by this issue, and the first because why not add the missing test coverage.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed c7916f0 and pushed to 8.x. Thanks!

alexpott’s picture

Version: 8.x-dev » 7.x-dev

Fixing Drupal version

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.phpundefined
@@ -66,4 +66,19 @@ function testThemeTableWithEmptyMessage() {
+  /**
+   * Tests that the 'no_striping' option works correctly.
+   */
+  function testThemeTableWithNoStriping() {

Hm. I don't see any other table striping-related tests in core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php. It probably makes more sense therefore to broaden this test to "testTableStriping" (or similar) and include a couple of assertions the other way around as well.

We could do this in a follow-up also (and I'm fine with pushback on this suggestion), but since this is a backportable patch to D7, I'd prefer to just fix it here. Otherwise we have to commit this to D8, backport it to D7, which possibly requires more follow-ups to D8, and in the meantime we've started up a new issue that's going to directly conflict with this one to make the tests work both ways.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)
Issue tags: +Needs followup

Heh. Oh, well, needs follow-up it is. :)

a.ross’s picture

Thanks! Should we backport the patch as is or do you want me to broaden the test coverage for D7 as you mentioned?

webchick’s picture

Nah, let's backport as-is to keep it simple, methinks (unless David Rothstein disagrees). The test consolidation can be another issue.

a.ross’s picture

I just noticed that I didn't make the messages of the asserts translatable. Not sure how important that it, but here are the D7 patches.
(No translations in the D8 test framework? Or is translation handled by the framework itself?)

Also, make sure to credit markabur and fietserwin (see other issue)

Status: Needs review » Needs work
Issue tags: -Quick fix, -Needs backport to D7, -Needs followup

The last submitted patch, theme_table-no_striping-invalid-html-attribute_1959110-35.patch, failed testing.

a.ross’s picture

Status: Needs work » Needs review
a.ross’s picture

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

This patch (or actually some only very slightly different versions) has been applied, tested and reviewed in D7 by multiple persons. See #935066: theme_table() should not output the no_striping option as an html attribute. This latest version contains the necessary tests that shows it works. So RTBC for me.

David_Rothstein’s picture

Title: theme_table outputs the no_striping option as an HTML attribute. » theme_table outputs the no_striping option as an HTML attribute (followups)
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Active

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f746ed8

(And I removed the t() from the test assertion messages on commit.)

Yeah, I agree with @webchick; a test that does assertNoRaw() with no corresponding assertRaw() is pretty fragile and could use some improvement. I am not sure where that followup should happen, but leaving this issue open for now... It can be closed once an appropriate followup is filed.

David_Rothstein’s picture

Issue summary: View changes

Add more reproduction steps

NancyDru’s picture

I am still seeing this behavior in 7.38.

joelpittet’s picture

Version: 8.0.x-dev » 7.x-dev

This looks to be done already as the patch indicates in D8 already. Moving back to D7

  • alexpott committed c7916f0 on 8.3.x
    Issue #1959110 by a.ross: Fixed theme_table() outputs the no_striping()...

  • alexpott committed c7916f0 on 8.3.x
    Issue #1959110 by a.ross: Fixed theme_table() outputs the no_striping()...
NancyDru’s picture

Now, in 7.50, I am no longer seeing the attribute, but it is still zebra striping.

joelpittet’s picture

Could that be your theme doing that zebra striping @NancyDru?

NancyDru’s picture

Well, yes, because the rows are still classed even/odd.

a.ross’s picture

What exactly is the problem? Unless you explicitly tell theme_table() to *not* use zebra striping, it will add the odd/even classes. If you don't want those classes, either pass no_striping on every table row, or override the theme_table() function in your theme.

  • alexpott committed c7916f0 on 8.4.x
    Issue #1959110 by a.ross: Fixed theme_table() outputs the no_striping()...

  • alexpott committed c7916f0 on 8.4.x
    Issue #1959110 by a.ross: Fixed theme_table() outputs the no_striping()...
amit0212’s picture

Striping patch

Attached patch checks for no_striping before adding the key/value as an attribute.

a.ross’s picture

@amit, colgroups are not supposed to have any 'no_striping' attribute.