Home » Structures » Structures_DataGrid » Bug #7848

[2006-06-09 11:48 UTC] michel dot dhooge at gmail dot com

Description:
------------
I started to study the new Datagrid and did some
corrections as I read the source. Note that most are minor
corrections. Those I am not sure are commented below:

In DataGrid/Renderer.php, I thought it would be better to
sort the properties (and also added missing _multiSort).
@+463, maybe I misunderstood something about container.

Question in DataGrid/Renderer/HTMLSortForm.php @+140.

Test script:
---------------
Index: DataGrid.php
===================================================================
RCS file: /repository/pear/Structures_DataGrid/DataGrid.php,v
retrieving revision 1.72
diff -u -r1.72 DataGrid.php
--- DataGrid.php 31 May 2006 11:25:00 -0000 1.72
+++ DataGrid.php 9 Jun 2006 08:42:20 -0000
@@ -489,6 +489,7 @@
*
* @param string $renderer The defined renderer string
* @param array $options Rendering options
+ * @return mixed True or PEAR_Error
* @access public
*/
function setRenderer($type, $options = array())
@@ -1041,7 +1042,7 @@
if (is_array ($sortSpec)) {
$this->sortSpec = $sortSpec;
} else {
- $this->sortSpec = array($sortBy => $direction);
+ $this->sortSpec = array($sortSpec => $direction);
}

