# practical phar considerations

- Thread: https://externals.io/message/36664
- Original archive: https://news-web.php.net/php.internals/36664
- Started by Greg Beaver on 2008-03-28T22:13:43+00:00
- Messages: 11
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #36664 — Greg Beaver — 2008-03-28T22:13:43+00:00

URL: https://externals.io/message/36664#36664

Hi,

Before I say anything, I have just concluded 20 hours of travel and feel 
kinda funny, so please take that into consideration :).

Regarding API changes, let's first be clear:

1) Any changes to file creation need to be understood in terms of what 
they means.  For instance, let's look at:

$phar['full/path/to/file'] = 'hi';

This creates 1 entry "full/path/to/file"

$phar['full']['path']['to']['file'] = 'hi';

This would create 4 entries
full
full/path
full/path/to
full/path/to/file

In addition, this would mean that offsetGet() would have to create a 
directory.  Why?

$phar['full']['path']['to']['file'] = 'hi' calls offsetGet() with 
'full', 'path', and 'to', and offsetSet() only with 'file'.

In other words, the proposed change is much more complex, introduces 
magic and counterintuitive behavior inside the extension.  For instance, 
let's say we have this code:

<?php
$a = $phar['dir'];
$b = $phar['dri']; // typo
?>

if phar.readonly=1, the second line would throw an exception for 
attempting to create the directory "dri"

What about this?

<?php
unset($phar['dri']); // dri doesn't exist
?>

The above code would first create the dri directory, and then erase it.

In other words, the proposed syntax is not a good idea.

2) Names must be very carefully considered in relation to existing APIs 
and the pre-existing methods.

Phar::create(), for instance, should be named buildFromDirectory() to 
match buildFromIterator().

what benefit is there in adding complexity of remembering the Phar::GZ 
constant over setCompressedGZ()?  We have to think hard about changes 
like this.

3) I would like to suggest these changes, and these alone:

  * simplify all compression methods, with more consistency of naming 
that clearly differentiates between compressing files within an archive, 
and compressing the entire archive.  This abstract idea is more 
important to me than the specific naming, but it should be very easy to do.
  * add a zip-style API for adding files/directories, addFile, 
addEmptyDir, addFileFromString with the exact method names
  * add buildFromDirectory()
  * any further suggestions have off-list vetting with the maintainers 
of phar for technical issues prior to publicly posting them, so we can 
truly work together and present a more polished API suggestion to those 
interested in phar on the first try without confusing them.  I am *NOT* 
proposing we take discussion to private, only that technical issues be 
worked out offlist prior to presenting an idea.

Thank you for your patience, I know I am a bit of a hard-ass at times, 
but I am genuinely excited about the discussion and creativity that is 
going into this.

Greg

---

## #36665 — Steph Fox — 2008-03-28T22:37:02+00:00

URL: https://externals.io/message/36664#36665 · In reply to: #36664

Hi Greg,

> what benefit is there in adding complexity of remembering the Phar::GZ 
> constant over setCompressedGZ()?  We have to think hard about changes like 
> this.

We did think hard about them. In fact I thought you'd implemented 
compress(Phar::GZ) to replace the long-named one now :) Need to go play I 
guess.

> 3) I would like to suggest these changes, and these alone:
>
>  * simplify all compression methods, with more consistency of naming that 
> clearly differentiates between compressing files within an archive, and 
> compressing the entire archive.  This abstract idea is more important to 
> me than the specific naming, but it should be very easy to do.

Yep.

>  * add a zip-style API for adding files/directories, addFile, addEmptyDir, 
> addFileFromString with the exact method names

OK...

>  * add buildFromDirectory()

Cool...

>  * any further suggestions have off-list vetting with the maintainers of 
> phar for technical issues prior to publicly posting them, so we can truly 
> work together and present a more polished API suggestion to those 
> interested in phar on the first try without confusing them.  I am *NOT* 
> proposing we take discussion to private, only that technical issues be 
> worked out offlist prior to presenting an idea.
>
> Thank you for your patience, I know I am a bit of a hard-ass at times, but 
> I am genuinely excited about the discussion and creativity that is going 
> into this.
>
> Greg

---

## #36671 — Lars Strojny — 2008-03-29T11:02:57+00:00

