Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 0.9.3

Request #8320 new methode removeColumn() for existing addColumn()
Submitted: 2006-07-30 01:06 UTC
From: arcadius at menelic dot com Assigned: olivierg
Status: Closed Package: Structures_DataGrid (version 0.7.1)
PHP Version: 5.1.3 OS: Windows
Roadmaps: (Not assigned)    
Subscription  


 [2006-07-30 01:06 UTC] arcadius at menelic dot com (Arcadius)
Description: ------------ As there is a method addColumn() in the class, I was in need of removeColumn(), so I added it. The code is in the "Test Script" section below. Test script: --------------- /** * Remove a column * * @access public * @param object $column The Structures_DataGrid_Column object to remove * This can also be an array of Structures_DataGrid_Column * @return void */ function removeColumn($col) { $tmp = Array(); $isArr = is_array($col); while(list ($k, $v) = each($this->columnSet)){ if(($isArr && in_array($v, $col)) || (!$isArr && $v === $col)) continue; $tmp[] = $v ; } $this->columnSet = $tmp; } Expected result: ---------------- The column removed won't be displayed. Note that previously values loaded from the grid via methods such as $dg->getColumnByField() before the removal of the column are still available. That's one can bind data to the grid, then load some needed columns to use for hidden field or so, then remove the column from the grid. Actual result: -------------- the method did not exist.

Comments

 [2006-07-30 11:15 UTC] olivierg at php dot net (Olivier Guilyardi)
Good idea. However your code is obviously going to clone Column objects in PHP4, breaking references... This is likely to break a few applications out there.
 [2006-07-30 11:46 UTC] arcadius at menelic dot com
The method I suggested previously has a little bug. Before itereting, I have to first call "reset($this->columnSet);" So, the fixed code is: /** * Remove a column * * @access public * @param object $column The Structures_DataGrid_Column object to remove * This can also be an array of Structures_DataGrid_Column * @return void */ function removeColumn($col) { $tmp = Array(); $isArr = is_array($col); reset($this->columnSet); while(list ($k, $v) = each($this->columnSet)){ if(($isArr && in_array($v, $col)) || (!$isArr && ($v == $col))) continue; $tmp[] = $v ; } $this->columnSet = $tmp; }
 [2006-07-30 12:16 UTC] olivierg at php dot net (Olivier Guilyardi)
Arcadius, can you please read my previous comment again? In PHP4, the following statement is going to clone every Column object: $tmp[] = $v Then this is going to clone all of the Column set again: $this->columnSet = $tmp; Consequences : 1 - It is going to break any references in external code 2 - It is going to use 4 times more memory for nothing Additionally, with foreach() instead of while(), you don't need to reset().
 [2006-07-30 12:18 UTC] olivierg at php dot net (Olivier Guilyardi)
Btw, I recommend you use a for() loop and compare everything in place.
 [2006-07-30 15:59 UTC] arcadius at menelic dot com
Hi Olivier. I did read you comment. And I understand the ref problem you talked about. forEach()/for is not a problem for me. The issue is how to <b>reindex the array</b> after deleting the colum(s). ctually, I'm doing: <pre> function removeColumn($col) { $isArr = is_array($col); foreach($this->columnSet as $k =>$v ){ if(($isArr && in_array($v, $col)) || (!$isArr && ($v == $col))) unset($this->columnSet[$k]); } $this->columnSet = array_values($this->columnSet); } </pre> This is still certainly loosing the ref, but works on my PHP5. When I remove the line $this->columnSet = array_values($this->columnSet); which does the copy/reindexing, I get an err/NOTICE: <pre> Notice: Undefined offset: 1 in pear\Structures\DataGrid\Renderer\HTMLTable.php on line 403 </pre> Any idea about how to reindex the array without copying it? Thanks.
 [2006-07-30 16:39 UTC] arcadius at menelic dot com
Well, I should be able reindex it manually: write a small method for reindexing $this->columnSet without copying it. But again, while reindexing $this->columnSet, I guess I'll have to copy each individual columns. Is there any better option? Thanks. Arcadius.
 [2006-08-03 20:37 UTC] olivierg at php dot net (Olivier Guilyardi)
Okay, I added dropColumn() into the CVS. As addColumn() it only accepts a single Column object, no array of columns. This bug is closed. Thanks for your report.
 [2006-08-03 22:49 UTC] arcadius at menelic dot com
