Re: phar: problem with renaming implementation

Hi Greg,

First, the bad news.

On PECL Announce? (just bemused by that)

The current implementation of renaming is fatally flawed. In searching for the source of the memleak, I found that what happens on renaming is this: 1) phar is added to manifest with the new name 2) phar is not removed from manifest with the old name on shutdown, we get PHP trying to free both the new and the old name, hence the double free and memleak. The only way around this is to completely copy the phar manifest, but then we have a problem with flushing and it gets really messy really fast.

Yes... I already looked at that and thought 'no way'.

The good news is that I think there is a simpler way to do this. What we can do is the following: 1) flush any changes 2) use good old copy() to copy from the old to the new phar (i.e. from blah.phar to blah.phar.gz) 3) parse the new phar, creating a brand new unconnected manifest entry with the new name 4) set the compression for the archive (if any) 5) flush the new archive, replacing the copy()ed file (blah.phar.gz in my example). This way, the old archive is never touched, and the new one then exists.

Sounds good.

Here's some more bad news. Every method that renames will have to return a new phar object - there is no way to transparently change the path used by DirectoryIterator once it has been instantiated.

This was in fact the original proposal, it's a lot less likely to cause problems.

As such, we need to reconsider the API. I'd like to suggest we drop most of the compression and conversion methods altogether from their current implementation. Instead, I'd like to create a factory method of sorts that accepts as arguments an existing Phar object, and some flags to tell it what to do (compress? convert to zip? etc.) sample code: <?php $phar = new Phar('blah.phar'); $newphar = $phar->convert(Phar::ZIP); $another = $phar->convert(Phar::TGZ); $third = $phar->convert(Phar::TAR); $fourth = $phar->convert(Phar::GZ); ?> The constants we would use would be Phar::ZIP, Phar::PHAR, Phar::TAR, Phar::BZ2, Phar::GZ, Phar::DATA and Phar::UNCOMPRESSED. We would allow Phar::TAR | PHAR::GZ or similar combos, but not Phar::ZIP | PHAR::GZ. Phar::DATA would be used to convert an active phar into a data-only phar (i.e. remove the alias and the stub from a tar or zip) and could be or'ed with Phar::TAR and its GZ/BZ2 constants, or with Phar::ZIP.

I'm not sure whether this factory method should be internal or exposed... my instinct would be to keep it internal because convertToZip() etc are very intuitive, as method names go.

compressAllFiles*() would go back to its original purpose of compressing individual files within the archive (and would throw an exception if called for tar-based phar archives).

I'm not 100% sure what you mean by that... Does anyone (except you presumably) knowingly compress each separate file within an archive? Altogether, if we're revamping that part of the API I'd like to keep it simple for the casual user: compress(Phar::GZ|BZ2) - returns a handle to the compressed file or FALSE isCompressed() - returns Phar::GZ|BZ2|FALSE either of 'uncompress()' or 'decompress()' - returns a handle to the decompressed file or FALSE - this the same naming for Phar, PharData and PharFileInfo. If you _also_ want compressAllFiles() and it means something different, bags you do the docs for it :)

Finally, we need a method for grabbing the phar filename.

? internal or exposed? (Not entirely sure what you mean here either.)

Discussion should ensue, and I'm also curious whether you want to take this on Steph? It should result in MUCH simpler code and logic for our debugging purposes as well as in the end-user PHP level.

More than happy to - relieved, even - with the caveat that I'd like those bits I'm not clear about, clarified. Apart from that, you just keep up the rear end with the battle for stream wrapper access to include_path ;) - Steph ps Thanks a million for looking into this. I knew where the problem was, but I'd no way to walk through it step-by-step under doze.

Greg -- PECL development discussion Mailing List (http://pecl.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php