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

Fixed UploadedFile::moveTo() so it actually removes the original file when used in CLI context, and doesn't leave orphaned files #98

Merged
merged 3 commits into from Jul 6, 2022

Conversation

k2rn
Copy link
Contributor

@k2rn k2rn commented Jul 4, 2022

Signed-off-by: katsuren katsuren.houken@gmail.com
UploadedFile::moveTo doesn't work in CLI. Original file leave it as is.

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Calling UploadedFile::moveTo in CLI, it just copies the file to the targetPath.
In addition to this, the stream grabs the original file, so it cannot delete the file, and I cannot access the stream so I couldn't release the file pointer.
In a test for uploading file, I want to remove the uploaded file to keep clean the test folder.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

OK for me, but a test needs to be added here.

test/UploadedFileTest.php Outdated Show resolved Hide resolved
test/UploadedFileTest.php Outdated Show resolved Hide resolved
@k2rn k2rn requested a review from Ocramius July 4, 2022 09:47
@Ocramius Ocramius added this to the 2.12.0 milestone Jul 4, 2022
@Ocramius Ocramius added Bug Something isn't working Enhancement labels Jul 4, 2022
@Ocramius Ocramius changed the title fixed UploadedFile::moveTo doesn't work by CLI. Fixed UploadedFile::moveTo() so it actually removes the original file when used in CLI context, and doesn't leave orphaned files Jul 4, 2022
src/UploadedFile.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Ocramius commented Jul 4, 2022

Also to be asked here: do we merge to 2.11.x (bugfix), or is it OK to merge to 2.12.x (feature)? Would probably be better to release a bugfix.

@k2rn
Copy link
Contributor Author

k2rn commented Jul 4, 2022

I'm not in a hurry, so it's OK to 2.12.x but, I'd appreciate if it could merge to 2.11.x

@Ocramius Ocramius modified the milestones: 2.12.0, 2.11.3 Jul 6, 2022
@Ocramius Ocramius self-assigned this Jul 6, 2022
@Ocramius Ocramius changed the base branch from 2.12.x to 2.11.x July 6, 2022 09:01
@Ocramius Ocramius force-pushed the fixed_UploadedFile_moveTo branch 2 times, most recently from 5c574d7 to b785859 Compare July 6, 2022 09:07
k2rn and others added 2 commits July 6, 2022 11:09
…t as is.

Signed-off-by: katsuren <katsuren.houken@gmail.com>
Signed-off-by: katsuren <t@k2rn.com>
test/UploadedFileTest.php Show resolved Hide resolved
test/UploadedFileTest.php Outdated Show resolved Hide resolved
@k2rn
Copy link
Contributor Author

k2rn commented Jul 8, 2022

@k2rn Please create an issue report to report the problem. Thanks in advance! 👍🏻

@froschdesign
#108
I created the issue and close it. Thank you!

@PowerKiKi
Copy link

I agree that this PR should be considered at the very least as a minor breaking change. It broke quite a few of my unit tests. At least it was easy to notice the breakage. But now I need to use what feels like a dirty workaround, something similar to:

 <?php

 use Laminas\Diactoros\Stream;
 use Laminas\Diactoros\UploadedFile;

- new UploadedFile('my-file-for-unit-test', 999, UPLOAD_ERR_OK),
+ new UploadedFile(new Stream('my-file-for-unit-test'), 999, UPLOAD_ERR_OK),

@Ocramius
Copy link
Member

Well, the fact that the file wasn't moved is an actual bug, and yes, it breaks compat for some tests, but unlikely for prod code.

This is an XKCD 1172:

workflow_2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants