@anonymous-piwik-user opened this issue on January 28th 2010
The line "$this->rowsIndexByLabel[$label] = count($this->rows)-1;" in function

"public function addRow( Piwik_DataTable_Row $row )" doesn't work correctly. For example:

$rows ( 1=>"AA", 2=>"BB", 3=>"CC", 4=>"DD", 5=>"EE", 6=>"FF"); note:1=>"AA": "1" is the index id of label "AA"

delete $rows[3], we get:

$rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF");

addRow($newrow),the label of $newrow is "GG"

The result should be: $rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF", 7=>"GG");

while according to function addRow(), the index id of $newrow's label("GG") is

$this->rowsIndexByLabel[$label] = count($this->rows)-1 =5-1=4,

but the right index id of label "GG" is 7.

We find the function rebuildIndex() useless after we change the function addRow().

@anonymous-piwik-user commented on January 28th 2010

Attachment: newdatatablebug.txt

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The patch file for DataTable.php DataTable-diff.txt

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The file with change by jetuelle DataTable.php

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The diff file of DataTable.php DataTable.php.diff

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The patch file for DataTable.php DataTable.php.patch

@anonymous-piwik-user commented on January 28th 2010

The code change contains the following three points:

11original code(4 functions): public function addRow( Piwik_DataTable_Row $row ) { $this->rows[] = $row;

    if(!$this->indexNotUpToDate
        && $this->rebuildIndexContinuously)
    {
        $label = $row->getColumn('label');
        if($label !== false)
        {
            $this->rowsIndexByLabel[$label] = count($this->rows)-1;             
        }
        $this->indexNotUpToDate = false;
    }
}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2)
{
    $rows1 = $table1->getRows();
    $rows2 = $table2->getRows();

    $table1->rebuildIndex();
    $table2->rebuildIndex();

    if($table1->getRowsCount() != $table2->getRowsCount())
    {
        return false;
    }

    foreach($rows1 as $row1)
    {
        $row2 = $table2->getRowFromLabel($row1->getColumn('label'));
        if($row2 === false
            || !Piwik_DataTable_Row::isEqual($row1,$row2))
        {
            return false;
        }
    }       
    return true;
}

public function getRowFromLabel( $label )
{
    $this->rebuildIndexContinuously = true;
    if($this->indexNotUpToDate)
    {
        $this->rebuildIndex();
    }

    if($label === self::LABEL_SUMMARY_ROW
        && !is_null($this->summaryRow))
    {
        return $this->summaryRow;
    }

    $label = (string)$label;
    if(!isset($this->rowsIndexByLabel[$label]))
    {
        return false;
    }
    return $this->rows[$this->rowsIndexByLabel[$label]];
}

    public function sort( $functionCallback, $columnSortedBy )
{
    $this->indexNotUpToDate = true;
    $this->tableSortedBy = $columnSortedBy;
    usort( $this->rows, $functionCallback );

    if($this->enableRecursiveSort === true)
    {
        foreach($this->getRows() as $row)
        {
            if(($idSubtable = $row->getIdSubDataTable()) !== null)
            {
                $table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable);
                $table->enableRecursiveSort();
                $table->sort($functionCallback, $columnSortedBy);
            }
        }
    }
}


2changed code(4 functions):
public function addRow( Piwik_DataTable_Row $row )
{
    $this->rows[] = $row;

    $label = $row->getColumn('label');
    if($label !== false)
    {
        $this->rowsIndexByLabel[$label] = $this->nextRowId;
        $this->nextRowId++;
    }
}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2)
{
    $rows1 = $table1->getRows();
    $rows2 = $table2->getRows();


    if($table1->getRowsCount() != $table2->getRowsCount())
    {
        return false;
    }

    foreach($rows1 as $row1)
    {
        $row2 = $table2->getRowFromLabel($row1->getColumn('label'));
        if($row2 === false
            || !Piwik_DataTable_Row::isEqual($row1,$row2))
        {
            return false;
        }
    }       
    return true;
}

public function getRowFromLabel( $label )
{
    if($label === self::LABEL_SUMMARY_ROW
        && !is_null($this->summaryRow))
    {
        return $this->summaryRow;
    }

    $label = (string)$label;
    if(!isset($this->rowsIndexByLabel[$label]))
    {
        return false;
    }
    return $this->rows[$this->rowsIndexByLabel[$label]];
}

    public function sort( $functionCallback, $columnSortedBy )
{
    $this->tableSortedBy = $columnSortedBy;
    usort( $this->rows, $functionCallback );

    if($this->enableRecursiveSort === true)
    {
        foreach($this->getRows() as $row)
        {
            if(($idSubtable = $row->getIdSubDataTable()) !== null)
            {
                $table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable);
                $table->enableRecursiveSort();
                $table->sort($functionCallback, $columnSortedBy);
            }
        }
    }
}   

2added codes: protected $nextRowId = 0; 3deleted code: protected $indexNotUpToDate = true; protected $rebuildIndexContinuously = false; private function rebuildIndex() { foreach($this->rows as $id => $row) { $label = $row->getColumn('label'); if($label !== false) { $this->rowsIndexByLabel[$label] = $id; } } $this->indexNotUpToDate = false; }

@robocoder commented on January 28th 2010

Can you upload this as a patch? (use "svn diff")

@mattab commented on January 28th 2010

Also if you can please submit a unit test to show that there is a bug somewhere? the unit test for the dataTable is: https://github.com/piwik/piwik/blob/master/tests/core/DataTable.test.php

@mattab commented on February 11th 2010

jetuelle, please post a patch, your code is very hard to read in its current form.

rebuildIndex is not useless, it is helpful to access quickly rows with a given label instead of looping through the hundreds of rows in the datatable.

@mattab commented on March 17th 2010

Marking as invalid. Using delete $rows[3] is not supported, you should use $dataTable->deleteRow( $idRow ); instead.

If you wish to prove a bug, please provide a new unit test to https://github.com/piwik/piwik/blob/master/tests/core/DataTable.test.php that would revel the bug. thx

This issue was closed on March 19th 2010
Powered by GitHub Issue Mirror