Create a common interface for Archive operations so we can handle .zip, .tar.gz.

@Crell: Very nice start!

I agree about TPOI, and I think it is potentially useful here, but not the way it is implemented. I agree we could do this instead of an abstract class because I don't see how a Template pattern would benefit us in this case (although we will likely find out later) :)

Gripes

1. The Interface you've defined is too exhaustive. Most archive providers will not support all those methods, don't force them to.

2. The Interface mimicking Archive_Tar is not necessarily a good thing. We should define a logical interface for us, not map it to Archive_Tar for the sake of convenience. That interface is full of wtfs. I still suggest the interface I defined above. (extract, list, create) - we can always extend this later.

3. There is absolutely no need to define 3 different classes for handling gz vs bz2 vs tar. These are all handled by Archive_Tar natively, with no flags passed in. There is just one we need to define for tar: ArchiverTar and it will wrap Archive_Tar. I think we should build 3:

ArchiveZip
ArchiveTar
ArchiveRar* Maybe

I can't think of any other archive formats that are needed here.

+++ modules/system/system.archiver.inc	2009-10-15 03:38:26 +0000
@@ -0,0 +1,39 @@
+/**
+ * Archiver for .tar files.
+ */
+class ArchiverTar extends Archive_Tar implements ArchiverInterface {
+  public function __construct($filename) {
+    parent::__construct($filename);
+  }
+}
+
+/**
+ * Archiver for .tar.gz files.
+ */
+class ArchiverTarGz extends Archive_Tar implements ArchiverInterface {
+  public function __construct($filename) {
+    parent::__construct($filename, 'gz');
+  }
+}
+

Why do this, it's just bloat IMO, Archive_Tar will take any of those without the flag, we already do it.

If you look at the Archive_Tar class, you can see this:

if (($p_compress === null) || ($p_compress == '')) {
            if (@file_exists($p_tarname)) {
                if ($fp = @fopen($p_tarname, "rb")) {
                    // look for gzip magic cookie
                    $data = fread($fp, 2);
                    fclose($fp);
<code>

Also, we're wrapping it, so if we want to provide a flag for whatever reason (can't think of any) we can.

<code>
+++ includes/common.inc	2009-10-15 03:24:12 +0000
@@ -5797,3 +5797,47 @@ function xmlrpc($url) {
+    if (in_array($extension, $implementation['extensions'])) {
+      return new $implementation['class']($file);
+    }

This assumes that extensions are the only way to tell what class should handle it. Should call a static detection method in all of the implementers of ArchiverInterface until one says it can handle it. More flexibility. If you want to make a base class with a convenience method for extensions, go for it. Archive_tar already has one btw: _is_Archive().

Summary
Interfaces are useful in many applications, and this is not a misuse. I just think building separate classes for all tar compression formats to show off interfaces with no other reason is overkill.

My todo list here is:

a) simplify. One implementer of the interface for now: ArchiverTar().

b) flexible. All implementers need to implement ::supports($file_path); @return bool.

c) suitable. Whack the Archive_Tar interface, make it better for our needs, follow our coding standards (can't believe I just said that) and easier to plug into and put it in ArchiverTar(), and be done with it.

d) ready. Build ArchiverZip. I think applying this pattern without building out a basic zip implementation is a mistake. Even if it might have low-lying bugs in it, at least is there and proves the usefulness. I recommend gutting http://pear.php.net/package/Archive_Zip.

Okay, hope that helps.

-J

In our factory, I think we should make sure the stream is local no Archiving tools we use are going to operate on remote streams AFAIK. Archive_Tar (or rather zlib) doesn't.

drupal_real_path() translates a stream to a local path (if it is local).
if it is not, copy() should be able to copy it to a local temporary location.

Of course this means copying it back if it is modified, and additional complexity. At a first go, I think throwin an exception saying "You can't work with remote archives is good enough", but the drupal_real_path will need to be in the factory to check that.

-J

+++ includes/common.inc	2009-10-15 08:10:09 +0000
@@ -5797,3 +5797,53 @@ function xmlrpc($url) {
+     if (strrpos($file, $extension) === strlen($file) - strlen($ext)) {
+       return new $implementation['class']($file);
+     }

This probably doesn't matter, but it doesn't check for the occurrence of a . in the filename. So blacktar.gz will match.

Also, "myzip" would match of course.

There are also a lot of whitespace changes. I don't know if you want to deal with them here or not. Either way, without testing it RTBC for code, docs and structure.

Okay, this looks good to me, and will be a nice addition to Plugin manager so that it can handle Zip files, etc

#605408: Provide an implementation of Archiver_Interface to handle .zip files

I think this is also very unit testable, at least the factory logic. Testing the performance of the actual archive operations is kinda silly in the case of Archive_Tar, but could be done for others, so:

#605412: ArchiverInterface needs unit tests.

->list is a system construct :)

We tried that first, but it's a parse error.

Okay, here it is.

White space killing spree!

Another follow up.

The error handling is bad in archiver_get_archiver since we can't operate on remote archives because zlib, etc won't support them. This change makes that clear, also improves the docs a little bit to indicate a return of FALSE on failure.

This is actually blocking implementation of
#605918: Port update manager to use the new Archiver class, not directly Archive_Tar (the original motivation)

because passing temproary://update-cache/whatever.tar.gz causes it to bork now.

I thought about that as I wrote it, but copped to "Well, we don't have any good classifications, so implementing one here is not worth it."

I don't want to hold up this patch for this issue, although it is important to get right in D8. I think in this case, it is not really an ArchiverException, because it doesn't really have much to do with Archiving, it's something like a "FileAccessException", but that just sounds too meta and weird. I guess Archiver would be okay... perhaps a BadFileStreamException, for cases were the wrong type of stream is passed in? But then it isn't implemented anywhere in the stream handling operations, so this would be weird to make a precedent here.

Either way, let's just take a call on it. I'd just leave it as is, but if you want to throw a type on it, just re-roll and let's RTBC this so we can progress on implementing it.

eh? Permission?