Lars Strojny wrote:
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.
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.
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?
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.
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
You're right, chock this up to jet lag on my part. :)
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.
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)?
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.
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?
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?
[...]
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
