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

Add method returning client provided file path #166

Conversation

hhovakimyan
Copy link
Contributor

UploadedFile class has a property containing temporary path of the
uploaded file, which is inaccessible outside of the class. But there is
a need of accessing this property outside of the class, when for
example you must upload the uploaded file directly to the cloud(e.g.
Amazon S3 bucket) without saving it to your local file system. So a
getter method is created returning the value of this property.

UploadedFile class has a property containing temporary path of the
uploaded file, which is inaccessible outside of the class. But there is
a need of accessing this property outside of the class, when for
example you must upload the uploaded file directly to the cloud(e.g.
Amazon S3 bucket) without saving it to your local file system. So a
getter method is created returning the value of this property.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 346854b on hhovakimyan:feature/add-method-returning-file-path-in-UploadedFile-class into 5b19133 on slimphp:master.

@hhovakimyan
Copy link
Contributor Author

hhovakimyan commented Aug 16, 2020

@l0gicgate @akrabat can you please merge this PR as soon as possible?

@l0gicgate
Copy link
Member

@hhovakimyan this is not part of the PSR-7 spec, that's why it has not been implemented. Unfortunately I don't think this is necessary, it seems like this is only for the edge case in your application.

@akrabat what are your thoughts on this?

@hhovakimyan
Copy link
Contributor Author

@l0gicgate first I don't think the case in my application is an edge case. I think many people would like to send the uploaded file directly to cloud(AWS S3 bucket, Google Drive, Dropbox, etc) without copying it to local file system and then deleting the local copy after upload succeeds. Second, if yo want to strictly follow the PSR-7 spec, there is also the option of changing the access modifier of $file property from protected to public. In Slim 3 the $file property of UploadedFile class was a public property and I used it for getting the value of uploaded file's tmp path. That's why I have a backward compatibility issue after migrating from Slim 3 to Slim 4.

Copy link
Member

@l0gicgate l0gicgate left a comment

Choose a reason for hiding this comment

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

I wasn’t aware this was a regression from Slim 3.

Just for the record saying things like “Can we get this merged as fast as possible” makes me not want to merge PRs.

Instead I would advocate using suggestive language like “Would it be possible to get this merged in a timely manner?”

Thank you for your contribution

@l0gicgate l0gicgate added this to the 1.2 milestone Aug 18, 2020
@l0gicgate l0gicgate merged commit 832912c into slimphp:master Aug 18, 2020
@hhovakimyan hhovakimyan deleted the feature/add-method-returning-file-path-in-UploadedFile-class branch August 19, 2020 08:24
@hhovakimyan
Copy link
Contributor Author

hhovakimyan commented Aug 19, 2020

@l0gicgate thanks a lot for merging this pull request. Sorry for the way I have asked about merging this PR. Next time I will use suggestive language in such cases. Also can you tell me please when the 1.2 milestone is planned to be released?

@l0gicgate
Copy link
Member

@hhovakimyan just released for you:
https://github.com/slimphp/Slim-Psr7/releases/tag/1.2.0

@piotr-cz
Copy link

Thanks @hhovakimyan.
By the way I think this should be part of a PSR-7 spec, for the reasons you outlined.

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

Successfully merging this pull request may close these issues.

4 participants