upgrade to 1.4.0 stable
http://pear.php.net/package/Archive_Tar
1.4.0 stable
* Add support for PHP 7
* Drop support for PHP 4
* Add visibility declarations to methods and properties
Apart from the upgrades done to Tar.php at pear.net.net, there are known platform issues with the current versions for Drupal (system.tar.inc in Drupal 7 and ArchiveTar in Drupal 8):
- On Windows, path names are translated wrongly and the result is that e.g. the backup and migrate module can generate backup files with wrong paths.
- On Ubuntu 14.04 LTS (Trusty Tahr) 32 bit, gzopen() etc. are not defined. This results in White Screen Of Death (WSOD) for e.g. import/export configuration, install module and backup and migrate.
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff.txt | 900 bytes | David_Rothstein |
#57 | update_system_tar_inc-d7-1414508-57.patch | 128.5 KB | David_Rothstein |
#53 | update_system_tar_inc-d7-1414508-53.patch | 128.54 KB | KimNyholm |
Comments
Comment #2
droplet CreditAttribution: droplet commentedComment #4
droplet CreditAttribution: droplet commentedComment #5
droplet CreditAttribution: droplet commentedlatest version: 1.3.10 (stable) was released on 2012-04-10 (Changelog)
back to need work.
Comment #6
kid_icarus CreditAttribution: kid_icarus commentedWell, here's my attempt at upgrading to 1.3.10, I hope I got it right :)
Comment #8
kid_icarus CreditAttribution: kid_icarus commentedWhoops, looks like I forgot to add in a __destruct().
Comment #9.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #10
hgurol CreditAttribution: hgurol commentedThe 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).
Comment #11
hgurol CreditAttribution: hgurol commentedOkay, 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.
Comment #12
mgiffordComment #13
mglamanRe-roll against 8.0.x HEAD
Comment #15
mglamanForgot to refactor usage of class with class name change (to match package - ArchiveTar -> Archive_Tar)
Comment #17
mglamanFirst patch made these changes
However the method _PEAR(); hasn't been implemented yet. Updated patch with fix.
Comment #18
mgiffordSeems to satisfy the bots.
What else needs to happen to mark this RTBC?
Comment #19
droplet CreditAttribution: droplet commentedThis is 1.3.13 stable.
Just need to make sure:
- all @unlink -> @drupal_unlink
- // comment out PEAR function: $this->PEAR()
Comment #20
mglamandroplet, thanks for catch!
Comment #21
mgiffordAny reason why this couldn't just be installed here http://simplytest.me/project/drupal/8.0.x?patch[]=https://www.drupal.org...
Comment #22
star-szrBecause the patch no longer applies. It will need a lot of rerolls unless we keep the class name the same.
Changing the class name seems a bit out of scope. Why not keep it as ArchiveTar?
Comment #23
droplet CreditAttribution: droplet commentedreroll
Comment #24
mgiffordThat 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"
Comment #25
star-szrMy main point was it was called ArchiveTar before so I think we were already renaming it, but I could be wrong on that.
Comment #26
droplet CreditAttribution: droplet commentedMake sense. Removed renaming.
Comment #29
droplet CreditAttribution: droplet commentedComment #30
star-szrI think this part maybe needs to be reassessed/rolled back.
Overall this seems hard to review :/ not sure the best way forward.
Comment #31
alexpottRegarding the deprecation of dl - I've checked following code in PHP7...
So I think we are okay here - http://3v4l.org/XgqCt
Comment #32
kim.pepperI suggest we move this to 8.1.x, use composer to load this library and refactor the Drupal specific code out.
Tagging PHP 7.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSince 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?)
Comment #34
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedFunction _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.Comment #35
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedI 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.
Comment #36
droplet CreditAttribution: droplet commented@KimNyholm,
Can you submit your concern to upstream?
https://github.com/pear/Archive_Tar
We also have to update to latest version.
Comment #37
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedI 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
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).
Comment #38
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedI 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.
Comment #39
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedComment #40
mgiffordWhat'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?
Comment #41
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedThis 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.
Comment #42
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedOops, 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.
Comment #43
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedThe 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.
Comment #44
klausitrailing space, also elsewhere.
What, only do significant changes? You mean not significant changes.?
should be "The following changes have been done:"
All comments should start upper case and end with a dot. Please check all your comments.
Otherwise makes sense to me.
Comment #45
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedComments updated and trailing whitespaces removed.
Comment #46
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedComment #47
mgiffordThanks @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.
Comment #48
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedFor 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.
Comment #49
mgiffordOk, this looks good to me then. I've done some testing too. Haven't found any problems yet.
Comment #50
webchickWe should be able to commit this shortly after tagging beta 15.
Comment #51
webchickCommitted and pushed to 8.0.x. Thanks!
Back to 7.x.
Comment #53
KimNyholm CreditAttribution: KimNyholm as a volunteer commentedAttached 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.
Comment #54
droplet CreditAttribution: droplet commentedCool. It's fully ported back to D7 ( and removed namespace in D8 )
Comment #55
andypostNew related for 8.x #2610984: Add Archive Tar via Composer, with BC shim
Comment #57
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
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.
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.
Comment #59
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDoesn't look real. I added a new test.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!