URL: https://externals.io/message/36664#36671 · In reply to: #36664

Hi Greg,

Am Freitag, den 28.03.2008, 17:13 -0500 schrieb Greg Beaver:
[...]
> In addition, this would mean that offsetGet() would have to create a 
> directory.  Why?
> 
> $phar['full']['path']['to']['file'] = 'hi' calls offsetGet() with 
> 'full', 'path', and 'to', and offsetSet() only with 'file'.

I know that and I discussed that with Marcus beforehand. I don't think
that implicitly creating the directory is much an issue. If somebody
does not want that, he can just use isset() before.

> In other words, the proposed change is much more complex, introduces 
> magic and counterintuitive behavior inside the extension.  For instance, 
> let's say we have this code:
> 
> <?php
> $a = $phar['dir'];
> $b = $phar['dri']; // typo
> ?>
> 
> if phar.readonly=1, the second line would throw an exception for 
> attempting to create the directory "dri"

Wouldn't the second line throw an exeception anyway because the index is
not defined? And: can't offsetGet() check weither Phar::canWrite() is
true?

> What about this?
> 
> <?php
> unset($phar['dri']); // dri doesn't exist
> ?>
> 
> The above code would first create the dri directory, and then erase it.

No, it would not. Please take a look at
http://lars.schokokeks.org/php/phar.phps and
http://lars.schokokeks.org/php/phar.php

> In other words, the proposed syntax is not a good idea.

Two of three assumptions were not correct or at least arguable, so I
can't agree that easily.

> 2) Names must be very carefully considered in relation to existing APIs 
> and the pre-existing methods.
> 
> Phar::create(), for instance, should be named buildFromDirectory() to 
> match buildFromIterator().

I agree, that this name is much better.

> what benefit is there in adding complexity of remembering the Phar::GZ 
> constant over setCompressedGZ()?  We have to think hard about changes 
> like this.

setCompressedGZ() is just not the way object oriented APIs should
behave, as it taints the API with concrete implementation details
("there can be gzip compression and every compression algorithm is
exposed" opposed to "there can be compression, one variant is gzip").
Phar::compress() indicates that "the archive can be compressed" with the
concrete algorithm as a parameter. What about in 5 years when a new,
free compression algorithm rises, which fixes the problems of bzip2.
Would you like to add another setCompressedFooBar() or just add the
algorithm and the class constant and be fine?
If you insist on setCompressedFoo(), I would at least propose to not
implement that as a setter but to describe the attached behaviour, which
would be Phar::compressWithGzip() or something like that. Nevertheless,
please seriously consider to do it with a single compress()-method.

> 3) I would like to suggest these changes, and these alone:
> 
>   * simplify all compression methods, with more consistency of naming 
> that clearly differentiates between compressing files within an archive, 
> and compressing the entire archive.  This abstract idea is more 
> important to me than the specific naming, but it should be very easy to do.

What would be your concrete proposal regarding that?
[...]

cu, Lars

---

## #36680 — Greg Beaver — 2008-03-29T22:58:17+00:00

URL: https://externals.io/message/36664#36680 · In reply to: #36671

Lars Strojny wrote:
> Hi Greg,
> 
> Am Freitag, den 28.03.2008, 17:13 -0500 schrieb Greg Beaver:
> [...]
> > […]
> 
> I know that and I discussed that with Marcus beforehand. I don't think
> that implicitly creating the directory is much an issue. If somebody
> does not want that, he can just use isset() before.

Hi Lars,

If one uses file_put_contents('/path/to/this/file', 'hi') and 
'/path/to/this' does not exist, there is an error.  The same is true of 
fopen, regardless of mode.  `mkdir()` even fails unless the recursive 
parameter is explicitly specified.  I do not think that this behavior 
should be different in ext/phar, especially because all that is needed 
is to create 1 directory (full/path/to/) and add the file.

Incidentally, you cut out the portion of my objections that related to 
the creation of many unnecessary entries in the phar archive.  It is 
important to understand that every unnecessary entry in a phar archive 
causes phar to do extra processing on loadup, which ultimately increases 
the latency (this will be true until we implement caching of phar 
manifests, which will be relatively easy to do, I think).  In addition, 
the performance of:

