Bug #66

[PATCH] validate package.xml in PEAR_Packager::package()

Details

Submitted	2003-10-05 16:08 UTC
From	neuhauser+pear-bugs at bellavista dot cz
Assigned	cox
Status	Closed
Package	PEAR
PHP Version	5CVS-2003-10-05 (dev)
OS	N/A
Roadmaps	(Not assigned)

Comments

[2003-10-05 16:08 UTC] 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

[2003-10-05 16:18 UTC] 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 18:01 UTC] neuhauser+pear-bugs at bellavista dot cz

new patch at http://smradoch.innuendo.cz/pear/PEAR::Packager::package()-validate2.patch

[2003-10-06 09:14 UTC] 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