update system.tar.inc to latest Archive_Tar (Pear)

Well, here's my attempt at upgrading to 1.3.10, I hope I got it right :)

Whoops, looks like I forgot to add in a __destruct().

The latest version of Archive_tar is 1.3.11
It fixes a serious bug which surfaces with php 5.5
https://pear.php.net/bugs/bug.php?id=19746

and I strongly feel that I am hitting that bug on D7 with ubuntu 14.04 (apache 2.4 & php 5.5).

Okay, some more details.
Running Ubuntu 14.04 (apache 2.4 & php 5.5).
Fresh installation of latest D7 & D8.
Module or Theme installation though the admin interface or updates do not work if the file extension is tar.gz.
If I try to install a module or theme with the zip extension does work. For updates, I do not have that choice.
When I try to install a module (any module) with tar.gz extension, 2 folders are created in my /tmp folder.
One is "update-cache" which has the module file inside and "update-extraction-" which is empty.
Obviously, something wrong is going on during the extraction, which fails.
Nothing on the logs, even though php is set for reporting all errors and apache logging level is set to debug; nothing.
I strongly believe, its due to the outdated Archive_Tar library.
This is not a task but a bug and a major one I believe.

This applies to both D7 and D8.

Re-roll against 8.0.x HEAD

Forgot to refactor usage of class with class name change (to match package - ArchiveTar -> Archive_Tar)

First patch made these changes

+//            @unlink($this->_temp_tarname);
             @drupal_unlink($this->_temp_tarname);
-//        $this->_PEAR();
+        $this->_PEAR();

However the method _PEAR(); hasn't been implemented yet. Updated patch with fix.

Seems to satisfy the bots.

What else needs to happen to mark this RTBC?

droplet, thanks for catch!

Any reason why this couldn't just be installed here http://simplytest.me/project/drupal/8.0.x?patch[]=https://www.drupal.org...

Because the patch no longer applies. It will need a lot of rerolls unless we keep the class name the same.

