r85922 MediaWiki - Code Review archive

Commit summary? What is this?

Sorry for the comment less commit.

Implement tbody, thead & tfoot. Fixes Bug 4740 Original patch by bluehairedlawyer@gmail.com, rewritten at some places by me. ParserTests are changed to accommodate the new elements, but no parserTest `logic` is changed

Strict Standards: Only variables should be passed by reference in D:\www\MW_trunk\phase3\includes\parser\Parser.php on line 880

Please use E_ALL | E_STRICT for PHP errors.

Oh, and welcome to SVN committer land :)

Thanks! Strict fixed in r85941

Could you please create a page with a few examples of the new possibilities at Translatewiki?

It doesn't add any new syntax, but simply renders stuff like http://translatewiki.net/wiki/TablesSandbox semantically correct. (Headers are actually theads, etc.) This makes it possible to switch to a better tablesorting script in the future.

Thanks for the example. I was confused by the proposal for new syntax of the bug reporter.

Welcome to commit land!

I have some advise for you, mostly related to code style. See also Manual:Coding conventions.

You should follow code conventions for variables. Thus $lastSection instead of $last_section. Your variables should be more descriptive. Use $tableDescriptor rather than $t.

Check your whitespace use: array_pop( $a ) instead of array_pop($a). You can use stylize.php to enfore this style.

Function names should really indicate what it does: a function called printTableHtml is expected to print/echo something to the output. It does not do that, it makes a string containing the table html. It should thus be named build/create/makeTableHtml; choose your favorite.

Personally I don't really like mixed key arrays, where numeric keys indicate data elements, and string keys indicate attributes/options. I'd rather use a nested array solution, like array( 'attributes' => array( ... ), 'caption => '...', 'data' => array( $row1, $row2, ... ) ). Then you can use foreach on $t['data'] which is much cleaner than for ( $i = 0; isset( $t[i] ); $i++ ).

After these changes, tables are behaving incorrectly.

For instance, if you have a header in a table cell, it won't render correctly if it is at the beginning of the cell.

e.g.

| 

=== Foo ===

Will not render correctly, but

|

----

=== Foo ===

will.

In addition, tables which appear further down on pages seems to have a margin-top: 50% behaviour. This is probably overwriteable with CSS, but it seems to be an odd default behaviour.

Some examples:

• http://theinfosphere.org/Broadcast_season_8#Season_8
• http://theinfosphere.org/Main_Page (notice the lack of header rendering for some of the headers in the table)

I think that's due to the trim(). As the goal of this revision was to add thead/tfoot, I think it could be stripped back.

This is fixed r86039

-			if ( $line ===  ) { # empty line, go to next line
+			if ( $line == ) { //empty line, go to next line

Why are you changing the strict equality to a weak one? Probably introduces some bug.

Oh, it was fixed in r85943

You're changing the tag history to using a stack. I'd appreciate some docs on the way it is done. It's probably ok, but would be easier to review it with an explanations.

Also, the new way looks more memory hungry.

Did you run the parserTests after your changes? You also broke other 3 parserTests:

• Fuzz testing: Parser14-table...
• Fuzz testing: Parser22...
• Bug 529: Uncovered table already at line-start...

I don't think it's important that there's no longer a table in those Fuzz testing tests, but in such case I'd expect the {| to appear as a literal, not to magically disappear.

The change to 'Uncovered table already at line-start' is wrong.

But didn't it practically disappear previously as well (besides leaving an invisible table) ? Parser test for Bug 529 works again in r86039. And thanks for the followups!

[15-Apr-2011 09:05:06] PHP Notice:  Undefined index: type in /www/w/includes/parser/Parser.php on line 1056
[15-Apr-2011 09:05:06] PHP Notice:  Undefined index: type in /www/w/includes/parser/Parser.php on line 1070
[15-Apr-2011 09:05:06] PHP Notice:  Undefined index: type in /www/w/includes/parser/Parser.php on line 1078
[15-Apr-2011 09:05:06] PHP Notice:  Undefined index: type in /www/w/includes/parser/Parser.php on line 1086
[15-Apr-2011 09:05:06] PHP Notice:  Undefined index: type in /www/w/includes/parser/Parser.php on line 1094

Do you have the offending code?

(Clarify: The wikitext that triggered it)

Of course not, since debugging PHP is such a pain. However, if it helps you I can try to match the error timestamps into requests and find the code that way.

I think that would be helpful. I'd guess it's something like an empty row or cell, but I can't seem to find something that triggers it

http://translatewiki.net/sw/User:Ahloubadar

Still happening: [22-Apr-2011 09:02:57] PHP Notice: Undefined index: type in /www/w/includes/parser/Parser.php on line 1091

Source code at http://translatewiki.net/sandwiki/i.php?title=Translating:Group_statistics/Header7&action=edit

Sigh, it's been almost two weeks now.

Revert?

Doesn't look worth it given the number of follow-ups. I imagine the fix is easy for someone who has wrapped their head around the code.

It was, just forgot to commit it :(

Was this all resolved? If not, we should back it out of REL1_18.

I'd say it's resolved. (At least I know nothing that's still broken)

+1, no errors from twn. I'd expect that Wikipedia finds some but it's impossible to say without testing.

Assuming this is resolved.

After running many versions of Parser, I can confirm that this causes bug 30746.

This version of doTableStuff() takes 20 times longer and uses 25 times as much memory as the old code for this test case: "{|\n" . str_repeat( "|-\n| x\n", 10000) . "|}\n"

I'm backing this out for 1.18.

Constructing a structured representation of the input text and then transforming it to HTML as a second step is an approach which has a very high memory overhead, and a moderately high CPU overhead. It should only be used as a last resort, where on-the-fly generation of output text is not feasible. It does appear to be feasible in this case -- in fact the main desired feature, adding of <thead> elements, should be possible with just a few extra lines in the old doTableStuff().

I'm also concerned about the number of unintended changes in the input syntax and output. Many were fixed, but at least bug 30746 remains. The lack of attention to backwards compatibility implied by these changes indicates that deployment may uncover further issues which affect our large existing text corpus.

A rewrite is fine in principle, since the code is old and ugly. But the behaviour should be made as close as possible to the old code, to enable differential fuzz testing to be done, and the overall design should take performance into account.