if (isset($this->_dataSource)) {
Index: package.sh
===================================================================
RCS file: /repository/pear/Structures_DataGrid/package.sh,v
retrieving revision 1.2
diff -u -r1.2 package.sh
--- package.sh 31 May 2006 22:43:23 -0000 1.2
+++ package.sh 9 Jun 2006 08:42:20 -0000
@@ -4,7 +4,7 @@
pear package package2-ds-array.xml
pear package package2-ds-csv.xml
pear package package2-ds-db.xml
-pear package package2-ds-dbdataobject.xml
+pear package package2-ds-dataobject.xml
pear package package2-ds-dbquery.xml
pear package package2-ds-dbtable.xml
pear package package2-ds-mdb2.xml
Index: DataGrid/Renderer.php
===================================================================
RCS file: /repository/pear/Structures_DataGrid/DataGrid/Renderer.php,v
retrieving revision 1.47
diff -u -r1.47 Renderer.php
--- DataGrid/Renderer.php 28 May 2006 22:29:48 -0000 1.47
+++ DataGrid/Renderer.php 9 Jun 2006 08:42:20 -0000
@@ -40,7 +40,7 @@
* - defaultColumnValues: (array) Per-column default cell value. This is an array
* of the form: array(fieldName => value, ...).
* (default: array())
- * - hideColumnLinks: (bool) By default sorting links are enabled on all
+ * - hideColumnLinks: (array) By default sorting links are enabled on all
* columns. With this option it is possible to
* disable sorting links on specific columns. This
* is an array of the form: array(fieldName, ...).
@@ -77,24 +77,27 @@
*
* Properties (all read-only):
* - $_columns
- * - $_records
* - $_columnsNum
- * - $_recordsNum
- * - $_firstRecord;
- * - $_lastRecord;
- * - $_totalRecordsNum
* - $_currentSort
+ * - $_firstRecord
+ * - $_lastRecord
+ * - $_multiSort << Added
+ * - $_options
* - $_page
* - $_pageLimit
* - $_pagesNum
+ * - $_records
+ * - $_recordsNum
* - $_requestPrefix
* - $_sortableFields
- * - $_options
+ * - $_totalRecordsNum
*
* Options that drivers may handle:
* - encoding
* - fillWithEmptyRows
* - numberAlign
+ * - extraVars
+ * - excludeVars
*
* @version $Revision: 1.47 $
* @author Olivier Guilyardi <olivier@samalyse.com>
@@ -145,17 +148,20 @@
var $_currentSort = array();

/**
- * Wether the backend support sorting by multiple fields
+ * Whether the backend support sorting by multiple fields
*
* Drivers can read the content of this property but must not change it.
*
* @var bool
* @access protected
*/
- var $_multiSort = array();
+ var $_multiSort = false;

/**
* Number of columns
+ *
+ * Drivers can read the content of this property but must not change it.
+ *
* @var int
* @access protected
*/
@@ -183,6 +189,9 @@

/**
* First record number (starting from 1), in the current page
+ *
+ * Drivers can read the content of this property but must not change it.
+ *
* @var int
* @access protected
*/
@@ -190,6 +199,9 @@

/**
* Last record number (starting from 1), in the current page
+ *
+ * Drivers can read the content of this property but must not change it.
+ *
* @var int
* @access protected
*/
@@ -219,6 +231,9 @@

/**
* Number of pages
+ *
+ * Drivers can read the content of this property but must not change it.
+ *
* @var int
* @access protected
*/
@@ -378,12 +393,13 @@
/**
* Specify how the datagrid is currently sorted
*
- * @var array
* @param array $spec Structure:
* array(fieldName => direction, ....)
- * @param bool $multiSortCapable Wether the backend support sorting by
+ * @param bool $multiSortCapable Whether the backend support sorting by
* multiple fields
* @access public
+ *
+ * >>> Who should call this function and decide for the multiSortCapabilities?
*/
function setCurrentSorting($spec, $multiSortCapable = false)
{
@@ -447,11 +463,13 @@
/**
* Create or/and prepare the container
*
- * Drivers may optionally implement this method.
- *
- * This method is responsible for creating the container if it has not
- * already been provided by the user with the setContainer() method.
- * It is where preliminary container setup should also be done.
+ * Drivers may optionally implement this method for any pre-build()
+ * operations.
+ *
+ * For the container support, it is responsible for creating the
+ * container if it has not already been provided by the user with
+ * the setContainer() method. It is where preliminary container
+ * setup should also be done.
*
* @abstract
* @access protected
@@ -550,7 +568,8 @@
/**
* Finish building the datagrid.
*
- * Drivers may optionally implement this method.
+ * Drivers may optionally implement this method for any post-build()
+ * operations.
*
* @abstract
* @access protected
@@ -708,7 +727,6 @@
* writing to the standard output (like calling header(), etc...).
*
* @access public
- * @return mixed True or a PEAR_Error
*/
function render()
{
Index: DataGrid/Renderer/HTMLSortForm.php
===================================================================
RCS file: /repository/pear/Structures_DataGrid/DataGrid/Renderer/HTMLSortForm.php,v
retrieving revision 1.2
diff -u -r1.2 HTMLSortForm.php
--- DataGrid/Renderer/HTMLSortForm.php 31 May 2006 17:42:42 -0000 1.2
+++ DataGrid/Renderer/HTMLSortForm.php 9 Jun 2006 08:42:20 -0000
@@ -72,7 +72,7 @@
var $_form;

/**
- * Wether the container was provided by the user
+ * Whether the container was provided by the user
* @var bool
* @access protected
*/
@@ -140,6 +140,7 @@
"{$this->_requestPrefix}DataGridSortForm", 'get');
$this->_isUserContainer = false;
} else {
+ // Isn't it a bit risky to set this flag here, because this method could be called more than once?
$this->_isUserContainer = true;
}
}
Index: DataGrid/Renderer/HTMLTable.php
===================================================================
RCS file: /repository/pear/Structures_DataGrid/DataGrid/Renderer/HTMLTable.php,v
retrieving revision 1.105
diff -u -r1.105 HTMLTable.php
--- DataGrid/Renderer/HTMLTable.php 27 Apr 2006 11:21:27 -0000 1.105
+++ DataGrid/Renderer/HTMLTable.php 9 Jun 2006 08:42:20 -0000
@@ -28,7 +28,7 @@
*
* Driver for rendering the DataGrid as an HTMLTable
*
- * SUPPORTED OPTIONS:
+ * SUPPORTED OPTIONS (Driver Specific):
*
* - evenRowAttributes: (array) An associative array containing each attribute
* of the even rows.
@@ -253,12 +253,7 @@
*/
function allowEmptyRows($value, $attributes = array())
{
- if ($value) {
- $this->_options['fillWithEmptyRows'] = true;
- } else {
- $this->_options['fillWithEmptyRows'] = false;
- }
-
+ $this->_options['fillWithEmptyRows'] = (bool)$value;
$this->_options['emptyRowAttributes'] = $attributes;
}