$phar['long/path/to/subdir/file'] = 'hi';

compared to:

$phar['long']['path']['to']['subdir']['file'] = 'hi';

is significantly slower.  Each [] results in a call to offsetGet and 
instantiation of a new PharFileInfo object plus the final offsetSet call.

> > […]
> 
> Wouldn't the second line throw an exeception anyway because the index is
> not defined? And: can't offsetGet() check weither Phar::canWrite() is
> true?

Yes, but if you're attempting to access the "dir" directory, an 
intuitive exception would be "dri not found" instead of "write access is 
disabled, cannot create dri directory."  If I got the write access 
error, I would report a bug, since I was not trying to write anything.

> > […]
> 
> No, it would not. Please take a look at
> http://lars.schokokeks.org/php/phar.phps and
> http://lars.schokokeks.org/php/phar.php

You're right, chock this up to jet lag on my part. :)

> > […]
> 
> Two of three assumptions were not correct or at least arguable, so I
> can't agree that easily.

Only 1 was incorrect, but my objection still holds on a fundamental 
level: offsetGet() should not attempt to write anything.

However, let's examine the feature from another perspective - I think it 
could be useful as a read access method, for several reasons:

1) foreach($phar as $name => $file)

This construct does not iterate over every file in the phar archive, but 
instead iterates over only those files in the top-most directory.  If a 
directory returned implemented ArrayAccess and Iterator, one could treat 
it like a phar object.  Currently this isn't possible.  This means 
ultimately one should be able to do:

foreach($phar['subdir'] as $name => $file)

or

foreach($phar['subdir/anothersubdir'] as $name => $file)

or

foreach($phar['subdir']['anothersubdir'] as $name => $file)

2) Some might find it easier to do something like:

$phar['subdir/with/really/long/path/file1.txt'] = 'a';
$subdir = $phar['subdir/with/really/long/path'];
$subdir['file2.txt'] = 'a';
$subdir['file3.txt'] = 'a';

Incidentally, if you want to know a *real* hack in the implementation of 
ArrayAccess, you can make a directory now with this code:

$phar['subdir/with/really/long/path/'] = 0;

Needless to say, that probably should be removed as an option :).

Could you live with $phar['this']['thing'] as a read access API 
(offsetGet only)?

> > […]
> 
> I agree, that this name is much better.
> 
> > […]
> 
> setCompressedGZ() is just not the way object oriented APIs should
> behave, as it taints the API with concrete implementation details
> ("there can be gzip compression and every compression algorithm is
> exposed" opposed to "there can be compression, one variant is gzip").
> Phar::compress() indicates that "the archive can be compressed" with the
> concrete algorithm as a parameter. What about in 5 years when a new,
> free compression algorithm rises, which fixes the problems of bzip2.
> Would you like to add another setCompressedFooBar() or just add the
> algorithm and the class constant and be fine?
> If you insist on setCompressedFoo(), I would at least propose to not
> implement that as a setter but to describe the attached behaviour, which
> would be Phar::compressWithGzip() or something like that. Nevertheless,
> please seriously consider to do it with a single compress()-method.

As I was trying to say in the portion you quoted below, Phar::compress() 
is already used to compress the entire archive, and should not be 
overloaded.  I would be happy with these method names:

Phar::compressArchive() /* Steph hated this when I originally proposed 
it, perhaps she's changed her mind? */
Phar::compressAllFiles()
Phar::uncompressArchive()
Phar::uncompressAllFiles()

If I need to explain what the two methods do, then we need to choose new 
names, could you react to these names and let me know what you think 
they would do?

> > […]
> 
> What would be your concrete proposal regarding that?
> [...]

See above.

Lars, I also think it might help to understand my reaction better that 
we've been publicly discussing the API of compression and other 
phar-related issues on pecl-dev for a while, with no feedback.  To 
suddenly see an RFC with major changes without having even been notified 
was annoying, I hope that in the future you will at least contact the 
leads of the project you are proposing changes for so that minor 
disagreements can be worked out in private.  I always relish 
improvements, Steph can attest to both my flaws and my talents in this 
regard (I believe you witnessed a rough patch on IRC one day...), and 
also the satisfaction in the ultimate result, so I hope we will find our 
right working interaction from here forward. :)

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)?

