Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in index the label of DataTable_Row in file DataTable.php #1128

Closed
anonymous-matomo-user opened this issue Jan 28, 2010 · 10 comments
Closed
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Milestone

Comments

@anonymous-matomo-user
Copy link

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-matomo-user
Copy link
Author

Attachment:
newdatatablebug.txt

@anonymous-matomo-user
Copy link
Author

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
Copy link
Contributor

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

@mattab
Copy link
Member

mattab commented Jan 28, 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
Copy link
Member

mattab commented Feb 11, 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.

@anonymous-matomo-user
Copy link
Author

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

@anonymous-matomo-user
Copy link
Author

Attachment: The file with change by jetuelle
DataTable.php

@anonymous-matomo-user
Copy link
Author

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

@anonymous-matomo-user
Copy link
Author

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

@mattab
Copy link
Member

mattab commented Mar 17, 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

@anonymous-matomo-user anonymous-matomo-user added this to the Piwik 0.5.5 milestone Jul 8, 2014
@mattab mattab added the wontfix label Aug 3, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Development

No branches or pull requests

3 participants