@@ -310,8 +305,10 @@
/**
* Generates the HTML for the DataGrid
*
+ * @deprecated Use getOutput() instead.
* @access public
* @return string The HTML of the DataGrid
+ * @see Structures_DataGrid_Renderer::getOutput
*/
function toHTML()
{
@@ -321,8 +318,7 @@
/**
* Gets the HTML_Table object for the DataGrid
*
- * OBSOLETE
- *
+ * @deprecated Use getContainer() instead.
* @access public
* @return object HTML_Table The HTML Table object for the DataGrid
*/

[2006-06-09 21:45 UTC] olivierg at php dot net

The best would be to put a link here to your patch if possible.

However, I've been reading all the above and I must say that you have pretty well studied the DataGrid :-) I'm honored of your attention.

You're right about the $_isUserContainer property in Renderer_HTMLSortForm that might get wrong if init() is called twice.

That would happen for example with two calls to render() like in :
$datagrid->render(DATAGRID_RENDER_SORTFORM);
$datagrid->render(DATAGRID_RENDER_SORTFORM);

I suppose nobody would do that ;-) Nevertheless, at the second call, the form might contain duplicate fields, and no submit button.

The best could be to set the $_isUserContainer property in setContainer()

Regarding your patch, I would like to reread it in my preferred editor, but I think it's 99% good. You seem to have rather well understood the container concept.

I changed the "Diff patch" summary of this request, because it was meaningless. Please provide more informational summaries in the future.

[2006-06-11 12:02 UTC] olivierg at php dot net

Your patch has been applied with a few minor changes :

http://news.php.net/php.pear.cvs/40615

I like your improvements to the Renderer::init() docblock.

Regarding your included-in-the-patch question about "Who should call setCurrentSorting() and decide for the multiSortCapabilities":

setCurrentSorting(), together with setData() and setLimit() are supposed to be called by the code that loads the driver. In our case that's the Structures_DataGrid class.

The multiSort capabilities is related to the multiSort DataSource feature. In short : the DataGrid checks if the DataSource supports multiSort and informs the Renderer about it.

I'm not sure what to do with the $_isUserContainer property, and where to set it. Could any of you review/develop this point ? A test script, showing what the problem really is, would be welcome...

[2006-06-12 08:40 UTC] michel dot dhooge at gmail dot com

@Mark: Sorry I wasn't connected this week-end!

Summary: I knew it was really poor but couldn't think of something good :-(

> You're right about the $_isUserContainer property in
> Renderer_HTMLSortForm that might get wrong if init()
> is called twice.
My concern (and this is also why I asked on the PEAR ML) is that init is quite generic and AFAIK there is no clear definition on when it will be called and when it won't. So even though this works right now, it could break in the future.

[2006-06-13 09:47 UTC] michel dot dhooge at gmail dot com

Additional comments and patches:
- modified DataGrid::render() to also accept a Renderer
instance,
- variable $test -> $result

