Closed (fixed)
Project:
Services
Version:
7.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
2 Mar 2011 at 07:32 UTC
Updated:
7 Mar 2011 at 21:24 UTC
Jump to comment: Most recent file
The task is to port file resource to D7 and add tests.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 1078276-file-resource-tests-review.patch | 7.27 KB | ygerasimov |
| #1 | 1078276-file-resource-tests.patch | 6.51 KB | ygerasimov |
Comments
Comment #1
ygerasimov commentedAttached patch made with git format-patch.
Comment #2
ygerasimov commentedComment #3
gddSome general comments, although I'm not sure they are worth holding up the patch for
1) In Drupal 6 I had to take file_save_upload() and mostly replicate it in Services, because that API call requires $_FILES[] to be used rather than simply being able to save a file already uploaded. Is there a better API call in Drupal 7? I was hoping to remove a lot of that code.
2) I don't really like that we define our own permissions here. I'd rather use Drupal ones but I don't know know of any that are appropriate.
3) In the access callback, this refers to 'administer files' which doesn't exist as far as I can tell.
4) I don't really like using $op in the access callbacks. This is a pattern Drupal 7 moved away from. However I know we use it a lot elsewhere and I'm not going to hold this up over it. We might want to get back to cleaning these up later though.
Overall though I don't see any huge problems here. Haven't had a chance to test it but I assume we will be able to commit it pretty quickly.
Comment #4
ygerasimov commentedIn Drupal 7 we do file_save_data(base64_decode($file->file), $file->filepath)) directly on the encoded data we have received.
I haven't found suitable core permissions for our file service so I would keep those permissions.
I have removed 'administer files' check from access callback.
Comment #5
gddCommitted! Thanks as always!