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

Bug #4295 csv file is not well closed
Submitted: 2005-05-06 09:42 UTC Modified: 2005-08-09 02:56 UTC
From: farell Assigned: dufuz
Status: Closed Package: File
PHP Version: 4.3.10 OS: windows
Roadmaps: (Not assigned)    
Subscription  


 [2005-05-06 09:42 UTC] farell
Description: ------------ All explains, source code and results, are available at URL http://pear.laurent-laville.org/proposals/file_csv_120.html Hope it will help ! Expected result: ---------------- CSV file closed at end of a search-unit Actual result: -------------- CSV file is still open

Comments

 [2005-05-10 06:21 UTC] dufuz
as I asked on the mailing list, instead of removing the code like you do in the patch, why not try to add a param that will clean out $resources on request but doesn't do that by default ? Because I'm a little scared someone is relying on this current behaviour.
 [2005-05-21 01:44 UTC] dufuz
To make this report alive again, would you accept to that I'd add a param to both read and write that would be boolean, and if passed as true than you'd be bypassing the static $resources and thus your script should run just fine ? note $resources is there for performance reasons so I won't remove it, I can tell you right away that it wouldn't be joke to create a new resource on each request :-) So really the param way is the easiest and the only way. I could tho also just try to reset the resource so it goes to the first line again and still doesn't open the file again and all the performance stuff. Tell me what you want me to attempt and would suit you.
 [2005-05-21 02:19 UTC] dufuz
I came up with a solution, it's so easy that it's just silly! :) Basically I added a new function which calls getFilePointer and moves the file pointer to the beginning :P I tried you test script and everything worked lovely in there, I just replaced File::close with the new functon, but before I commit anything I have to wait for input from Mike on a question of mine, but you can consider the bug to be fixed. When I commit than I will close this bug report and tell you the name of the new function and we'll try to release soon after that,
 [2005-06-07 01:57 UTC] dufuz
Laurent, ping, any thoughts on this ? The function in question will probably be called, resetFilePointer, but I'm open for other naming suggestions.
 [2005-06-07 12:05 UTC] farell
Helgi, To be honest, i don't think it's a good idea to add a new api to reset pointers. My point of view is as we used the _getFilePointer() function of main class (File.php) at http://cvs.php.net/co.php/pear/File/File/CSV.php?r=1.21#142 to get a filepointer, it's not necessary to have lines i recommanded to removes (previously). I know you're scared about removing such lines, but tell me why should we do complex things when we can do it so simple. There is no risks! If you find some, then explains me and i'll heard you . Hope you understand Laurent
 [2005-06-07 12:47 UTC] farell
In reorganisation of my server, i've moved the pages at http://laurent-laville.org/pear/File/ You may still find all explains, highlight sources code. And now since today a changelog and a live demo to have visual results. Laurent
 [2005-06-07 21:46 UTC] dufuz
I am still not going to remove any lines. Are you aware that you are removing a thing that potentially keeps the performance up in this sript ? If you have runned the script once it will save to the resources array which in turns makes you not have to go in there again and fetch the filepointer from file, thus saving you from extra calls and such, removing this could also damage other scripts. function resetFilePointer($file, &$conf, $mode) { if (!File_CSV::getPointer($file, $conf, $mode, false)) { return false; } return true; } function getPointer($file, &$conf, $mode = FILE_MODE_READ, $reset = false) { static $resources = array(); static $config; if (isset($resources[$file])) { $conf = $config; if ($reset) { fseek($resources[$file], 0); } return $resources[$file]; } Here are the parts I changed, which allows you to reset the file pointer to the beginning again and still have the resources array, the resetFilePointer is really just a wrapper function, you can get getFilePointer straight, but that didn't make much sense to make :) Doesn't this do everything you need with out the need of removing ? This is the best so far I could come up with that worked with your test case and didn't remove any lines. And you are just replacing the file::close in your test script with file_csv::resetfilepointer() and adding conf arrray as on of the params.
 [2005-06-07 22:00 UTC] dufuz
that should have been "true" which is past on from resetFilePointer
 [2005-06-08 11:17 UTC] farell
Helgi, to reply to your comment about : "Are you aware that you are removing a thing that potentially keeps the performance up in this sript ?" is it not the behaviour of File::_getFilePointer() which is called by File_CSV::getPointer itself (line 142). I won't raise a flame war, so if you think it's really necessary to have twice api that keep track (cache) of file pointers, then i suggest you to call your new api *close*. It' more evident than resetFilePointer, because end-user will do : - open a csv file - read a csv file - close a csv file and don't want to know what there is behind such operation (manage pointers, cache them , or anything else ...) Laurent
 [2005-06-09 22:42 UTC] dufuz
I've no interest in flame war, but the thing is, this function isn't to keep track of anything really, just a wrapper around a protected function. The only thing it does, it moves the pointer of the stream back to the beginning with out the need of closing the file and opening it again since it's using what has already been open. File::_getFilePointer actually has this line: $filePointers = &PEAR::getStaticProperty('File', 'filePointers'); Which is doing the same as static $resources = array(); AFAIK with the exception that getStaticProperty can't handle many files at the same time, only $foo[$class][$var] but correct me if I'm wrong. Anyway I would have think you'd _like_ the fact that you are only being moved back from the beginning instead of close -> open to get to the beginning :) Anyway tell me what you thing, and if "close" or "reset" sound better after having read this. Thanks for you patience and time ;)
 [2005-08-09 02:56 UTC] dufuz
This bug has been fixed in CVS. In case this was a documentation problem, the fix will show up at the end of next Sunday (CET) on pear.php.net. In case this was a pear.php.net website problem, the change will show up on the website in short time. Thank you for the report, and for helping us make PEAR better.