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; } } }
Comments
Comment #1
a.ross CreditAttribution: a.ross commentedAttaching patch (in less lines of code than the original!™) :)
Comment #2
fietserwinNot 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.Comment #4
a.ross CreditAttribution: a.ross commentedComment #5
markabur CreditAttribution: markabur commentedignore
Comment #6
a.ross CreditAttribution: a.ross commentedNot 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
Comment #7
marcingy CreditAttribution: marcingy commentedIssues are fixed in d8 and then backported so this is a dup of the other issue referenced.
Comment #8
a.ross CreditAttribution: a.ross commentedSee description...
Comment #9
marcingy CreditAttribution: marcingy commentedYes 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.
Comment #10
a.ross CreditAttribution: a.ross commented#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?
Comment #11
marcingy CreditAttribution: marcingy commentedWell 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.
Comment #12
a.ross CreditAttribution: a.ross commentedI 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.
Comment #13
a.ross CreditAttribution: a.ross commentedGah, 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.
Comment #14
fietserwinMarking this issue as the quick fix.
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).:
Of course, the other use of $no_striping need also to be refactored (and become more readable as well).
Comment #15
a.ross CreditAttribution: a.ross commentedI agree with that, but as the API already uses no_striping it's more consistent to use $no_striping IMO.
Comment #16
fietserwinIt'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.
Comment #16.0
fietserwinClarification.
Comment #17
xjmThanks @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
Comment #18
xjmLet'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.)
Comment #19
a.ross CreditAttribution: a.ross commentedI'm on it
Comment #20
a.ross CreditAttribution: a.ross commentedOops crosspost
Comment #21
a.ross CreditAttribution: a.ross commentedGo testbot
Comment #21.0
a.ross CreditAttribution: a.ross commentedAdded related issues
Comment #21.1
a.ross CreditAttribution: a.ross commentedAdded steps to reproduce
Comment #21.2
a.ross CreditAttribution: a.ross commentedcorrection
Comment #22
fietserwinIf 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.Comment #23
a.ross CreditAttribution: a.ross commentedHm, 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 :)
Comment #24
xjmThanks @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?@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.
Comment #25
fietserwin#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.
Comment #26
xjmThe 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.
Comment #27
a.ross CreditAttribution: a.ross commentedYeah, 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.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #29
alexpottCommitted c7916f0 and pushed to 8.x. Thanks!
Comment #30
alexpottFixing Drupal version
Comment #31
webchickHm. 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.
Comment #32
webchickHeh. Oh, well, needs follow-up it is. :)
Comment #33
a.ross CreditAttribution: a.ross commentedThanks! Should we backport the patch as is or do you want me to broaden the test coverage for D7 as you mentioned?
Comment #34
webchickNah, let's backport as-is to keep it simple, methinks (unless David Rothstein disagrees). The test consolidation can be another issue.
Comment #35
a.ross CreditAttribution: a.ross commentedI just noticed that I didn't make the messages of the asserts translatable. Not sure how important that it, buthere 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)
Comment #37
a.ross CreditAttribution: a.ross commented#35: theme_table-no_striping-invalid-html-attribute_1959110-35.patch queued for re-testing.
Comment #38
a.ross CreditAttribution: a.ross commented#35: theme_table-no_striping-invalid-html-attribute_1959110-35-tests.patch queued for re-testing.
Comment #39
fietserwinThis 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.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.
Comment #40.0
David_Rothstein CreditAttribution: David_Rothstein commentedAdd more reproduction steps
Comment #41
NancyDruI am still seeing this behavior in 7.38.
Comment #42
joelpittetThis looks to be done already as the patch indicates in D8 already. Moving back to D7
Comment #45
NancyDruNow, in 7.50, I am no longer seeing the attribute, but it is still zebra striping.
Comment #46
joelpittetCould that be your theme doing that zebra striping @NancyDru?
Comment #47
NancyDruWell, yes, because the rows are still classed even/odd.
Comment #48
a.ross CreditAttribution: a.ross commentedWhat 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 passno_striping
on every table row, or override thetheme_table()
function in your theme.Comment #51
amit0212 CreditAttribution: amit0212 as a volunteer and commentedStriping patch
Attached patch checks for no_striping before adding the key/value as an attribute.
Comment #52
a.ross CreditAttribution: a.ross commented@amit, colgroups are not supposed to have any
'no_striping'
attribute.