#66 [Opn->Csd]: [PATCH] validate package.xml in PEAR_Packager::package()

ID: 66 Updated by: cox@php.net Reported By: neuhauser+pear-bugs at bellavista dot cz -Status: Open +Status: Closed Bug Type: PEAR Operating System: N/A PHP Version: 5CVS-2003-10-05 (dev) New Comment: Fixed in CVS thanks Roman. Previous Comments: ------------------------------------------------------------------------ [2003-10-06 05:14:13] neuhauser+pear-bugs at bellavista dot cz > Could you provide a test script where is it b0rked? there is code that can never be reached: see the first chunk of the second patch. > As far as I see in your patch validatePackageInfo() uses an unitialized $pkgdir. you're right. I've updated the patch. > Anyways I can't see the benefit of this patch without more explanations. * no code that can never be possibly reached * the directory containing $pkgfile doesn't need to be writable anymore * no chdir() calls all over the place * no half-assed code duplication from PEAR_Common::validatePackageInfo() plus, see this log from #pear @ efnet <roman_> I submitted a PR (#66), just to realize a few seconds later it was bogus. the code is broken, but in a different way. <roman_> PEAR_Packager::package(): <roman_> 1. if pkginfo['version'] is empty, throw an error <roman_> 2. if pkginfo['version'] is empty, and pkginfo['release_state'] == 'snapshot', populate pkginfo['version'] <-- but this will never be reached <roman_> 3. do a lot of other stuff, and only then validatePackageInfo() <roman_> why this order of things? <_mj> roman_: 2. should definitively be fixed <_mj> and providing a clean patch for 3. sounds like a good idea as well <roman_> _mj: there's more I can't wrap my head around in package(): <roman_> it chdir()s into dirname($pkgfile), yet validatePackageInfo() is called with 4th argument of dirname($pkgfile) <roman_> that seems wrong <_mj> maybe the chdir is needed after validatePackageInfo() has been called? <roman_> that's likely, but not what I'm concerned about. AFAICT the 4th arg in validatePackageInfo() is meant for the cases when cwd != dirname(/path/to/package.xml) <roman_> which is NOT the case after the chdir(). <_mj> ic <roman_> btw, which way should 2. be fixed? <_mj> roman_: i'd remove the second check <roman_> _mj: remove the pkginfo['version'] generation? <_mj> yeah ------------------------------------------------------------------------ [2003-10-05 17:35:15] cox@php.net Could you provide a test script where is it b0rked? As far as I see in your patch validatePackageInfo() uses an unitialized $pkgdir. Anyways I can't see the benefit of this patch without more explanations. ------------------------------------------------------------------------ [2003-10-05 14:01:11] neuhauser+pear-bugs at bellavista dot cz new patch at http://smradoch.innuendo.cz/pear/PEAR::Packager::package()-validate2.patch ------------------------------------------------------------------------ [2003-10-05 12:18:25] neuhauser+pear-bugs at bellavista dot cz erm, scratch that, it does call validatePackageInfo(), just further below. package() is borked, but in another way. i'll reopen this PR later. ------------------------------------------------------------------------ [2003-10-05 12:08:07] neuhauser+pear-bugs at bellavista dot cz Description: ------------ while the installer aborts if the package.xml file doesn't pass validatePackageInfo(), the packager doesn't even perform this check. is that intended? IMO the packager should make every attempt to make sure that the created package will not wreak havoc when installed, shouldn't it? see http://smradoch.innuendo.cz/pear/PEAR::Packager::package()-validate.patch ------------------------------------------------------------------------ -- Edit this bug report at http://pear.php.net/bugs/bug.php?id=66&edit=1