Thanks,
Greg

---

## #36682 — Lars Strojny — 2008-03-30T01:19:44+00:00

URL: https://externals.io/message/36664#36682 · In reply to: #36680

Hi Greg,

Am Samstag, den 29.03.2008, 17:58 -0500 schrieb Greg Beaver:
[...]
> If one uses file_put_contents('/path/to/this/file', 'hi') and 
> '/path/to/this' does not exist, there is an error.  The same is true of 
> fopen, regardless of mode.  `mkdir()` even fails unless the recursive 
> parameter is explicitly specified.  I do not think that this behavior 
> should be different in ext/phar, especially because all that is needed 
> is to create 1 directory (full/path/to/) and add the file.

I mostly agree. Except that the whole argument chain applies to the
current API. You mentioned mkdir("foo/bar/baz") triggering an error
without the recursive argument, but Phar['foo/bar/baz'] does not.

[... Performance concerns ...]
> $phar['long/path/to/subdir/file'] = 'hi';
> 
> compared to:
> 
> $phar['long']['path']['to']['subdir']['file'] = 'hi';
> 
> is significantly slower.  Each [] results in a call to offsetGet and 
> instantiation of a new PharFileInfo object plus the final offsetSet call.

Could you estimate how hard that performance hit would be?
[....]
> Yes, but if you're attempting to access the "dir" directory, an 
> intuitive exception would be "dri not found" instead of "write access is 
> disabled, cannot create dri directory."  If I got the write access 
> error, I would report a bug, since I was not trying to write anything.

The exception message would be far away from being optimal, yes.
[...]
> Only 1 was incorrect, but my objection still holds on a fundamental 
> level: offsetGet() should not attempt to write anything.
> 
> However, let's examine the feature from another perspective - I think it 
> could be useful as a read access method, for several reasons:
> 
> 1) foreach($phar as $name => $file)
> 
> This construct does not iterate over every file in the phar archive, but 
> instead iterates over only those files in the top-most directory.  If a 
> directory returned implemented ArrayAccess and Iterator, one could treat 
> it like a phar object.  Currently this isn't possible.  This means 
> ultimately one should be able to do:
[... Code snippets ... ]

I would like that, yes.

> 2) Some might find it easier to do something like:
> 
> $phar['subdir/with/really/long/path/file1.txt'] = 'a';
> $subdir = $phar['subdir/with/really/long/path'];
> $subdir['file2.txt'] = 'a';
> $subdir['file3.txt'] = 'a';
> 
> Incidentally, if you want to know a *real* hack in the implementation of 
> ArrayAccess, you can make a directory now with this code:
> 
> $phar['subdir/with/really/long/path/'] = 0;
> 
> Needless to say, that probably should be removed as an option :).

Yes, this shouldn't be allowed while
$phar['foo/bar/baz'] = new DirectoryIterator(); could be allowed.

> Could you live with $phar['this']['thing'] as a read access API 
> (offsetGet only)?

I'm not sure if I would vote for a partial solution. I think - also I
really do think the current ArrayAccess API is cluttered - it is as
least consistent, which is a plus.

[...]
> As I was trying to say in the portion you quoted below, Phar::compress() 
> is already used to compress the entire archive, and should not be 
> overloaded.  I would be happy with these method names:
> 
> Phar::compressArchive() /* Steph hated this when I originally proposed 
> it, perhaps she's changed her mind? */

I like it. And much better, it does not contain the compression
format :-)

> Phar::compressAllFiles()

Wouldn't compressFiles() be fine?

> Phar::uncompressArchive()
> Phar::uncompressAllFiles()

Same here, maybe uncompressFiles()

> If I need to explain what the two methods do, then we need to choose new 
> names, could you react to these names and let me know what you think 
> they would do?

compressAllFiles()/uncompressAllFiles(): compress all the files
contained in the archive
compressArchive()/uncompressArchive(): Compress the archive
[...]
> Lars, I also think it might help to understand my reaction better that 
> we've been publicly discussing the API of compression and other 
> phar-related issues on pecl-dev for a while, with no feedback.  

I would not care so much for phar, if it's inclusion in core has not
been targeted. 

