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

Throwing API_Exception is file fails to copy when creating attachment… #16465

Merged
merged 2 commits into from
Feb 10, 2020

Conversation

albionbrown
Copy link
Contributor

@albionbrown albionbrown commented Feb 4, 2020

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
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented Feb 4, 2020

(Standard links)

@colemanw
Copy link
Member

colemanw commented Feb 4, 2020

Add to whitelist

@albionbrown
Copy link
Contributor Author

Would you prefer if I corrected those spaces and created another PR?

@colemanw
Copy link
Member

colemanw commented Feb 4, 2020

@albionbrown well you definitely don't need to delete your fork :)
Don't actually need to delete this branch or PR either. You can just amend your commit.
Here's how I do minor touch-ups to existing commits in a PR:

git checkout #1565-API-Exception
(in your text editor, make the changes and save the file)
git commit -a --amend
git push -f origin #1565-API-Exception

api/v3/Attachment.php Outdated Show resolved Hide resolved
@eileenmcnaughton
Copy link
Contributor

Makes sense now the commits are cleaned

@eileenmcnaughton eileenmcnaughton merged commit df6da69 into civicrm:master Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants