Fix(Task/Ext/Archive/ZipTask): Preseves directories' permissions in zip

[leagris]

Fix #1817 When file permissions are supported, stores directories' original permissions as ZipArchive::OPSYS_UNIX ExternalAttribute.

[codecov]

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 53.01%. Comparing base (1c69be9) to head (04a516e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Phing/Task/Ext/Archive/ZipTask.php	81.25%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1820      +/-   ##
============================================
+ Coverage     52.99%   53.01%   +0.01%     
- Complexity     9798     9805       +7     
============================================
  Files           495      495              
  Lines         24647    24662      +15     
============================================
+ Hits          13062    13074      +12     
- Misses        11585    11588       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mrook]

Hi,

Thanks for your contribution! I'd like to make this behavior optional (i.e. controlled by an attribute). The attribute should default to false because we're changing behavior. The attribute should also be documented in the guide and the RNG grammar. When that's all done, it'll be added to the 3.1 release line (not 3.0).

[leagris]

Hi,

Thanks for your contribution! I'd like to make this behavior optional (i.e. controlled by an attribute). The attribute should default to false because we're changing behavior. The attribute should also be documented in the guide and the RNG grammar. When that's all done, it'll be added to the 3.1 release line (not 3.0).

I am ok with that, though, something annoys me with letting the current working as default.
With current behaviour, files permissions are preserved because it is handled natively by the ZipArchive's addFile method. But since directories are not copied but created, ZipArchive does not know the original directory permissions.

Having files inheriting permissions but not directories ; it is more a bug than a feature. When dealing with such zip archives in a unix/Linux system, unzipped with the unzip command, those 777 read/write/execute permissions for everyone in directories is a serious problem. Letting this as the default feels quite odd.

I'd propose some alternative instead with a savFileAttributes switch default false as you propose, to save the extra attributes (os-dependant or unix/POSIX only?)

The ZipTask's addFilesetsToArchive method would need to be updated to implement those two attributes with:

• savFileAttributes="false" (default): Add files and directories to the zip without preserving their permission and timestamp. (Means not using ZipArchive's addFile but some other method like addFromString which does not have a source file attributes.
• saveFileAttributes="true": The addFile methods would do that natively, but it is not handled for directories. The handling of directories' file attributes need to be implemented outside the ZipArchive's expansion which does not support it natively. It has OS-specific handling that can add complexity especially for OS/2 or VMs (does not look like a priority to deal with those). If we stick to Unix/Linux/BSD POSIX environment, permission bits and timestamps are the things to preserve in fileAttributes.

See: the zip command's man-page for the -X, --no-extra option:

-X
--no-extra
Do not save extra file attributes (Extended Attributes on OS/2, uid/gid and file times on Unix). The zip format uses extra fields to include additional information for each entry. Some extra fields are specific to particular systems while others are applicable to all systems. Normally when zip reads entries from an existing archive, it reads the extra fields it knows, strips the rest, and adds the extra fields applicable to that system. With -X, zip strips all old fields and only includes the Unicode and Zip64 extra fields (currently these two extra fields cannot be disabled).

Negating this option, -X-, includes all the default extra fields, but also copies over any unrecognized extra fields.

[leagris]

@mrook Implemented a savefileattributes flag.

[leagris]

Even Copilot says my code is smart ;)

The addFile method in the ZipArchive class is typically used to add a file to the zip archive. However, in your script, it’s being used in a clever way to add a directory.

Here’s how it works:

When you call addFile with the mtimeDummy file and the entryName (which is the directory path), it adds an entry to the zip archive with the name of the directory. But because the mtimeDummy file is empty, this entry is effectively a directory, not a file.

The key here is the trailing slash in the entryName. In a zip archive, an entry with a trailing slash is treated as a directory.

So, even though addFile is normally used to add files, in this case, it’s being used to add a directory. This is possible because of the way entries are handled in zip archives and the use of the mtimeDummy file. This workaround allows you to preserve the mtime of the directory in the zip archive, despite the limitations of PHP 7.4’s ZipArchive class. It’s a smart solution! 👍

[mrook]

I'll take a look at this soon. Sorry for not having had the time earlier.

[siad007]

@leagris would you mind to add a test case please?
You can add one to this BuildTest and to the correspondent TestBuild XML file.