> To suddenly see an RFC with major changes without having even been notified 
> was annoying, I hope that in the future you will at least contact the 
> leads of the project you are proposing changes for so that minor 
> disagreements can be worked out in private.

On http://pecl.php.net/package/phar Marcus is listed as one of the two
project leads. I've talked to him *before* writing the RFC on friday. So
this accusation fizzles out. I sort of understand your irritation, but
from my point of view improvement takes place constantly. In the time
Phar was discussed on pecl-dev I was not subscribed to it, now I am. So
the next time I will come up earlier most presumably.

[...]

> 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.

cu, Lars

---

## #36685 — Steph Fox — 2008-03-30T14:32:14+00:00

URL: https://externals.io/message/36664#36685 · In reply to: #36680

Hi Greg,

> Phar::compressArchive() /* Steph hated this when I originally proposed it, 
> perhaps she's changed her mind? */

Nope :)

I still think compress(Phar::BZ2|GZ), uncompress(), compressAllFiles(), 
uncompressAllFiles(). And I also think compressAllFiles() only means 
something to a handful of people on the planet and needs good clear 
documentation.

> 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)?

Aw, I had my eye on that :)

- Steph

>
> Thanks,
> Greg

---

## #36891 — Gregory Beaver — 2008-04-09T04:48:03+00:00

URL: https://externals.io/message/36664#36891 · In reply to: #36682

Lars Strojny wrote:
> Hi Greg,
> 
> Am Samstag, den 29.03.2008, 17:58 -0500 schrieb Greg Beaver:
> [...]
> > […]
> 
> I mostly agree. Except that the whole argument chain applies to the
> current API. You mentioned mkdir("foo/bar/baz") triggering an error
> without the recursive argument, but Phar['foo/bar/baz'] does not.
> 
> [... Performance concerns ...]
> > […]
> 
> Could you estimate how hard that performance hit would be?

We're talking about a similar difference between iterating over an array
and using an ArrayIterator, which is to say about 20 times slower.  This
is due to the many extra opcodes for each offset retrieval, which then
calls an expensive method call to offsetGet(), creates the directory
inside the phar archive, writes to disk, and continues.  Because we are
writing to disk, without a not-at-all-obvious startBuffering() call,
with large archives, the performance difference could be 100 times
slower or more.

In addition, although Phar does support the idea of thinking of archives
as multi-dimensional arrays, this is an abstraction not supported by the
actual archive file formats.  For other examples, take a look at at
ext/zip.  There is no support for `opendir()` in the stream wrapper of
ext/zip because it requires the kind of path grepping that pecl/phar
does.  Internally, no archive format stores paths the way directories
are stored in a typical file system.  In other words, typically a
directory is a file that contains a list of other files or directories.
 This allows very quick navigation on a random access disk, but causes
extreme bloat inside an archive.  Files are stored as full paths in what
is the equivalent of a single-dimensional array.  In other words, the
truth of the matter is that phar's ArrayAccess API is a 100% accurate
reflection of the true structure of the actual archives we are
accessing, and the iteration supported is a convenience for simulating
regular file systems.  The API you describe actually layers an
artificial complexity over this design, and as a result requires much
more code complexity.

Let's also remember we're talking about phar creation, which is going to
be far less common than phar usage.  To use a phar, you don't even need
to know what phar *is* let alone what the API is.  One just includes the
phar archive, or adds it to include_path with a phar:// stream wrapper path.

I've spent a significant amount of time actually implementing the new
read-only implementation of offsetGet() in the past day, and although I
think this might be possible I am running into a weird segfault in SPL
on shutdown caused by a double destruction of the object.  In addition,
the patch is not even close to complete, as it does not support the
addition of ArrayAccess to PharFileInfo, which would be required in
order to implement what your RFC describes.  The complexity of the patch
is reaching the hundreds of lines and will be in the thousands of lines,
all to add syntactic sugar that is far more inefficient than the current
syntax - is this really a necessary feature?

As Elizabeth points out, most people are going to build their iterator
by creating a directory structure and then add the entire directory all
at once.  Use of ArrayAccess to write to the archive would be done for
one or two files at most.

