Knut Urdalen wrote: > Marshall Pierce wrote: >> Over the past little while, I've been working here and there on >> improving log4php to use PHP 5 features for my employer, and I've gotten >> the OK to contribute my improvements back to the project. I'm not quite >> finished with my improvements, but I wanted to get some feedback as I >> continue forward. >> >> Improvements: >> >> - Relocate and rename classes to support autoloading (and provide an >> autoload implementation) >> - Move from php4 'var' to private/protected as appropriate and add >> getters/setters as needed >> - Bug fixes / other minor refactorings >> - Use exceptions (still remaining to be done: make sure exceptions will >> not bubble up into code that uses log4php) >> - Reformat code (mostly -- there are still parts that I haven't gotten >> to yet) to match the standard we use at work > > Thanks for sharing your fork of the project. > > log4php already works with PHP 5: > http://svn.apache.org/repos/asf/incubator/log4php/trunk/src/main/php/
Very true (after all, 0.9 worked with PHP5), but it still has leftover PHP4 cruft like 'var' and use of PHP references (&). > > and we have a bunch of working examples: > http://svn.apache.org/repos/asf/incubator/log4php/trunk/src/examples/php/ I shall take a look at those and make sure they all work as expected. > > and unit tests (using PHPUnit 3.2): > http://svn.apache.org/repos/asf/incubator/log4php/trunk/src/test/php/ All the unit tests pass (after some modifications to use my class names instead of the old ones, and other minor tweaks). > > The PHP 5 version of log4php was the reason why the whole incubation > process was restarted and I used all the examples and the unit tests to > hunt down bugs. > > I prefer to stick with the current naming convention and focus on the > functionality. When I started working on the PHP 5 version I did the > same thing as you did and ended up with the same directory structure and > naming convention. But after a while I realized that I was just wasting > hours for no good reason and would break existing applications, so I > started focusing on the stuff that mattered. I disagree that changing the name convention is not a good reason to change. First of all, it doesn't follow the PEAR standard of Packagename_Subpackage_Etc. Second, many of the classes are named confusingly: LoggerHierarchy objects are referred to as 'repository' objects in variable names and comments (which makes sense since the class really should be named something to do with a Repository); Logger is a poor class prefix to use since many classes do not actually have anything to do with a Logger object, etc. Third, classes are placed in unhelpful directory structures: 'spi' and 'varia' are not meaningful directory names; 'xml' contains a configuration class that would make more sense living elsewhere; 'config' does not actually contain all or even most of the classes that have to do with configuration, etc. In any case, I think that it is reasonable to continue to maintain the old code base (backporting bug fixes) to let users maintain their investment in any old code they might have that depends on it (especially PHP4 users), while simultaneously offering a new, cleaner codebase for PHP5 users. The migration path, for those who choose to switch to the new codebase, would be quite easy as well. Just change the class names specified in the config file and find/replace LoggerFactory::factory to be the new method call. As far as Curt Arnold's objection that a monolithic changeset like mine is hard to understand, that's completely understandable, and I agree that it does make it more difficult to see exactly what was changed. However, that does not mean that it *is* difficult -- just *more* difficult. While files have been moved around and renamed, the code has not been drastically altered beyond that. Most of my fixes have been small, isolated changes which will be easy for interested parties to find by just looking through the old and new versions. Log4PHP is not a big project (about 10k lines), so this is not infeasible. > > You mention that you have done various bug fixes. It's really > appreciated if you can file patches against the current code base. While > writing the PHP 5 version I fixed a whole lot of minor things. I'm > pretty sure you have find some new, and that many of them are already > fixed. I'll take a look at what's happened in trunk since 0.9 and make sure any bug fixes are reflected in my fork. > > About new functionality we already have a lot of stuff on our TODO-list: > http://svn.apache.org/repos/asf/incubator/log4php/trunk/TODO > > I see that one of the reasons behind your fork is the autoload > mechanism. We should provide an autoload method in log4php, but this > could be done with a associative class mapping array, where the > classname is the key and the relative file path is the value. That is a > better solution of two reasons: > 1) log4php can provide an autoload method with the current naming > convention > 2) The performance is much better because it's simply a lookup in an > array instead of IO-access and checking if the class file that are to be > loaded actually exists, which is unnecessary. (log4* should be as > lightweight as possible so this is important) > > Best regards, > Knut Urdalen I've done some simple performance tests, and my autoload implementation is at a statistical dead heat with one using an associative array. (There is variability as to which would come in faster in a given run, and all runs indicated that regardless of which one was clocked as faster, they were extremely close, sometimes as little as .01s apart after 1000 iterations.) In any case, far more time was used actually starting the PHP interpreter and parsing files than it was during autoloading, let alone actual code execution. Marshall Pierce
