Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

taskPack modified to correctly set file permissions in ZIP files on Unix and on Windows NTFS #897

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from
Open

taskPack modified to correctly set file permissions in ZIP files on Unix and on Windows NTFS #897

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/Task/Archive/Pack.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,42 @@ protected function addItemsToZip($zip, $items)
if (!$zip->addFile($file->getRealpath(), "{$placementLocation}/{$relativePathname}")) {
return Result::error($this, "Could not add directory $filesystemLocation to the archive; error adding {$file->getRealpath()}.");
}
// This will retain the file permissions for *nix systems
// Source: https://github.com/consolidation/Robo/pull/888
if (stripos(PHP_OS, 'WIN') !== 0) {
$zip->setExternalAttributesName(
"{$placementLocation}/{$relativePathname}",
\ZipArchive::OPSYS_UNIX,
$file->getPerms() << 16
);
} else {
// For windows we force 0644 for all the files
// Because it seems when the archive is created on Windows the files by default has
// 0666 permission (can be seen when unpacked on a *nix system)
$zip->setExternalAttributesName(
"{$placementLocation}/{$relativePathname}",
\ZipArchive::OPSYS_WINDOWS_NTFS,
33188 // Is the decimal for 100644
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer if expressed in octal (0100644).

I guess there is no +x bit on Windows? is-executable returns true only for .exe files, which is pointless on Linux. I guess manipulating Linux files on a Windows system just has to be an edge case that we don't / can't support.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Octal expression doesn't work on Windows, it seems only decimal values work on windows via setExternalAttributesName().
And this is not an edge case, many of the PHP devs develop on windows and may create an archive to distribute on linux servers. It causes a huge problem, like if the index/root file has a permission of 0666, it shows an 403 forbidden error when accessed via HTTP and file compressed via Robo in windows has 0666 permission for all the files(except directories, they have 0755).
So, I guess it would be an essential things to implement, I'm using this locally and it's working for me and someone may face the same issue in future that's why I created the pull request, it's your choice if you want to merge or not.
Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you get when you run this on Windows?

php -r 'print 0100644;'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By saying octal expression doesn't work on windows I meant about the setExternalAttributesName(); method's parameter, not the OS entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems impossible to me; regardless of whether you represent a number in octal or decimal in the source, the data passed to a method parameter should be the same. Can you explain why setExternalAttributesName() fails with an octal parameter? What is at work here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it's an internal bug of PHP, besides setExternalAttributesName() isn't properly documented, the only example they provided is for the unix system and while on windows the octal values doesn't change the permission as expected, I'm kinda busy to write the tests, and I'm not sure what to tests against, as extracting the zip on windows to check if the has the permission of 0644 or not won't work on Windows because that's not how windows permissions work at all.
I'm closing this, thanks.

);
}
}
} elseif (is_file($filesystemLocation)) {
if (!$zip->addFile($filesystemLocation, $placementLocation)) {
return Result::error($this, "Could not add file $filesystemLocation to the archive.");
}
if (stripos(PHP_OS, 'WIN') !== 0) {
$zip->setExternalAttributesName(
$placementLocation,
\ZipArchive::OPSYS_UNIX,
fileperms($filesystemLocation) << 16
);
} else {
$zip->setExternalAttributesName(
$placementLocation,
\ZipArchive::OPSYS_WINDOWS_NTFS,
33188
);
}
} else {
return Result::error($this, "Could not find $filesystemLocation for the archive.");
}
Expand Down