Index: DataGrid.php
===================================================================
RCS
file: /repository/pear/Structures_DataGrid/DataGrid.php,v
retrieving revision 1.74
diff -u -r1.74 DataGrid.php
--- DataGrid.php 11 Jun 2006 21:46:58 -0000
1.74
+++ DataGrid.php 13 Jun 2006 09:29:49 -0000
@@ -383,24 +383,30 @@
*
* You can call this method several times with
different renderers.
*
- * @param int $type Renderer type (optional)
+ * @param mixed $renderer Renderer type (optional)
+ * @param array $options An associative array of
the form:
+ * array(optionName =>
optionValue, ...)
* @access public
* @return mixed True or PEAR_Error
*/
- function render($type = null,$options = array())
+ function render($renderer = null,$options = array())
{
- if (!is_null($type)) {
+ if (!is_null($renderer)) {
$this->_saveRenderer();

- $test = $this->setRenderer($type);
- if (PEAR::isError($test)) {
+ if
(is_a($renderer, 'Structures_DataGrid_Renderer')) {
+ $result =
$this->attachRenderer($renderer);
+ } else {
+ $result = $this->setRenderer($renderer);
+ }
+ if (PEAR::isError($result)) {
$this->_restoreRenderer();
- return $test;
+ return $result;
}
} else if (!isset($this->_renderer)) {
- $test =
$this->setRenderer(DATAGRID_RENDER_DEFAULT);
- if (PEAR::isError($test)) {
- return $test;
+ $result =
$this->setRenderer(DATAGRID_RENDER_DEFAULT);
+ if (PEAR::isError($result)) {
+ return $result;
}
}

@@ -409,10 +415,10 @@
}

$this->_renderer->isBuilt() || $this->build();
- $test = $this->_renderer->render();
+ $result = $this->_renderer->render();

- if (PEAR::isError($test)) {
- if ($test->getCode() ==
DATAGRID_ERROR_UNSUPPORTED) {
+ if (PEAR::isError($result)) {
+ if ($result->getCode() ==
DATAGRID_ERROR_UNSUPPORTED) {
$type = is_null($this->_rendererType)
? get_class($this->_renderer)
: $this->_rendererType;
@@ -421,10 +427,11 @@
"render() method.
Try using fill().");
} else {
$this->_restoreRenderer();
- return $test;
+ return $result;
}
}
$this->_restoreRenderer();
+ return true;
}

/**
@@ -485,21 +492,24 @@
/**
* Set Renderer
*
- * Defines which renderer to be used by the DataGrid
+ * Creates a new renderer to be used by the DataGrid
based on given
+ * $type and $options. To attach an existing renderer
instance, use
+ * attachRenderer instead.
*
- * @param string $renderer The defined
renderer string
+ * @param string $type The defined
renderer string
* @param array $options Rendering options
- * @return mixed True or PEAR_Error
+ * @return mixed Renderer instance or PEAR_Error
* @access public
+ * @see Structures_DataGrid::attachRenderer
*/
- function setRenderer($type, $options = array())
+ function & setRenderer($type, $options = array())
{
$renderer =& $this->rendererFactory($type,
$options);
- if (!PEAR::isError($renderer)) {
+ if (PEAR::isError($renderer)) {
+ return $renderer;
+ } else {
$this->_rendererType = $type;
return $this->attachRenderer($renderer);
- } else {
- return $renderer;
}
}

@@ -512,6 +522,10 @@
function _saveRenderer()
{
if (isset($this->_renderer)) {
+ // The following line is a workaround for PHP
bug 32660
+ // See: http://bugs.php.net/bug.php?id=32660
+ // Another solution would be to remove __get
which is used only for BC
+ $this->_rendererBackup = 1;
$this->_rendererBackup =& $this->_renderer;
$this->_rendererTypeBackup =
$this->_rendererType;

@@ -577,14 +591,15 @@
}

/**
- * Attach a rendering driver
+ * Attach an existing rendering driver
*
* @param object $renderer Driver object, subclassing
*
Structures_DataGrid_Renderer
- * @return mixed Either true or a
PEAR_Error object
+ * @return mixed Renderer instance or PEAR_Error
* @access public
+ * @see Structures_DataGrid::setRenderer
*/
- function attachRenderer(&$renderer)
+ function & attachRenderer(&$renderer)
{
if
(is_subclass_of($renderer, 'structures_datagrid_renderer'))
{
// The following line is a workaround for PHP
bug 32660
@@ -607,7 +622,7 @@
'must be a valid
renderer driver class');
}

- return true;
+ return $renderer;
}


@@ -962,6 +977,7 @@
$this->_renderer->setLimit($this->page,
$this->rowLimit,

$this->getRecordCount());
if ($this->sortSpec) {
+ // According to comment, test isn't
needed ???
$this->_setRendererCurrentSorting();
}
}
Index: DataGrid/Renderer.php
===================================================================
RCS
file: /repository/pear/Structures_DataGrid/DataGrid/Renderer.php,v
retrieving revision 1.48
diff -u -r1.48 Renderer.php
--- DataGrid/Renderer.php 11 Jun 2006 11:41:48 -0000
1.48
+++ DataGrid/Renderer.php 13 Jun 2006 09:29:49 -0000
@@ -73,7 +73,6 @@
* - finalize()
* - flatten()
* - render()
- * - getPaging()
*
* Properties (all read-only):
* - $_columns
@@ -380,6 +379,9 @@
/**
* Provide columns and records data
*
+ * This method is supposed to be called ONLY by the
code that loads the
+ * driver. In most cases, that'll be the
Structures_DataGrid class.
+ *
* @param array $columns Array of
Structures_DataGrid_Column objects
* @param array $records 2D array of records values
* @access public
@@ -392,22 +394,32 @@

/**
* Specify how the datagrid is currently sorted
- *
- * @param array $spec Structure:
+ *
+ * This method is supposed to be called ONLY by the
code that loads the
+ * driver. In most cases, that'll be the
Structures_DataGrid class.
+ *
+ * The multiSort capabilities is related to the
multiSort DataSource
+ * feature. In short : the DataGrid checks if the
DataSource supports
+ * multiSort and informs the Renderer about it.
+ *
+ * @param array $currentSort Structure:
* array(fieldName
=> direction, ....)
* @param bool $multiSortCapable Whether the
backend support sorting by
* multiple fields
* @access public
*/
- function setCurrentSorting($spec, $multiSortCapable =
false)
+ function setCurrentSorting($currentSort,
$multiSortCapable = false)
{
- $this->_currentSort = $spec;
+ $this->_currentSort = $currentSort;
$this->_multiSort = $multiSortCapable;
}

/**
* Specify page and row limits
*
+ * This method is supposed to be called ONLY by the
code that loads the
+ * driver. In most cases, that'll be the
Structures_DataGrid class.
+ *
* @param int $currentPage Current page number
* @param int $rowsPerPage Maximum number of rows per
page
* @param int $totalRowNum Total number of data rows
Index: DataGrid/Renderer/HTMLSortForm.php
===================================================================
RCS
file: /repository/pear/Structures_DataGrid/DataGrid/Renderer/HTMLSortForm.php,v
retrieving revision 1.3
diff -u -r1.3 HTMLSortForm.php
--- DataGrid/Renderer/HTMLSortForm.php 11 Jun 2006
11:41:48 -0000 1.3
+++ DataGrid/Renderer/HTMLSortForm.php 13 Jun 2006
09:29:49 -0000
@@ -215,10 +215,13 @@
}
$this->_form->setConstants($values);

- // Only add a submit button if the QF
container wasn't provided
- // by the user
+ // Only add a submit button and extraVars if
the QF container wasn't
+ // provided by the user
if (!$this->_isUserContainer) {
$this->_form->addElement('submit', null,
$this->_options['textSubmit']);
+ foreach($this->_options['extraVars'] as
$var => $value) {
+ $this->_form->addElement('hidden',
$var, $value);
+ }
}
}
}
Index: DataGrid/Renderer/HTMLTable.php
===================================================================
RCS
file: /repository/pear/Structures_DataGrid/DataGrid/Renderer/HTMLTable.php,v
retrieving revision 1.106
diff -u -r1.106 HTMLTable.php
--- DataGrid/Renderer/HTMLTable.php 11 Jun 2006
11:41:48 -0000 1.106
+++ DataGrid/Renderer/HTMLTable.php 13 Jun 2006
09:29:49 -0000
@@ -490,6 +490,7 @@
* delta: The number of pages to display before
and
* after the current page
*
+ * @deprecated Use Structures_DataGrid_Renderer_Pager
instead?
* @access public
* @param array $options Array of
HTML::Pager options
* @return string The HTML for the
page links

[2006-06-13 13:14 UTC] olivierg at php dot net

Thanks for your Patch Michel. However there's a methodology problem here.

1 - Please provide links to your your patches. This bug has got unreadable.

2 - A bug or feature request must try to target a specific issue. To avoid confusion, it's better to divide problems into small parts, as Descartes said.

You can not mix everything like you currently do : we were talking about the lack of definition of init()'s role, which I recognize to be fuzzy, and you submit a patch that addresses a dozen of other issues... I'm getting lost here.

Don't get me wrong, your patch is very welcome and seem to contain good things.

But please open several bugs/requests, each with a specific and precise summary.

[2006-08-17 13:59 UTC] michel dot dhooge at gmail dot com

To conclude that painful bug report, I still have the
following comments in my workspace that weren't included.
If you don't want them, I'll remove them to stop
triggering my CVS update...

diff -u -r1.105 DataGrid.php
--- DataGrid.php 15 Aug 2006 10:35:49 -0000
1.105
+++ DataGrid.php 17 Aug 2006 13:54:10 -0000
@@ -448,7 +448,7 @@
*
* You can call this method several times with
different renderers.
*
- * @param int $renderer Renderer type (optional)
+ * @param mixed $renderer Renderer type or instance
(optional)
* @param array $options An associative array of
the form:
* array(optionName =>
optionValue, ...)
* @access public
@@ -586,6 +586,7 @@
* @param array $options Rendering options
* @return mixed Renderer instance or PEAR_Error
* @access public
+ * @see Structures_DataGrid::attachRenderer
*/
function &setRenderer($type, $options = array())
{