Problem/Motivation

PEAR's Archive_Tar just made a new 1.5.0 release.

D7 has a copy of this code in the system module.

We should update D7's copy with the changes; there's nothing too significant but the full minor release bump was because of a change in behaviour around file permissions which is probably worth a CR.

Steps to reproduce

Compare D7's modules/system/system.tar.inc with the new Archive_Tar release.

Proposed resolution

Sync / merge upstream changes.

Remaining tasks

As above.

User interface changes

n/a

API changes

See comment above re. file permissions change - this was the upstream PR:

https://github.com/pear/Archive_Tar/pull/46

Data model changes

n/a

Release notes snippet

tbc

Issue fork drupal-3429165

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mcdruid created an issue. See original summary.

mcdruid opened merge request !7083

mcdruid’s picture

mcdruid commented
Status: Active » Needs review
poker10’s picture

poker10 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Thanks for working on this!

Seems like the Archive_Tar class is used for example when using the Update manager to install a new module via UI (or update a module). See the update_manager_install_form() or update_manager_archive_extract() function.

I was able to trigger the Archive_Tar::_dirCheck() when installing the module via UI (using tar.gz archive). It seems like this function is currently setting the 777 permissions in the temporary directory, where the archive is extracted for the first time. Then the Updater::install() or Updater::update() function is copying the directory from temporary filesystem to the correct destination and permissions are corrected to 755 (at least during my limited testing).

Are we aware of any (other) usage in D7 core where the 777 permissions are kept (so that we can explain that in the CR)? Contrib modules can also use this core library differently (extracting code somewhere else), so I agree that we need the CR to describe this behavioral change.

Otherwise the changes looks good to me.

poker10’s picture

poker10 commented
Issue tags: -Needs change record +Pending Drupal 7 commit

I have drafted a simple CR here: https://www.drupal.org/node/3451526 (feel free to update if needed).

Otherwise I think this is ready. Adding a tag.

mcdruid’s picture

mcdruid commented

I'm not aware of anywhere else that core might be affected by this, no.

We have some quasi-unit tests (which couldn't be actual unit tests as they involve using e.g. the temp directory which means Drupal has to be somewhat bootstrapped IIRC) in SystemArchiverTest (added in #3102159: Add tests for Archive_Tar) if we wanted to add anything specific around this change.

I don't think we specifically need to in this case as we're just mirroring the upstream change, and existing tests cover core's actual usage - I think that's e.g. \UpdateTestUploadCase::testUploadModule which I only found after I'd added the unit(ish) tests mentioned above.

mcdruid’s picture

mcdruid commented

Oh, and draft CR looks good, thanks.

  • poker10 committed e755b13e on 7.x 
    Issue #3429165 by mcdruid: Sync D7's copy of Archive_Tar with new 1.5.0...
poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks! Committed and pushed this.

mcdruid’s picture

mcdruid commented
Related issues: +#3452398: Archive_Tar release 1.5.0

Status: Fixed » Closed (fixed)

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

nicxvan closed merge request !7083