Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:36906
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain chiaraquartet.net from 38.99.98.18 cause and error)
Message-ID: <47FCE3B0.5090004@chiaraquartet.net>
Date: Wed, 09 Apr 2008 10:41:36 -0500
User-Agent: Thunderbird 2.0.0.12 (X11/20080227)
MIME-Version: 1.0
To: Steph Fox <steph@zend.com>
CC: Lars Strojny <lars@strojny.net>, php-dev <internals@lists.php.net>, 
 Marcus Boerger <mail@marcus-boerger.de>
References: <47ED6D97.3030109@chiaraquartet.net>	 <1206788577.3621.15.camel@localhost>  <47EEC989.5020603@chiaraquartet.net> <1206839984.3621.71.camel@localhost> <47FC4A83.6080205@chiaraquartet.net> <009c01c89a24$4d88f2e0$4001a8c0@foxbox>
In-Reply-To: <009c01c89a24$4d88f2e0$4001a8c0@foxbox>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] practical phar considerations
From: greg@chiaraquartet.net (Gregory Beaver)

Steph Fox wrote:
> Hi Greg,
> 
>> What I would like to do, however, is to rethink offsetSet().  I think
>> that we should introduce a property of PharFileInfo called "content"
>> that is read/write, and can be used to perform the equivalent of
>> file_get_contents()/file_put_contents().  This will allow a consistency
>> that is much more intuitive.  offsetSet() could then simply be removed,
>> or perhaps be refactored as a way to pass in a PharFileInfo and clone
>> the information.
> 
> Could you offer up some pseudo-code showing how this will be used? I
> can't visualize it at all from that description.

I should have done this in the original post, because it turns out that
it would require offsetGet() to implicitly create the file, which brings
me back to my original objection to these changes.  I'm adding in
getContents() right now to retrieve the file contents into a string, so
one can do:

<?php
$phar = new Phar('blah.phar');
echo $phar['path/to/file.txt']->getContents();
?>

The truth is, I think offsetSet() could accept either a string/fp (file
contents) or a PharFileInfo() that can be used to set a slew of things:

 - metadata
 - per-file compression
 - file contents

This would also be relatively trivial to implement.

>> One problem with this suggestion is that phar renames the archive by
>> default when compressing.  I would rather we combine compression and
>> format conversion into a single convert() method that accepts constants.
>>
>> $phar = new Phar('blah.phar');
>> $phar = $phar->convert(Phar::GZ); // compress with gzip
>> $phar = $phar->convert(Phar::TAR | PHAR::GZ); // convert to .phar.tar.gz
>> $phar = $phar->convert(Phar::NONE | PHAR::DATA); // convert to
>> non-executable .tar, returns PharData object
>> $phar = $phar->convert(Phar::EXE); // convert back to .phar.tar
> 
> Didn't we just go full circle here? There was a proposed convert()
> method that did absolutely everything (on e-paper) and it was far less
> intuitive than when conversion was separated from compression. That's
> why you ended up implementing them separately, as I recall.

It does look similar, but the key difference here is that convert() was
a method that changed the underlying object.  If we return a new object,
this is a completely different ballpark, the current implementation
actually can remain the same and remove the intermediary files.

>> This would also solve a subtle but important problem with the current
>> conversion API where intermediary files are lying around.
> 
> Can't we simply delete those intermediary files on success? That would
> be the normal procedure when renaming, no?

Actually, no.  The current implementation has no modification to the
original file on disk, as there is no way to know in advance if the user
actually intended to erase it, and it is actually impossible to do so if
there are any open references to the archive (i.e. Phar objects or
stream fp's lying around).

>>>> Do you want to take a crack at a patch for any of these API features
>>>> once they are worked out (buildFromDirectory should be an easy one, if
>>>> you would like to start there)?
>>>
>>> Yes, I'm willing to help with it to a degree, as I said in a previous
>>> mail.
>>
>> How soon can we expect a patch for this?  Steph also expressed interest,
>> and we are on a rather tight time schedule to get this to beta, which
>> will mean a feature freeze (no new crap, just bug fixes).
> 
> I'll implement buildFromDirectory() this weekend. It would've been last
> weekend, but I got caught up in trying to bring the last few non-core
> PECL modules into line over versioning structure, which hasn't been
> entirely plain sailing. (Still 5 + IBM to go.)

OK.

Greg