Hello again Olivier! I have tested the new method on PHP 5.0.3, Win32. Unfortunately, there is a little problem. When using your method dropColumn($myCol) I get: <pre> Notice: Undefined offset: 4 in pear\Structures\DataGrid\Renderer\HTMLTable.php on line 403 </pre> I don't have a debugger, but my guess is that the call of unset($this->columnSet[$ii - 1]); destroys not only the value, but also the ref of $this->columnSet[$ii - 1] to which $this->columnSet[$ii - 2] is still referencing too (that's my guess) I have replaced unset($this->columnSet[$ii - 1]); by array_pop($this->columnSet); and it works fine. (according to the warning at http://www.phpdig.net/ref/rn03re25.html , the array_pop() method alters directly the passed param, which is fine in our case) Again, thanks! Arcadius A.
 [2006-08-04 10:14 UTC] olivierg at php dot net (Olivier Guilyardi)
Mark, can you confirm this on Windows? I don't have any problem here with PHP 5.1.4 on Linux.
 [2006-08-04 10:22 UTC] wiesemann (Mark Wiesemann)
> Mark, can you confirm this on Windows? I was just trying. ;-) The answer is: No. (with PHP 5.1.2) Arcadius, can you please show us the test code that produces the PHP notice on your system? BTW: I like the name removeColumn() better than dropColumn(), "remove" sounds a little bit more polite.
 [2006-08-04 22:51 UTC] arcadius at menelic dot com
Hi! I was doing something wrong: <pre> ini_set("error_reporting",E_ALL); ... $column_user_id = $dg->getColumnByField('user_id'); $dg->dropColumn($column_user_id); $action_column = new Structures_DataGrid_Column('Action', null, null, null, null, 'printCheckbox', $column_user_id ); $dg->addColumn($action_column); $dg->render(); </pre> So, after I called $dg->dropColumn($column_user_id), the method printCheckbox() was still using the variable $column_user_id which was the problem. Now, I call $dg->dropColumn($column_user_id) at the last moment, just before $dg->render(), and everything work well. Note that when array_pop() is used to implement the dropColumn() method, $dg->dropColumn($column_user_id) can be called everywhere while $column_user_id is still available for use. Thanks. Arcadius.
 [2006-08-07 09:28 UTC] olivierg at php dot net (Olivier Guilyardi)
Mark, I've renamed dropColumn() to removeColumn() as you propose. Actually, it's also conceptually right, because it does not drop (destroy) the column. It only removes it from the column set. The column is still available for reuse. Arcadius: I don't have any problem like the one you mention, neither under PHP4 nor PHP5. I can still use the Column object after I've removed it. Can you please show me your printCheckBox() function? PS: Mark, I don't set the status to Feedback because of bug #8082
 [2006-08-08 04:43 UTC] arcadius at menelic dot com
my printCheckBox() is below. Both params are columns from a datagrid which are later droped I'm not sure if this code can help. During the weekend, I'll post here the simplest test case that can reproduce the problem I have been describing. <pre> function printCheckbox($the_user_param, $the_role_param) { global $browsingInThePast; extract($the_user_param); $current_row_user_id = $record['user_id']; $tmp = is_array($the_role_param) ? $the_role_param : Array (); extract($tmp); $current_row_role_id = $record['role_id']; $chkBox = '<input type="checkbox" name="idList[]" '.($browsingInThePast ? "disabled" : "").' value="'.$current_row_role_id.'">'; if (is_numeric($current_row_user_id)) { if ($_SESSION['currentUser']->user_id == $current_row_user_id) $chkBox = '<input type="checkbox" name="idList[]" '.($browsingInThePast ? "disabled" : "").' checked value="'.$current_row_role_id.'">'.'<input type="hidden" name="idList2[]" value="'.$current_row_role_id.'">'; else $chkBox = '<input type="checkbox" name="idList[]" checked disabled value="">'; } return $chkBox; } </pre>
 [2006-08-08 09:17 UTC] olivierg at php dot net (Olivier Guilyardi)
Okay, I've reproduced this bug under PHP4 and PHP5 with the following script: http://www.samalyse.com/ln/0016.php The problem is that removeColumn() shorten the $_columnSet array without reindexing it. In my script above there are 5 columns at the beginning, with keys from 0 to 4. After removing the columns there are 4 columns in the array. Then after addColumn() the keys are : 0,1,2,3,5. The key 4 is missing. That causes the "undefined offset" notice you mention. Using unset() in removeColumn() does not reindex the array, so that the "$this->columnSet[] =& $column" expression ignores the real size of the array. All of this is properly documented in the PHP manual: http://www.php.net/manual/en/language.types.array.php Your array_pop() solution is indeed a short and efficient fix for this issue. It's in the CVS. I close this bug. Thanks for you contribution :-)
 [2006-08-08 19:44 UTC] arcadius at menelic dot com
Many thanks for taking care of this issue. Arcadius.