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

Prevent ContentType assignation when dirs are created. #164

Merged
merged 1 commit into from Jun 5, 2019

Conversation

ganeko
Copy link

@ganeko ganeko commented Feb 13, 2019

ContentType assignation is not needed for directories and this cause unexpected behavior in third-parties like s3fs.

Using this adapter, if you create a directory and you try to read it via s3fs you get an error because it is managed as a file.

Sample code,

$flySystem->createDir('exports://fromFlySystem/')

Accessing via s3fs (in other system outside PHP environment),

root@xxxxx:/mnt/exports# ls -la
total 1
drwxrwxr-x 1 www-data www-data 0 Jan  1  1970 .
drwxrwxrwx 1 root     root     0 Jan  1  1970 ..
---------- 1 root     root     0 Feb 13 11:59 fromFlySystem
root@xxxxx:/mnt/exports# file fromFlySystem 
fromFlySystem: empty
root@xxxxx:/mnt/exports# cd fromFlySystem 
-bash: cd: fromFlySystem: Not a directory

@frankdejonge
Copy link
Member

Holla! Could you update the tests for this too? Also, just a quick question, why do you create directories on s3?

@ganeko
Copy link
Author

ganeko commented Feb 13, 2019

Hi,

We create directories via this adapter because s3fs' performance is very bad and in order to prevent CPU problems in the other system.

@frankdejonge
Copy link
Member

Do you actually need the directories? Because s3 itself doesn't need them. What's your use-case?

@ganeko
Copy link
Author

ganeko commented Feb 13, 2019

Yes, because we deploy the same core in different environments with different adapters (Local, S3, etc...) and we need to ensure that the directories exist for the following processes, otherwise, we have to be constantly checking this and as I said, s3fs has performance issues.

@ganeko
Copy link
Author

ganeko commented Feb 14, 2019

Hi @frankdejonge ,

Any chance to keep an eye on the changes I've suggested?.

Thanks!

@frankdejonge
Copy link
Member

Dude, the PR is 19 hours old.

@ganeko
Copy link
Author

ganeko commented Feb 14, 2019

Oh, sorry.

Yesterday we were having a conversation and I thought it would be more fluid.

Many thanks.

@frankdejonge
Copy link
Member

Also, I want to first check what the consequences are for DO spaces and AWS S3 itself before merging to see if it doesn't cause regressions. I'm also moving house now so I expect to get to this somewhere next week at the earliest.

@ganeko
Copy link
Author

ganeko commented Feb 14, 2019

Ok, no problem.

Many thanks, and apologies.

@frankdejonge frankdejonge merged commit 8c47cf0 into thephpleague:master Jun 5, 2019
@frankdejonge
Copy link
Member

This is released as 1.0.23

@ganeko
Copy link
Author

ganeko commented Jun 6, 2019

Amazing, thanks a lot!.

@frankdejonge
Copy link
Member

Thank you for your contribution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants