Marshall Pierce wrote: > 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
Should I interpret this list's continued silence on this topic as unwillingness to adopt my changes? -Marshall
