Posted by kylebrowning on March 16, 2012 at 8:26am
15 followers
| Project: | Services |
| Version: | 7.x-3.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Title says it all, this is the first revision of this patch I'm sure it'll have a couple issues. It needs tests but anyways,
This will let you targeted action a node i.e., node/1234/attach_file
And you can multipart enc a file without having to base 64.
Comments
#1
Hi there, don't see patch file attached - however I'm interested in it and would like to see it. Would you be so kind to attach the patch?
#2
#3
#4
Mini review, I haven't had time to actually test it yet.
1. Spelling in the field_name description.
2. Is the file_usage_add necessary? (See #1490552: File resource should not count itself to file usage)
Thanks for posting this. It could be useful.
#5
Thanks a million, Kyle.
#6
Hi there Kyle, when you define args to the action, you mention type = 'array'
<?phparray(
'name' => 'field_name',
'optional' => TRUE,
'source' => array('param' => 'field_name'),
'description' => 'The file parameteres',
'type' => 'array',
),
?>
and then, in _node_resource_attach_file you're receiving field name as array - why don't change type to 'string' so $node->{$field_name[0]}['und'][0] = (array)$file; will be $node->{$field_name}['und'][0] = (array)$file; ?
It's confused me a bit, however I might miss your point here - that's why I'm asking.
Kind Regards, Slava.
#7
What if you want to supply the file path, how do you that with your code?
What if there are multiple files?
Its an array b/c you need to pass in all the elements of a file object so that it can be created successfully.
#8
Kyle, in terms of #1060362: Remove resources in Services 4.x I would not like to add this action to core of services. Lets create separate module for it and gather all useful resources there.
Regarding patch. Can we use standard file_save_upload() to save the files and gather errors with form_get_errors()? Also I believe file validators should be taken from field settings.
Tests are must for this case.
#9
id prefer not to wait for that patch to land unless you are going to do it very soon.
#10
EDIT: nevermind, I redownloaded services, and then it worked. Must have messed up something myself.
Is the #2 patch working? I'm getting a lot of errors.
#11
patch two probably needs to be remade since schanges have happened since i wrote it. One sec ill get a new one up.
#12
Patch in #2 aplies cleanly for me, not sure what errors you are getting?
#13
#2: 0001-Attach-files-to-a-node.patch queued for re-testing.
#14
Yeah I think we should probably try to let file_save_upload() do more of the work here. Did you really mean to change servers/rest_server/includes/RESTServer.inc? It seems like that would break multipart form handling.
#15
Also the error messages spell unknown as 'unkown'. You might want to check $file->fid after the first file_save() because if it's not set by then all the subsequent stuff will explode... though that makes me wonder why you're calling file_move()? Why not just put it into the final destination?
Actually I think the real solution is, as ygerasimov also suggested, just to use file_save_upload().
#16
Yes I really meant to change RESTServer, it doesn't break multipart the tests still pass.
However it'd be a really big help if someone else could get this patch up to snuff, I think Ive brought it pretty far along but I need to focus on a few other backports.
#17
Im trying this out from a java (android) app by posting to endpoint/node/999/attach_file and passing in the params using params.put with a library called loopj:
RequestParams params = new RequestParams();params.put("file", file);
params.put("uid", uid);
params.put("field_name", "field_log_file");
params.put("filename", filename);
params.put("filepath", "private://logs/" + filename);
params.put("filesize", Long.toString(bytes));
client.post(TARGET, params, new JsonHttpResponseHandler() {
etc etc}
Strangely its getting added to the file_managed table but with a status 0 and wrong filepath. Its not attaching to the node at all. Whats the expected parameters for this to work?
#18
@7 wonders,
I got it to work posting a form input type=file to: site.com/endpoint/node/22/attach_file.json?field_name=field_log_file
This patch:
- rerolls against the latest dev
- fixes the above misspellings
- fixes the services_error( t('text,500 ));
- clears some whitespace at end of line
node_save, adds an entry to {file_usage}
File_usage_add adds a second entry to {file_usage}
file_usage_add($file, 'services', 'files', $file->fid);I don't think services needs to add an entry with file_usage_add, because it blocks Drupal from deleting that file. ex. Upload a file with this patch, then edit node and remove/replace that file and save and the file will still exist.
file_delete(stdClass $file, $force = FALSE)If the $force parameter is not TRUE, file_usage_list() will be called to determine if the file is being used by any modules. If the file is being used the delete will be canceled.
http://api.drupal.org/api/drupal/includes!file.inc/function/file_delete/7
File usage functions:
http://drupal.org/node/1017800
Also, attach_file doesn't apply the file field's directory as if you uploaded within Drupal.
#19
can someone please add some documentation as how to use this patch. I have enabled the attach file method on the node resource. Then I am using xhr poster to test the end point and I keep getting 404 Not found: Could not find the controller.
It would be helpful if someone can give an example of how to use this patch.
Thank you.
#20
#21
To get this to work I'm posting a multipart/form-data to endpoint/node/123/attach_file?file_field=field_machine_name. <input type=file name=files[anything]>
This is a major rewrite/simplification. There's no need to duplicate the fine work already provided by file_save_upload. Especially since the only thing to change was the name attribute of the input element.
Has $nid & $field_name been sanitized/made safe by the time they reach _node_resource_attach_file()?
@todo
-Right now, this overwrites a file already on the node... if N files are uploaded it overwrites the first N files. As a targeted action 'attach_file'... should it error if a file is already attached, and only 1 file can be attached? Give options, and default behavior?
-The querystring ?field_name=etc needs to be changed to a posted variable.
-This doesn't trigger the image field's style(thumbnail, small, medium,etc) generator. Thoughts on how this is implemented?
-A nice feature might be to allowing changing of the filename when the file.
#22
Oops
#23
We define 'field_name' argument as array but use only first element (a machine name). Maybe we should change argument to string?
Should be added check whether field really exists and attached to this node type.
Test should be added.
#24
:)
I really wanted a way to upload a picture, while the user was entering information. I saw this in one of the videos from Drupalcon. So the flow would be to upload to presave/presavedata, store the fid, then attach to the node on node/create. Doing it this way, let's it work similar to how file upload works on a new node.
Action:
node/file_presave
**I can post file to and get back the fid
node/file_presavedata
** there's code but it needs work, compare/borrow from file_create
Targeted Action
node/123/attach_file
* fixed the array param to data
* I'm unsure whether to use array(data=>field_name) or just 'data'
Should it be broken up into separate issues?
#25
3am posts w/o attachments, mmmm.
#26
This is not a functional review just a code strandards review
Please remove commented out lines
<?php+// 'access arguments' => array('wo'),
+// 'access arguments append' => TRUE,
?>
Why is user declared as a global it is not used
<?php+function _node_resource_attach_file($nid, $field_name) {
+ global $user;
?>
Missing space after // and missing period at the end of the line
<?php//Load the information for this field
?>
Missing space after $counter and before 0
<?php+ $counter=0;
?>
let should be Let and missing space after //
<?php+ // let the file module handle the upload and moving.
?>
Missing space after // and missing period at the end of the line
<?php+ //Add info to the array that will be returned/encdoed to xml/json
?>
Missing space after // and missing period at the end of the line
<?php+ //Check if the maximum number of files for this field has been reached. A
+ //cardinality of -1 (unlimited) will never match
?>
Duplicate function
<?php+function _node_resource_attach_fileid($nid, $field_name) {
+}
?>
Why is user declared as a global it is not used
<?php+function _node_resource_file_presave($node_type, $field_name) {
+ global $user;
?>
Missing space after // and missing period at the end of the line
<?php//Load the information for this field
?>
Extra spaces after ( and before )
<?phpif ( !$instance = field_read_instance('node', $field_name, $node_type) ) {
?>
Extra space after =>
<?php+ 'file_validate_extensions' => array(0 => $instance['settings']['file_extensions']),
?>
Missing space after // and missing period at the end of the line
<?php//Check $_FILES to make sure
?>
Missing space after $counter and before 0
<?php+ $counter=0;
?>
let should be Let and missing space after //
<?php+ // let the file module handle the upload and moving.
?>
Most of the code in _node_resource_file_presave and _node_resource_attach_file is the same lets have a helper function for those elements.
Missing space after // and missing period at the end of the line
<?php//Add info to the array that will be returned/encdoed to xml/json
?>
Remove commented out code
<?php+ //'uuid' => services_resource_uri(array('file', $file->uuid)),
?>
No need for this services has a generalise callback that does this already, and if the intent is additional permissions then the callback needs to do that and not return true.
<?php+function _node_resource_access_true() {
+ return true;
+}
?>
#27
Yea, it's a little rough still. ;)
#28
I am posting some thing like this
<FORM action="http://localhost/site-name/rest-server/node/78/attach_file?field_name=field_attach_file_service" enctype="multipart/form-data" method="post"><input type="file" name="files[field_attach_file_service_und_0]"><BR>
<INPUT type="submit">
</FORM>
I keep getting the 406 not acceptable.
In my logs Services report the following
Called arguments:Array
(
[0] => 78
[1] =>
)
I this a bug or something is wrong with what I am posting?
#29
I'm guessing some of you are able to upload at least a file using the services, can someone please give a code sample of his to do this?
I want to upload an image file in android to my drupal website?. I've lost 2 weeks so far.
I have most probably very easy questions for you all:
#30
@metow - Ive been using the loopj (just google loopj android) library which works nicely with something like this:
/**
* upload files to the server
*
* @param filename
*/
private void sendFiles(final String filename) {
String fing = filedir.toString();
final File file = new File(fing + "/" + filename);
String in = null;
try {
in = Base64.encodeFromFile(fing + "/" + filename);
}
catch (IOException e1) {
// TODO Auto-generated catch block
e1.printStackTrace();
}
Long bytes = file.length();
RequestParams params = new RequestParams();
params.put("file", in);
params.put("uid", uid);
params.put("filename", filename);
params.put("filepath", "private://" + uid + "/" + filename);
params.put("filesize", Long.toString(bytes));
client.post(FILE_URI, params, new JsonHttpResponseHandler() {
@Override
public void onStart() {
Log.i(TAG, "sendFile " + filename + " started");
}
@Override
public void onSuccess(JSONObject json) {
Log.i(TAG, "sendFiles success " + json.toString());
}
@Override
public void onFailure(Throwable e) {
Log.e(TAG, "sendFiles Failed = " + e.toString());
}
@Override
public void onFinish() {
Log.i(TAG, "finished");
}
}
});
}
The Base64 is another custom jar I picked up from somewhere so I could use earlier versions of android, just google base64 android earlier versions or similar and you will find it.
This will upload the file to the server into the directory you specify (public/private). If you want it to be attached to a node you will have to use the fid that you get back in the json in onSuccess to send a subsequent request to create a node (or figure out the targeted action in this issue). FILE_URI is "http://yourdomain.com/your_endpoint/file" which can all be set up in the services settings.
#31
What is the status of the patch?
Which version should I be using?
I am currently using the 0001-Attach-files-to-a-node.patch , my app says it successfully uploaded the photo but when I go to check my Drupal node, the photo doesn't seem to be uploaded.
I am currently setting the name, fileName, fileData, mimetype, field_name and nid when I upload my photo dictionary, am I doing something wrong ?
Anyone able to shed some light would be great.
EDIT
------------
After finally take a good read through some of these comments, I manage to get it working with patch 0002.
I also had to change from "fileName" to "filename" although my final image file name on Drupal came out as "null" = /
EDIT 2
------------
OK, have fixed the file name. Forgot that Kyle's Drupal IOS SDK uses "fileName" rather than "filename" and because patch 0002 uses "filename", I had to modify Kyle's Drupa IOS SDK class to use "filename" rather than "fileName".
I'm a happy fella again :)
#32
I took out the part that changed $_POST to $_FILES from the first patch. Having the $_FILES returned meant that all multipart posts would only return the $_FILES variable, and any other post data wouldn't be passed to the resource functions. (ie it allows you to post data, rather than attach it to the url as a param).
And obviously still needs tests.
#33
@jhr, thank you for your work on the patch.
Regarding #24, yes lets have this issue only for targeted_action. Can you split the patch? I believe blocker here is only absence of the test. Will you take care about it or shall I work on it?
#34
Here is patch with the test. I have kept only attach_file targeted action.
Also I have changed a logic a bit. We now do validation on cardinality and number of incoming files before starting uploading files.
#35
As agreed with kylebrowning on IRC comitting #34. Needs to have backport.
#36
Is this supposed to be working?? I just get:
POST http://test.dev/test/node/1/attach_file?field_name=field_log_file
Content-Type: multipart/form-data
Filename: /home/me/logs/test.txt
-- response --
401 Unauthorized: Missing required argument field_name
#37
I am getting 500
500An error occurred: (23000): SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_recording_display' cannot be null
#38
The following change worked for me when attempting to address the issue in #37.
I changed the following code in _node_resource_attach_file (line 626) of node_resource
From
<?phpforeach ($file_objs as $file_obj) {
$node->{$field_name}[LANGUAGE_NONE][ $counter++] = (array)$file_obj;
}
?>
To
<?phpforeach ($file_objs as $file_obj) {
$counter++;
$node->{$field_name}[LANGUAGE_NONE][$counter] = (array)$file_obj;
$node->{$field_name}[LANGUAGE_NONE][$counter]['display'] = 1;
}
?>
I know this is not a proper solution but I hope this helps in identifying the problem and providing a better solution.
Also, I think the original patch works for image field without any changes but not for any file.
(I am not sure if this should be switched back to 7.x)
#39
Needs review as per #37.
#40
@awm086 I get the same thing if I try to attach the file to a file field. If I attach the file to an image field, it works as expected. Your solution fixed this for me. I have rolled a patch for this.
#41
The last submitted patch, services-1484992-40.patch, failed testing.
#42
#40: services-1484992-40.patch queued for re-testing.
#43
The last submitted patch, services-1484992-40.patch, failed testing.
#44
I also have a question about upload destinations (file paths). This was possible to set with file_resource_create, can this be done now?I have a filefield_path setting that should take care of this but it isn't respected by the file_field_widget_uri method. Perhaps some kind hook or alter call could be added after this:$destination = file_field_widget_uri($field, $instance);so that I can modify the destination from my module? Any thoughts about this?It turns out that filefield_path actually took care of this.
#45
There also seems to be race conditions when trying to post multiple files from mobile device, ios, in my case.
Everything works fine when I hit the endpoint once per file but when I do multiple multiple files the node is not updated properly.
I tested with 3 files and, although, all the files were uploaded to the proper directory, only one file was attached to the node, namely the 3rd file.
In the case of trying 11 files, Only 8 files were attached.
I am not sure if this is a race condition related to node save (http://drupal.org/node/1679344) but I may be the case.
#46
#40: services-1484992-40.patch queued for re-testing.
#47
The last submitted patch, services-1484992-40.patch, failed testing.
#48
I re-rolled the patch to check for field settings. The current one will fail in case of field_image.
#49
Can there be added support for submitting optional extra file field information, (alt, description etc) on this endpoint? Rather than a required extra /node update to add them.
#50
#49 : It is possible but I believe, if I'm not mistaken, that this will have to be a feature request.
#51
I'v put up a feature request #1823908: Attach file to node targeted_action extra field information with an example.
#52
I can't get this to work, unfortunately.
When I upload a file through the endpoint, I get the following errors with no change in content on my server:
Here's the request and response:
Status: 200 OK
Request headers
Content-Type: multipart/form-data; boundary=----WebKitFormBoundarysEBQtnDJKzzJwZIP
Response headers
Date: Fri, 23 Nov 2012 21:56:33 GMT
Server: Apache/2.2.14 (Ubuntu)
X-Powered-By: PHP/5.3.2-1ubuntu4.18
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Fri, 23 Nov 2012 21:56:33 +0000
Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0
ETag: "1353707793"
Vary: Accept
Content-Length: 2
Keep-Alive: timeout=15, max=51
Connection: Keep-Alive
Content-Type: application/json
#53
@wildermuthn , as you may already know, 200 means the call succeeded. Can you verify the no file is actually uploaded to the target folder?
#54
#48 has been commited.