Re: On technical debt

On Wed, Oct 6, 2010 at 4:16 PM, Daniel O'Connor <daniel.oconnor@gmail.com> wrote: > > > On Wed, Oct 6, 2010 at 2:07 AM, Daniel O'Connor <daniel.oconnor@gmail.com> > wrote: >> >> >> On Fri, Sep 24, 2010 at 3:51 AM, till <till@php.net> wrote: >>> >>> On Thu, Sep 23, 2010 at 6:30 PM, Michael Gauthier <mike@silverorange.com> >>> wrote: >>> > On Mon, 2010-09-20 at 18:47 +0930, Daniel O'Connor wrote: >>> >> I know a lot of us know this, but I thought I'd share anyway: >>> >> http://petdance.com/perl/technical-debt/ >>> >> >>> >> Looking at pear as a whole; what would people say are the bigger debts >>> >> around? >>> >> >>> > >>> > I'd say pearweb and pear packages themselves are have fairly large >>> > technical debts. Only a few people understand how pearweb works and it >>> > is not documented. The PEAR code itself is confusing and sparsely >>> > documented. >>> > >> >> PEAR core; agreed - it's nearly incomprehensible to me when I've gone >> diving in. I mean, I can find specific bugs and stomp on them; but often I >> just have to stare at code to work out what it's doing. >> A lot of it feels needlessly complex, and not exactly self documenting - I >> can't get a great feel for the core. > > > Today's spelunking: Why is there so much need for &new? > > It's deprecated as heck; but widely used in PEAR core. As you can see, > modern PHP doesn't like it. > If we grab one line and trace it through... > > http://test.pear.php.net:8080/cruisecontrol/buildresults/Crypt_DiffieHellman > > Looking at the code here reveals nothing ominous: >         $pf = &new PEAR_PackageFileManager2(); > > Ah hah! Unless the planets align into a rare scenario as in > http://www.php.net/manual/en/oop4.newref.php ; surely this is > safe to > remove. > > Better check the constructor: > >     function PEAR_PackageFileManager2() >     { >         parent::PEAR_PackageFile_v2(); >         $config = &PEAR_Config::singleton(); >         $this->setConfig($config); >     } > > Uh oh, a reference to a singleton? > > http://svn.php.net/viewvc/pear/pear-core/trunk/PEAR/Config.php?view=markup#l691 > and another &new PEAR_Config? > > What's the constructor actually do which is so important you only ever need > one configuration ever instantiated? > >  1. A bit of work in the constructor >  2. Guess your local and system setup config files >  3. Merge them and populate the object >  4. &new PEAR_Registry($this->configuration['default']['php_dir']); > > PEAR_Registry is just a data structure with bits and pieces on it; so no > dramas there. > > So; way up high in PFM2; it's not very safe to remove the &; because you > might break the PEAR config singleton. > > Probably not something you want to do; because it's certainly not a read > only thing - it deals with writing back configuration too. > > ack-grep tells me it's used in: > make-gopear-phar.php > make-installpear-nozlib-phar.php > PEAR/Command/Common.php > PEAR/Command.php > PEAR/Common.php > PEAR/Config.php > PEAR/DependencyDB.php > PEAR/Downloader.php > PEAR/Frontend/CLI.php > PEAR/RunTest.php > PEAR/Start.php > and a bunch of the tests. > > My head tells me that only Frontend/CLI or the high level application class > should ever tell the configuration to update; when you do channel-update, > install, upgrade, uninstall, config-set or similar. > > Because this wasn't really known when the pear installer was written > originally, the singleton became the quick/safe way to make sure you don't > destroy pear config; and stability > refactoring. > > > > Contrast it with Pyrus: > > http://svn.php.net/viewvc/pear2/Pyrus/trunk/src/Pyrus/Config.php?view=markup > > There's still statics, work in the constructor and a singleton. > > However, it's much healthier; as the singleton method is largely not used.. > clockwerx@clockwerx-desktop:/media/Elements_/backup/home/clockwerx/pear2/Pyrus/trunk/src$ > ack-grep -al 'Config::singleton' > Pyrus/PluginRegistry.php > Pyrus/ScriptFrontend/Commands.php > > ... or is it? > Config::current is a pointer to a single copy of a Pyrus Config object. > > It's used in many places: > Pyrus/Package/Base.php > Pyrus/Package/Creator.php > Pyrus/Package/Dependency/Set/PackageTree.php > Pyrus/Package/Dependency/Set.php > Pyrus/Package/Remote.php > Pyrus/AtomicFileTransaction.php > Pyrus/Channel/RemoteCategory.php > Pyrus/Channel/RemotePackage.php > Pyrus/Config/Snapshot.php > Pyrus/Dependency/Validator.php > Pyrus/Installer/Role/Cfg.php > Pyrus/Installer/Role.php > Pyrus/Installer.php > Pyrus/Main.php > Pyrus/PackageFile/v2/Validator.php > Pyrus/PECLBuild.php > Pyrus/PluginRegistry.php > Pyrus/Registry/Base.php > Pyrus/Registry/Package/Base.php > Pyrus/Registry/Pear1/DependencyDB.php > Pyrus/Registry/Pear1.php > Pyrus/Registry/Sqlite3.php > Pyrus/Registry/Xml.php > Pyrus/REST.php > Pyrus/ScriptFrontend/Commands.php > Pyrus/Task/Postinstallscript.php > Pyrus/Task/Replace.php > Pyrus/Uninstaller.php > > > So the main motivation for doing pyrus was to make something PHP 5.3 > friendly and ditch the cruft. > > I think we've achieved that only at a cosmetic level - solving the &new > problem; not the let's instantiate one copy of a config object and pass that > around to whomever needs it problem (dependency injection style hurray). > > I find the way out of this kind of code is a good strong mocking library in > your favorite test suite, and hours of refactoring. However, given that > things like PHPUnit tend to be pear installed, you either have to solve an > svn(git?):externals problem or settle for simpler tests. > > > So; I'm at a crossroads: is it better to try small refactorings to remove > &new and global state in old pear code; to keep tools that rely on it > happily running; or to invest large efforts to remake those tools based on > Pyrus? > Or is it better to put in large efforts refactoring pyrus' global state away > and then build the tools atop that? > > Or; and what is a painful but pragmatic choice; say nuts to you, CI box, I'm > not looking at you! whenever I think these thoughts. > > I think all efforts should be directed towards Pyrus and PEAR2. It's the installer and framework which we want to build on in the future. Of course it's unfortunate that PEAR(1)-core has these issues (Hello, PHP4!) but IMHO it's something that we shouldn't waste time fixing. Brett, Greg and Helgi (and others) have been coding on the new stuff and I think they all deserve help there. In the long run it would be nice to migrate everything away to a much better pear2/pyrus based infrastructure and to achieve that some day, it needs to be done right. In regard to the current state of PEAR2 and pyrus - I know that an issue can be that with the complaining some of us, e.g. myself, seem ungrateful for what has been done already. I am really not. I'm just not happy with the way things are. But I really appreciate all the work that went into it. But I'd also like to be able to help and also understand why it's so hard to hold up the principles we enforce for contribution in the core code base. Currently, it's just damn hard to go into code and write tests for it if you don't understand it and don't have the time to dive into it for a week or so. I also feel that whenever you require people to read heaps and heaps of slightly outdated documentation, something is wrong. ;-) Feel free to discuss my point of view, but that's where I currently stand. Think adoption of the SCS vs. Pirum - the codejust has to be easier. No books or docs will help otherwise. And maybe I also suck at PHP - who knows. =) I'm still serious about this though - I want to contribute. :-) Till