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.

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

8330 Correct issue where permission umask of files were not set when … #8331

Open
wants to merge 1 commit into
base: master
from

Conversation

@jimmy-ho
Copy link

jimmy-ho commented Sep 17, 2019

…archiving in zip format with ZipArchiver

Copy link
Contributor

stof left a comment

should we also set the permissions when adding an empty dir ?

src/Composer/Package/Archiver/ZipArchiver.php Outdated Show resolved Hide resolved
@jimmy-ho

This comment has been minimized.

Copy link
Author

jimmy-ho commented Sep 17, 2019

should we also set the permissions when adding an empty dir ?

@stof Guess there should not be any harm to

@jimmy-ho jimmy-ho force-pushed the jimmy-ho:bug/issue-8330 branch from db998f9 to 5e154a6 Sep 17, 2019
@jimmy-ho

This comment has been minimized.

Copy link
Author

jimmy-ho commented Sep 17, 2019

@stof code updated to always be applied on directory and files, and removed the defined() check if the minimum requirement is PHP 5.3 for composer.

…archiving in zip format with ZipArchiver
@jimmy-ho jimmy-ho force-pushed the jimmy-ho:bug/issue-8330 branch from 5e154a6 to 91732ee Sep 17, 2019
@stof

This comment has been minimized.

Copy link
Contributor

stof commented Sep 17, 2019

Have you check the support of empty dirs here ? According to https://stackoverflow.com/questions/36287554/php-ziparchive-file-permissions you might need to handle dirs with a specific change.

@jimmy-ho

This comment has been minimized.

Copy link
Author

jimmy-ho commented Sep 17, 2019

Have you check the support of empty dirs here ? According to https://stackoverflow.com/questions/36287554/php-ziparchive-file-permissions you might need to handle dirs with a specific change.

@stof I have not had this use-case to test unfortunately, the immediate issue has been the executable flag for files being dropped... this has caused binaries which should be executable to stop working.

Happy to explore if there are examples to really validate that use-case - perhaps a new issue to address it further.

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Sep 17, 2019

Well, try to run the archiver on a folder containing an empty dir and see whether the empty dir preserves the permission when unzipping.
I don't think we have existing issues about permissions of empty directories specifically, given that the existing archiver does not support permissions at all. But adding support for permissions should ideally be done in a working way

@alcohol

This comment has been minimized.

Copy link
Member

alcohol commented Sep 18, 2019

Do we need to take windows hosts into account here in some form or manner, or can we safely run this at all times?

@a-camacho

This comment has been minimized.

Copy link

a-camacho commented Dec 2, 2019

Any news from this ? We are also experiencing the problem where composer is installing packages and using CHMOD 666 instead of default value of server CHMOD 644. When I unzip the .zip or .tar file myself it works, but not with Composer.

@alcohol

This comment has been minimized.

Copy link
Member

alcohol commented Dec 2, 2019

@a-camacho this issue is unrelated to yours. You are referring to Composer extracting files from archive(s). This issue is related to archives generated by Composer.

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Dec 2, 2019

@alcohol he's referring to extracting files from a Satis repo according to composer/satis#577, and so extracting files generated by the code being fixed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.