Problem/Motivation

ArchiveTar is a code from another project in core/lib/Drupal/Core/Archiver/ArchiveTar.php - but we can get it from composer now - see https://packagist.org/packages/pear/archive_tar

Proposed resolution

Do it and alias the core class to the vendor version and deprecate the core class.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Previously, Drupal packaged a copy of the PEAR Archive_Tar library in a Drupal core namespace. In 8.7, this has been deprecated and replaced with a proper Composer dependency on this library. The dependency has also been updated to version 1.4.6.

CommentFileSizeAuthor
#17 3026588-17.patch96.5 KBalexpott
#17 6-17-interdiff.txt702 bytesalexpott
#6 3026588-6.patch95.82 KBalexpott
#6 3-6-interdiff.txt5.49 KBalexpott
#3 3026588-2.patch100.35 KBalexpott

Comments

alexpott created an issue. See original summary.

cilefen’s picture

cilefen commented

Task?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3026588-2.patch100.35 KB

Here's a start on this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Bug report » Task

Indeed a task this is.

Status: Needs review » Needs work

The last submitted patch, 3: 3026588-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3-6-interdiff.txt5.49 KB
new 3026588-6.patch95.82 KB

Ho hum we changed the error handling to use exceptions.... so this works.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented 
+++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
@@ -1,2517 +1,24 @@
+class ArchiveTar extends \Archive_Tar {

Would it make sense to mark this @internal? \Drupal\Core\Archiver\Tar is already acting as an adapter, kind of. This would allow us to avoid exposing the "actual" implementation (and PEAR) as a Drupal API.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented 
+++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
@@ -1,2517 +1,24 @@
- *  Changed all calls of unlink() to drupal_unlink().

One more thing, we are no longer using \Drupal\Core\File\FileSystem::unlink() here: is this ok?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@plach I'm not sure it would make sense for it to be internal...

+++ b/core/modules/config/src/Controller/ConfigController.php
@@ -89,7 +90,7 @@ public function __construct(StorageInterface $target_storage, StorageInterface $
-    $archiver = new \Archive_Tar(file_directory_temp() . '/config.tar.gz', 'gz');
+    $archiver = new ArchiveTar(file_directory_temp() . '/config.tar.gz', 'gz');

We use it directly in controllers and forms that's not internal. We could have a follow-up to using the adapter instead but these things only support tar so I'm not sure. What would make sense is for this to final as there is no use-case to extend this class - you should extend the PEAR class but that is a contentious issue atm and does not need to be broached here. Plus we couldn't make this final in minor or patch release so moot atm.

Our unlink is specialised for windows - this is really really old code. And speaks to environment and core PHP issues rather than Drupal issues but also more modern PHP file system abstractions likehttps://github.com/symfony/filesystem don't do this for Windows and I really doubt that we need to. Also unlink Drupal, Symfony code is actually tested on Windows - see https://github.com/symfony/symfony/blob/master/.appveyor.yml

drupal_unlink() was added in #443286-25: Windows File Handling: International characters break file handling, permissions don't translate with little thought to the costs of maintaining this. There is also a comment saying we should have a link to the PHP bug - but unfortunately this is not there. There is also a comment saying that the fix should be filed upstream to ArchiveTar but it does not appear to have been. But also we have alot of calls to unlink already in vendor - for example in Twig and PhpUnit both of which are tested on Windows regularly outside of the Drupal and they don't contain this Windows work-around.

Personally I think it is fine to remove these calls and use the library as is apart from the move to exceptions.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re the using the adapter - I think we could think about filing a follow-up for making all the usages in core use the adapter instead of the ArchiveTar directly.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

[...] Personally I think it is fine to remove these calls and use the library as is apart from the move to exceptions.

Compelling argument :)

Re the using the adapter - I think we could think about filing a follow-up for making all the usages in core use the adapter instead of the ArchiveTar directly.

Ok, a follow-up works for me, given that people might already be using ArchiveTar in contrib/custom code. But since now we are formally bringing in another external dependency (pear/*), I was hoping to find a way to decouple ourselves from it. Do we have a way to state that only ArchiveTar is a Drupal API, not any of its parents? Could we make ArchiveTar wrap \Archive_Tar instead of extending it?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@plach For me we should file an issue to make all the controller / form could use our adapter... and then we can consider marking our ArchiveTar @internal in Drupal 9

I've tried to discovered if the PHP issue that caused us to create drupal_unlink() has been fixed. I found https://bugs.php.net/bug.php?id=42291&thanks=6 so far.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

For me we should file an issue to make all the controller / form could use our adapter... and then we can consider marking our ArchiveTar @internal in Drupal 9

Sounds good

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Created the follow-up - #3027290: Use \Drupal\Core\Archiver\Tar instead of \Drupal\Core\Archiver\ArchiveTar in controllers are forms

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

After the recent security issues with Pear and in particular pear.net, are we sure that going forward Pear is what we want to continue with?

I agree we should be offloading this code to another project but is Pear what we'd pick in 2019?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The code for pear projects has been on github for a long time, so installing them with composer is not related to that security issue in any way?

And no, maybe we wouldn't pick this library in 2019, but switching it with something else is a much bigger task than changing how we include it?

One thing I'm wondering is how much extra dependencies/code this results in, as it depends on the pear base stuff..

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 6-17-interdiff.txt702 bytes
new 3026588-17.patch96.5 KB

Here's what is included:

Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 4 installs, 0 updates, 0 removals
  - Installing pear/pear_exception (v1.0.0): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing pear/console_getopt (v1.4.1): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing pear/pear-core-minimal (v1.10.7): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing pear/archive_tar (1.4.5): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
pear/archive_tar suggests installing ext-xz (Lzma2 compression support.)
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess

So what does this mean for core?

It means when we load ArchiveTar we include vendor/pear/pear-core-minimal/src/PEAR.php - some defines in main and it does @ini_set('track_errors', true); and this is the only new code loaded. This code also defines two classes - Pear and PEAR_Error and function _PEAR_call_destructors(). We potentially call PEAR::loadExtension() but that is only if an extension is not loaded which will be rare and this code is very simple and we had copied it before. I don't think we have to much extra here.

All told:

  • pear/pear_exception contains 1 class
  • pear/console_getopt contains 1 class
  • pear/pear-core-minimal contains 5 classes
  • pear/archive_tar contains 1 class

Whilst thinking about this I realised we need to remove the tests.

sam152’s picture

sam152 commented

Which tests would be removed? I came here from #3026470: ArchiveTar is throwing fatal error and was going to suggest adding some basic integration tests for adding some files to an archive, extracting them and asserting the contents are the same.

I am working on a feature integrated into webform (like in the original issue), which zips up submissions, writes them to a file and then deletes the originals. The result for me was, a corrupt archive with uncatchable warnings and deleted submissions (not in production yet, thankfully :)).

I think I'll have to make my code a little more defensive and possibly attempt to extract the generated archive to assert it's valid before deleting the submissions, but I think that would have been avoided if core had tests for the class in the first place?

I realise we're not responsible for testing our upstream, but especially with conversations about superseding it and the fact that a bug snuck in before, it might be worth it?

Edit: adding a check which simply reads the contents of the archive was enough to catch the error in 8.6.7.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 3026588-17.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

Random JS test fails unrelated to this change.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Parent issue: » #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release
Related issues: +#2610984: Add Archive Tar via Composer, with BC shim, +#2571965: [meta] Fix PHP coding standards in core, stage 1

Marked #2610984: Add Archive Tar via Composer, with BC shim a duplicate, though actually this one here was created later.

alexpott credited Crell.

alexpott credited andypost.

alexpott credited catch.

alexpott credited hussainweb.

alexpott credited jibran.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Pancho thanks for spotting that and doing the issue management. Crediting the contributors from that issue.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Do we need a change record for adding new library?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@jibran I don't think so because what is changing? :) No one should need to make any changes due to this.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

@jibran:

I don't think we should advertise PEAR as a newly available library, if that's what you meant. On the contrary I think we should have a policy in place that our vendor libraries should not be considered Drupal APIs and should not be covered by our BC policy. However discussing this will require a new issue, so I will stop ranting about that now ;)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

I agree we don't need a change record.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 3026588-17.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

@Sam152 the tests being removed are the ones shipped with the library, we always remove those on the basis that upstream projects manage running their own test suites.

Committed 53c6cc8 and pushed to 8.7.x. Thanks!

  • catch committed 53c6cc8 on 8.7.x 
    Issue #3026588 by alexpott, plach, jibran, Berdir, larowlan, Sam152,...
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

🎉

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Closed (fixed) » Needs work
Issue tags: +Needs change record, +8.7.0 release notes

Actually I agree with Jibran's first opinion; this should have had a change record. It could affect sites or custom code that have their own dependencies on it to know it's now managed in Composer. We also usually briefly mention dependency changes (including updates and new dependencies) in the release notes.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added a release note to the IS.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Added the CR. Can someone check it?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Looks correct to me, moving to RTBC but this can probably just be moved to Fixed?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: -Needs followup, -Needs change record

Change record is done, followup exists as well (whether it's still relevant is another thing): #3027290: Use \Drupal\Core\Archiver\Tar instead of \Drupal\Core\Archiver\ArchiveTar in controllers are forms

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Fixed

Thanks 👍 looks proper status now

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone commented

Published the change record.