dev/core#1565 Throwing an API_Exception if uploaded file fails to move with APIv3

[albionbrown]

Overview

When creating an attachment using APIv3, the civicrm_api3_attachment_create function copies the file to the custom file upload directory. However, there is no error checking currently.

Before

The file would attempt to be copied and the original removed.
copy($moveFile, $path); unlink($moveFile);

If the directory is non-writable, the file would be lost.

After

if (!copy($moveFile, $path)) { throw new API_Exception("Cannot copy uploaded file ".$moveFile." to ".$path); } unlink($moveFile);

Now, an API_Exception is thrown before the file is removed.

[civicrm-builder]

Can one of the admins verify this patch?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[albionbrown]

I have only made changes to api/v3/Attachment.php. I'm not sure where the test file conflict has come from but I assume it can be resolved easily.

[eileenmcnaughton]

@albionbrown it looks like the code is applied to the wrong branch or something - maybe checkout a clean copy of master & cherry-pick your patch on & force-push?

[albionbrown]

I'll try again today. Shall we close this PR?

[colemanw]

It looks like your master branch has gotten messed up because you committed df77575 and then did git pull which did a merge commit. You also seem to have done an unrelated commit d4581b3 after that.

Also it appears you created this PR from master instead of a branch, which is a recipe for mess. I'll close this so you can create a branch for this PR.

To reboot the whole shebang, you can do:

git checkout master
git reset HEAD~4
git pull upstream master
git checkout -b apiException
git cherry-pick df775751fb69289de1dc0567d1c5b0b8872c487c
git push -uf origin apiException

It will give you a link to opening a new PR.

Note, if it tries to create another merge commit during step 3, cancel it and do step 2 again. Reset as far back as you need to so it will pull cleanly without prompting you to create a merge commit. Change step 2 to git reset HEAD~9999 if you need to.

[albionbrown]

Thanks for having a look @colemanw. Out of curiosity, is it preferable to create a new fork when going to create a new PR or can I just use the same one I forked last time but just need to update it?

[albionbrown]

Ok I think I'm going to have to delete my repo and fork again because I realised what I've done now.

[albionbrown]

Created new PR #16465

[colemanw]

@albionbrown I've created thousands of PRs and only ever had one fork :)
git reset is magic for getting you out of trouble. In the worst case I suppose you could delete your own master branch and then check it out again from upstream, but I've never found it necessary to do that either.