Hello Benjamin,
Thursday, March 27, 2008, 8:58:47 PM, you wrote:
> Hi Marcus,
>>> First thing: yes i fully understand what the code is doing but i
>>> still
>>> think that it doesn't need to be so "hackish".
>>
>> I wouldn't call it hackish. I'd eventually call it new to people that
>> haven't used the new PHP 5.0 features yet.
> I used PHP 5 when it had namespaces the first time ;) So i am not new
> to PHP5
> features, that's not the point.
>>
>>> One thing is that i think there is no point in using ArrayAccess
>>> here,
>>> in my oppinion ArrayAccess is great to hack stuff but it doesn't
>>> belong in such (maybe soon?) core functionality.
>>
>> So you are basically saying we should remove ArrayAccess, right?
> In my personal oppinion: yes. There is no need for array syntax if you
> provide
> a OO api, why not setContent() or something else? There is no real
> difference
> in the implementation, right?
Of course not as you can use offsetSet() already today.
>>
>>> The second thing that looks weird to me is that the setter
>>> (offsetSet)
>>> sets the file content but the getter (offsetGet) retrieves a file
>>> object.
>>
>> Right now the setter takes a resource or a string. Maybe we can
>> allow an
>> SplFileInfo instance too. But note that this is even more wierd as
>> we then
>> would need to convert this instance into another instance and still
>> only
>> transfer the contents.
> The point is that the getter does something completely different from
> the
> setter. The getter returns a file object but the setter sets the content
> of the file, this is just not consistent.
To be precise, the getter returns a SplInfoObject per default. That is not
a file object.
>>
>>
>>> What about changing this into something more OO like this (just a
>>> proposal):
>>
>>> $p = new Phar('coollibrary.phar');
>>
>>> /* What about creating a non-static pendant to canWrite()?
>>> Maybe there is an archive file that has only read permissions on
>>> the filesystem or
>>> phar will be able to generate readonly-archives later? (Might be
>>> interesting for
>>> companies that want to provide readonly and signed archives for
>>> the customers)
>>
>> We can already create readonly archives.
> And how do i check if the archive is readonly? canWrite() just tells me
> if the php.ini setting is enabled, right?
Hmm, we have is_writeable() which takes the full path (phar+index), so this
is probbaly a good addition. But we also have ArrayAccess/offsetGet return
a SplFileInfo object which supports isWritable(). So what else do you need?
Or is there something briken, which actually might be the case.
>> And obviously you cannot create
>> signed archives where only selected entries are readonly.
> I don't see where this has to do with the point of my mail but i think
> it could easily achieved with
> $p->getFile('data/hugefile.dat')->canWrite()
>>
>>
>>> if ($p->canWrite()) {
>>> // Create a file instance
>>> $hugeFile = $p->createFile();
>>
>>> $fp = fopen('hugefile.dat', 'rb');
>>
>>> // Set the content
>>> $hugeFile->setContent($fp); (or maybe even
>>> setContentResourceHandle(...))
>>> if (Phar::canCompress()) {
>>> /* how is the compression implemented? through streamfilters?
>>> than how about a ->setCompression('bzip')
>>> that internally resolves to the bzip.(de)compress:// stuff?
>>> $p['data/hugefile.dat']->setCompressedGZ();
>>> }
>>
>> Hi? This makes no sense whatsover. Because:
>> Ther is no connection between your stuff.
>> - $hugeFile would be an anonymous entry in your file
> correct, it would be just some "random" file object related to the phar
> archive until i use addFile() like below, if you need this relation one
> could provide something like
> $p->createFile('data/hugefile.dat');
> $p->setContent('...');
That is better as this would go well with the internal structures. Adding
this sounds fine with me. Assuming a) createFile() returns the file info
object and you work on that or b) setContent takes the file name, too which
would be offsetSet() then (no aliases for method names as that would be
confusing when overloading comes into play).
a)
$fi = $phar->createEntry('data.dat');
$fi->file_put_contents($data); // this is a change in SPL however
b)
$phar->createFile('data.dat');
$phar->setContent('data.dat', $data);
same as:
$phar->offsetSet('data.dat', $data);
the second is shorter, faster and clearer. As in your example the phar
object would have to keep state of a kind of dangling entry object. That
might get changed when there is something between createFile() and
setContent(). This will never be supported as it is extremely dangerous and
error prone.
>> - and then wher is $p['data/hugefile.dat'] coming from?
>> - and did you not just write ArrayAccess is wrong?
> sorry, my fault - it should be $hugeFile
>>> // add the file to the archive
>>> $p->addFile($hugeFile, 'data/hugefile.dat');
>>
>> Well I prefer the other order. Oh and then that is just what
>> $p->offsetSet('data/hugefile.dat', $fp); does already.
> Do you think that is well named? It's what the ArrayAccess interface
> prescribes but wouldn't a $p->setFile(), ->addFile() describe better
> what's happening? I mean we're talking about OO here.
>>>
>>> // another option would be to pass the file path to the
>>> createFile() method and
>>> // implicitely create it in the archive
>>> $indexPhp = $p->createFile('index.php');
>>> $indexPhp->setContent(...);
>>
>> Well right now, you'd do $indexPhp = $p['index.php']; $indexPhp->...
> Yep, array syntax? but it's an object right? That is what i mean with
> "hackish" - no
> need for array syntax if i work with objects. I still believe that
> ArrayAccess has
> it's advantages but IMHO not in this case.
But after all a phar object is a container for files. And actually it is an
associative container for files. And each file can be seen as the filename
which is the key plus its data as string or just as a resource. This is
what we try to do and why we chose ArrayAccess semantics. Yet adding one or
the other method probably helps.
Best regards,
Marcus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