When reading from a phar, there is no real incentive to use relative
paths outside of scanning a directory for files (to locate drivers or
plugins, for instance), and iterating over the entire archive is usually
reserved for searching for a particular file.  The ability to glob
through a phar archive would of course be the most ideal solution for
that approach (perhaps globiterator already works?  I haven't tried it).

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.

> Yes, this shouldn't be allowed while
> $phar['foo/bar/baz'] = new DirectoryIterator(); could be allowed.

I will simply add makeEmptyDir() a la zip, although it is too bad
ext/zip never thought to go with `mkdir()`.

> > […]
> 
> I like it. And much better, it does not contain the compression
> format :-)

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

This would also solve a subtle but important problem with the current
conversion API where intermediary files are lying around.

> > […]
> 
> Wouldn't compressFiles() be fine?

Probably, especially if we remove the compress() method.

> > […]
> 
> I would not care so much for phar, if it's inclusion in core has not
> been targeted. 
> 
> > […]
> 
> On http://pecl.php.net/package/phar Marcus is listed as one of the two
> project leads. I've talked to him *before* writing the RFC on friday. So
> this accusation fizzles out. I sort of understand your irritation, but
> from my point of view improvement takes place constantly. In the time
> Phar was discussed on pecl-dev I was not subscribed to it, now I am. So
> the next time I will come up earlier most presumably.

My point is you should contact the leads (plural) of a project.  I've
been breaking my back implementing changes and features requested as far
back as last May, and finally reached a fully functional solution a few
weeks ago.  Your RFC would completely redo a large portion of the API.
I think it is a common courtesy to be consulted prior to publishing the
RFC for the simple reason that I am the one writing the majority of the
code and implementing these changes.

> > […]
> 
> 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).

Let me also be clear: the largest priority right now is on making
pecl/phar work with xdebug, APC and on increasing stability, test
coverage, and ensuring we work out all OS X issues since none of the
devs have an OS X platform to test phar on.

Greg

---

## #36893 — Pierre Joye — 2008-04-09T06:57:50+00:00

URL: https://externals.io/message/36664#36893 · In reply to: #36891

Hi,



>   For other examples, take a look at at
>  ext/zip.  There is no support for `opendir()` in the stream wrapper of
>  ext/zip because it requires the kind of path grepping that pecl/phar
>  does.

That's easy to add and will be available at some point, that was
simply not needed and nobody ever requested this feature. Extract or
add using a pattern is however much more useful. It is already
partially supported.

> > […]
>
>  I will simply add makeEmptyDir() a la zip, although it is too bad
>  ext/zip never thought to go with `mkdir()`.

I thought about mkdir but it is a bad name as it implies a shell
command. makeEmptyDir while being longer is self explaining. I also
asked before choosing this name, as far as I remember nobody proposed
mkdir either.

Cheers,
-- 
Pierre
http://blog.thepimp.net | http://www.libgd.org

---

## #36899 — Steph Fox — 2008-04-09T09:30:05+00:00

URL: https://externals.io/message/36664#36899 · In reply to: #36891

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.

> 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.

> 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?

> > […]
>
> 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.)

- Steph

---

## #36901 — Gregory Beaver — 2008-04-09T12:44:45+00:00

URL: https://externals.io/message/36664#36901 · In reply to: #36893

Pierre Joye wrote:
> Hi,
>
> 
>
>   
> > […]
>
> That's easy to add and will be available at some point, that was
> simply not needed and nobody ever requested this feature. Extract or
> add using a pattern is however much more useful. It is already
> partially supported.
Yes, I agree.
> > […]
>
> I thought about mkdir but it is a bad name as it implies a shell
> command. makeEmptyDir while being longer is self explaining. I also
> asked before choosing this name, as far as I remember nobody proposed
> mkdir either.

I'm fine with makeEmptyDir(), I was just thinking out loud :)

This wasn't intended to be a referendum on ext/zip, just to be clear.

Greg

---

## #36906 — Gregory Beaver — 2008-04-09T15:41:36+00:00

URL: https://externals.io/message/36664#36906 · In reply to: #36899

Steph Fox wrote:
> Hi Greg,
> 
> > […]
> 
> 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.

> > […]
> 
> 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.

> > […]
> 
> 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).

> > […]
> 
> 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