Description: ------------ Clean up code which directly accesses private variables; and make sure that everything is stored appropriately. See Request #14917 and Bug #14892 - The former has a patch which half changes this behaviour, the latter is a bug caused by forgetting to do type casting on a private variable. Use of getters / setters here would appropriately prevent the need to type cast in the middle of other code. IE: function doFiles($ser_func) { if (!is_array($this->files)) { $this->files = explode(',', $this->files); } }

Can you provide a rationale of using getters in class methods? They are bad for performance. Setter is a reasonable change to ensure that argument is converted to array as a convenience feature, but checks with getters are not necessary. Getters/setters are good for visual editors from Java world, but I can't see their immediate advantage here. I would revert http://cvs.php.net/viewvc.cgi/pear/File_SearchReplace/SearchReplace.php?r1=1.19&r2=1.20 before release and remove $local_find/$local_replace with $this->find and $this->replace where these variable are not modified. It will avoid extra memory allocation. You may also want to recheck if PHP5 API added more features to count replacements more effectively that this library does when it was designed for PHP4. The message "Fixes bug similar to Bug #14892" is vague. CS affairs is not a sufficient reason to change API for a stable package. Can you be more specific what similar bug were you aimed at? As for doFiles example - converting such minor logical steps to a method is not a good programming practice, because getter name doesn't give you an idea why can't you just take and use your class variable in class method.

With: http://cvs.php.net/viewvc.cgi/pear/File_SearchReplace/SearchReplace.php?r1=1.19&r2=1.20 Using $local_find = $this->getFind(); 1. Ensures that it does the typecasting - array_values((array) $this->find); in the getter 2. Prevents another occurance of Bug #14892, with: $file_array[$i] = str_replace($local_find, $local_replace, $file_array[$i], $counted); // a second occurance of str_replace being called with mixed types, which is exactly the same symptoms as Bug #14892 This request is open to track leftovers from Request #14917; which is basically what you are looking for: where appropriate, make use of setters / getters to ensure the data we expect is in the relevant types. Refactoring the setters in the class was getting a bit beyond the scope of the original request (at least the difficulty was higher), so it got split off.

setFind() from documented API is used to ensure that arguments are converted. Why do you need another check in getFind()?

Principle of least surprise. Example 1: $foo = new Foo(); $foo->setBar(1234); // If I haven't read the documentation, I don't know this converts the input to array(1, 2, 3). print $foo->getBar(); // I get back array(1, 2, 3); but the phpdoc for the method gives me no hint that it will return it. What the heck is going on. Example 2: class Bar extends Foo { public function __construct($data) { $this->bar = $data; } } $foo = new Foo(); $foo->setBar(1234); $bar = new Bar(1234); assert($bar->getBar() == $foo->getBar()); //That's funny, why are these different? getBar() isn't changed from the original class, both instances return $this->bar; what's going on! When you look at Bug #14892 ; using the getters: 1. Ensures you really know you are getting an array 2. Simplified code greatly Finally, (and this is the main point of the change request) when you look at other places within this class, such as doFiles(), you can see member variables are getting directly manipulated. Look at: $foo->setFiles("1,2,3"); var_dump($foo->files); // string "1,2,3", check! $foo->doFiles(); var_dump($foo->files); //Whoa, who changed my $foo->files to be an array? $foo->doFiles(); // This time the code takes a different execution path; it doesn't manipulate internal stuff now var_dump($foo->files); // Now it stays the same; Unless the unit tests run twice over, you've just lowered your testability / code coverage. Specific places where this is happening: doDirectories() doFiles() doReplace() Other places where defensive programming can't hurt: $foo = new File_SearchReplace(); $foo->setIgnoreLines(true); // Doesn't do type casting at the moment. $foo->doSearch(); // calls search() ... all of a sudden, I'm trying to use a bool as an array.

Neither of examples illustrate the problem you are trying to solve in this particular package. If you use documented API - E_NOTICE will not happen, because arguments are correctly transformed. If you do not read documentation and prefer to change private variables directly then it can't be helped. Another level of indirection doesn't simplify the code. It also can not ensure you know you are getting an array, because getter name does not tell you that. Docstrings and documentation ensure you are doing things right. These docstrings are better applied to $this->find / replace directly. Finally, it is absolutely OK for methods of the class to directly manipulate object variables. Just review the code and make sure they are used properly. I can ensure you that in this particular case there is no need for additional harness. If you are still about to make the changes - consult the mailing list. I am not an active developer anymore. Their opinion should be decisive.

anatoly, if you would like, I can flick the package over to unmaintained?

You are the only maintainer now. De facto. Do as you wish. =)