Description: ------------ I wish there was a getTable() method in Structures_DataGrid_Renderer_HTMLTable. IMO, the render() method should be changed to execute the rendering operation only, not to output HTML. Ususally, method that output html are called toHtml(), this is a de facto standard for PEAR classes. function &getTable() { if ($this->_rendered) { return $this->_table; } $this->_dg = &$dg; // Define Table Header if ($this->header) { $this->_buildHTMLTableHeader(); } // Build Table Data $this->_buildHTMLTableBody(); // Define Alternating Row attributes $this->_table->altRowAttributes(1, $this->evenRowAttributes, $this->oddRowAttributes, TRUE); $this->_rendered = true; return $this->_table; } function render(&$dg) { $table =& $this->getTable($dg); return $table->toHtml(); } And render() should be called toHtml(). Why that ? Because there are time when you want to modify the table structure yourself once it has been filled by SDG. For example to add a total row at the bottom of it.

This is now fixed in CVS. Please take a look and let me know if you agree with the changes. I changed the render method to print the HTML and added a toHTML method for returning the HTML string as well as a getTable method. Thanks for the help!

In my comments, I added a new instance variable called $_rendered set by default to false, this to make sure that the table is not rendered twice by successive calls to render() and the like. Don't you think it is useful, I didn't see it in your commits. Another thing, I think you should return the table by reference (add & in front of getTable()) so that the table returned is not a copy (better for memory and might be useful for other manipulations later). Don't you agree ?

I added the return by reference, thanks for catching that. I also added the rendered switch, I like the idea, I am just concerned that it could have a bad side effect. It's like caching, with no way to flush the cache. Maybe a setRendered method might be appropriate?