+++ b/core/lib/Drupal/Core/Archiver/Tar.php
@@ -15,7 +15,7 @@ class Tar implements ArchiverInterface {
-   * @var \Drupal\Core\Archiver\ArchiveTar
+   * @var \Drupal\Core\Archiver\Archive_Tar

@@ -30,7 +30,7 @@ class Tar implements ArchiverInterface {
-    $this->tar = new ArchiveTar($file_path);
+    $this->tar = new Archive_Tar($file_path);

Changing the class name seems a bit out of scope. Why not keep it as ArchiveTar?

That latest patch works well. Thanks @droplet!

Did you address @Cottser's concerns about the class name? Not sure if there is any way to avoid it. A problem with "Proudly built elsewhere"

My main point was it was called ArchiveTar before so I think we were already renaming it, but I could be wrong on that.

+++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
@@ -131,92 +159,89 @@ function __construct($p_tarname, $p_compress = null)
-//                PEAR::loadExtension($extname);
-                $this->loadExtension($extname);
+                PEAR::loadExtension($extname);
...
-    /**
-    * OS independent PHP extension load. Remember to take care
-    * on the correct extension name for case sensitive OSes.
-    * The function is the copy of PEAR::loadExtension().
-    *
-    * @param string $ext The extension name
-    * @return bool Success or not on the dl() call
-    */
-    function loadExtension($ext)
-    {
-        if (!extension_loaded($ext)) {
-            // if either returns true dl() will produce a FATAL error, stop that
-            if ((ini_get('enable_dl') != 1) || (ini_get('safe_mode') == 1)) {
-                return false;
-            }
-
-            if (OS_WINDOWS) {
-                $suffix = '.dll';
-            } elseif (PHP_OS == 'HP-UX') {
-                $suffix = '.sl';
-            } elseif (PHP_OS == 'AIX') {
-                $suffix = '.a';
-            } elseif (PHP_OS == 'OSX') {
-                $suffix = '.bundle';
-            } else {
-                $suffix = '.so';
-            }
-
-            return @dl('php_'.$ext.$suffix) || @dl($ext.$suffix);
-        }
-
-        return true;
-    }

I think this part maybe needs to be reassessed/rolled back.

Overall this seems hard to review :/ not sure the best way forward.

Regarding the deprecation of dl - I've checked following code in PHP7...

php -r "var_dump(ini_get('enable_dl'));"
string(0) ""

So I think we are okay here - http://3v4l.org/XgqCt

I suggest we move this to 8.1.x, use composer to load this library and refactor the Drupal specific code out.

Tagging PHP 7.

Since this is fixing a bug (and needed in Drupal 7 also) let's put this back to 8.0.x. (Or alternatively, I guess we could reopen #1301376: undefined constant OS_WINDOWS Archive_Tar->loadExtension() with the aim of doing a more limited fix there?)

Function _translateWinPath() does also reference OS_WINDOWS. Since OS_WINDOWS is not defined in all Windows/PHP versions the function is unreliable. I am not sure we can assume that ':' (colon) and '\' (backslash) is not used in nix context, but if the assumption is valid, the most easy fix it is to remove the condition if (defined('OS_WINDOWS') && OS_WINDOWS) and always do the translation. In that case, the translation won't harm and the performance penalty is low.

I have removed OS_WINDOWS as explained in #34. I have also removed one further instance of OS_WINDOWS, which has been re-introduced between 8.0.x and patch #29.

If attached is ok, I can backport to Drupal 7, however I am not sure under which issue # this shall be done.

I have checked at https://pear.php.net/package/PEAR and they have defined OS_WINDOWS in pear.php 1.9.5 (last stable) with the following fragment

if (substr(PHP_OS, 0, 3) == 'WIN') {
    define('OS_WINDOWS', true);
    define('OS_UNIX',    false);
    define('PEAR_OS',    'Windows');
} else {
    define('OS_WINDOWS', false);
    define('OS_UNIX',    true);
    define('PEAR_OS',    'Unix'); // blatant assumption
}

By documentation PHP_OS is the OS where PHP is build, but since above is accepted, I assume that you normally build on the platform you target.

Not sure how to proceed. Either keep patch #35, make new patch look like above or ... ? And then merge with updates in Archive_Tar (last stable 1.3.16).

I have dropped patch #35 and instead done the change as in pear.php and also included the changes from tar.php 1.3.13 to 1.3.16.

What's the best way to do manual testing on this patch? Bot likes it. Certainly looks like it's using a more recent version of Tar.php (2012-04-05).

The current release though is now 1.4.0 (stable) released just over a week ago. Seems like it would make sense to use that, no?

This patch updates to Tar.php 1.4.0 stable. Lots of changes have been introduced in Tar.php since 1.3.16 (contained in patch #38). It seems to me, that the most safe port, is to redo all drupal specific changes as in attached patch #41. I have tested with success a backport to Drupal 7.38 of patch #41 on Windows 7 with AMPPS and PHP 5.5 with the Backup and Migrate module. Since I have not been working with this issue from the beginning, I may have overlooked something, so comments are appreciated.

Oops, the reference to PEAR::loadExtension was not removed. As mentioned in #30 the reintroduction of this should be rolled back. It was introduced in #486558: Add Archive Tar to Drupal and I have seen no reference to why it should be changed.
I have reproduced the issue in #11 which looks to be the same as Drupal 7 on Ubuntu 14.04 : can't install modules with wsod. A backport to Drupal 7 of patch #42 fixes this.

The way Drupal namespace was implemented broke functionality. This is fixed now. I have reproduced #2310183: Config export fails on Ubuntu 14.04 on Drupal 8.0.x and tested with success with patch #43.

1. +++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
   @@ -30,83 +30,145 @@
   + * This file origin is Tar.php, release 1.4.0 (stable) with some code ¶
   ...
   + *  changed $this->error_object = &$this->raiseError($p_message); ¶
   ...
   + ¶
   

   trailing space, also elsewhere.

2. +++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
   @@ -30,83 +30,145 @@
   + * Only do significant changes to this file, this will simplify
   + * future upgrades from pear.com.
   

   What, only do significant changes? You mean not significant changes.?

3. +++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
   @@ -30,83 +30,145 @@
   + * The following changes has been done
   

   should be "The following changes have been done:"

4. +++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
   @@ -30,83 +30,145 @@
   +// drupal addition
   

   All comments should start upper case and end with a dot. Please check all your comments.

Otherwise makes sense to me.

Comments updated and trailing whitespaces removed.

Thanks @KimNyholm - just testing if the bots like it in PHP 7.

Thanks for updating it to 1.4.0. I confirmed that this works when testing it manually. Not sure what manual testing is required though.

For information, I have done the below manual tests with the update and they worked for me. Before the update, all tests failed on Ubuntu Trusty 32 bit and the 'Backup and Migrate' test failed on Windows 7.

• Drupal 8.0.x: tested 'Admin Import Module' and 'Config Export' on Ubuntu Trusty both 32 and 64 bit.
• Drupal 7.38: backported and tested 'Admin Import Module' and 'Backup and Migrate' on Ubuntu Trusty both 32 and 64 bit and Windows 7.

Ok, this looks good to me then. I've done some testing too. Haven't found any problems yet.

We should be able to commit this shortly after tagging beta 15.

Committed and pushed to 8.0.x. Thanks!

Back to 7.x.

Attached is a backport to Drupal 7 of ArchiveTar.php. This file is called system.tar.inc in Drupal 7. Changes done to backport are:
- removed namespace
- renaming the class
I have tested the backport patch #53 with Install Module and Backup and Migrate on Drupal 7.x with success on Ubuntu Trusty (both 32 and 64 bit) and Windows 7.

New related for 8.x #2610984: Add Archive Tar via Composer, with BC shim

I reviewed this to the extent possible, and overall it looks good to me (it's a big update, but a significant percentage of it is documentation and code style changes).

Thanks for the manual testing, which also gives confidence in committing this to Drupal 7.

1.         throw new Exception($p_message);
   +        // Drupal change $this->error_object = $this->raiseError($p_message).
   +        throw new \Exception($p_message);
   

   We can't do this since Drupal 7 still supports PHP 5.2. I've fixed it in the attached patch. Since this is such a small change, I'm leaving the issue at RTBC and will plan to commit it for the upcoming release as long as it passes tests.

2. -        // Drupal integration.
   -        // Changed the code to use drupal_mkdir() instead of mkdir().
   -        if (!@drupal_mkdir($p_dir, 0777)) {
   +        if (!@mkdir($p_dir, 0777)) {
   

   I was a little worried about this, but as far as I can see from https://api.drupal.org/api/drupal/includes!file.inc/function/drupal_mkdir/7 it should be fine not to use drupal_mkdir() here.

Doesn't look real. I added a new test.

Committed to 7.x - thanks!