Re: On technical debt

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.