Posted by sanduhrs on June 17, 2011 at 1:51pm
6 followers
| Project: | Services |
| Version: | 6.x-3.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | cotto |
| Status: | needs work |
Issue Summary
The code in node_resource.incdrupal_form_submit($node->type . '_node_form', $form_state, $old_node);
makes the assumption that $node->type is allways set.
If not, the absence of type results in the following notices/warnings:
Notice: Undefined index: type in _node_resource_update() (line 282 of /services/sites/all/modules/contrib/services/resources/node_resource.inc).
Notice: Undefined index: _node_form in drupal_retrieve_form() (line 735 of /services/includes/form.inc).
Warning: call_user_func_array() expects parameter 1 to be a valid callback, function '_node_form' not found or invalid function name in drupal_retrieve_form() (line 770 of /services/includes/form.inc).From my point of view it's not desireable to force the client to PUT the node type for an update.
We could just take it from the $old_node object, if not set in the PUT request.
Comments
#1
Patch attached, please review.
#2
#3
This seems like a good feature however as it stands I can set $node['type'] to be an empty string and it will try to use that value an !empty check would be more appropriate than isset. Also as this is a new feature can you add a test for this?
#4
Still, I think a method that throws notices and warnings instead of returning a meaningful response should be considered a bug.
By the way, the generated response for a request like
PUT http://example.org/rest/node/1
node[title]=New Title Here
is
Status: 200 OK
<?xml version="1.0" encoding="utf-8"?>
<result>
<nid>1</nid>
<uri>http://example.org/rest/node/1</uri>
</result>
Which implies to me, that all went well while it didn't.
Bug?
#5
The new patch checks whether the type attribute ist set and if it's a valid node type.
If not set or invalid it will keep the old value.
The patch inludes three test cases to check for missing, empty and invalid type attribute.
#6
This still does not take into account that node type can be empty with the isset check. Plus there is no need to do an !array_key_exists($node['type'], $node_types)) check instead you can do an isset for this. This should be used as it is more performant.
#7
Well, if the node type doesn't exist, the same error occurs, as if none or empty is given.
So to be sure, we need to check if the node type is valid
array_key_exists($node['type'], $node_types))This also takes into account when type is empty.
Can we assume that the $old_node->type is valid per se as I do in that patch?
Probably not, because some administrator could have deleted it.
I'm not sure what to do in that case.
#8
To prevent the error from occuring, we need to check for a valid node type parameter in any case.
Whether it has been submitted or not doesn't make a difference.
I tried to check for validity with a simple
function_exists($node['type'] . '_node_form'), but that didn't work out.Any other ideas for a better performing solution?
Attached is a patch that checks for isset, empty and validity.
It returns 406 in case of invalidity.
#9
This can simplified
<?php
// Make submission of node type optional
if (!isset($node['type']) || empty($node['type'])) {
$node['type'] = $old_node->type;
}
>
to
<?php
// Make submission of node type optional.
if (empty($node['type'])) {
$node['type'] = $old_node->type;
}
?>
as empty will return false if the item does not exist or is ''.
Also you need to add periods after your comments.
The approach of making sure the node type is valid is a really good idea, once the above is done this should be good to go. I haven't run the tests yet but they look sane from an eyeball.
#10
Changed, as you suggested.
#11
Please review.
#12
Thanks looks good.
#13
if we can get a 6.x version that would be great.
#14
#15
I don't know where to put this notice:
Notice: Undefined variable: final_resource en services_edit_form_endpoint_resources_submit() (línea 396 de ~/sites/all/modules/services/plugins/export_ui/services_ctools_export_ui.class.php).
#16
Grabbing to reroll this weekend
@Killua99 please raise a new issue as your post is not related to this issue at all thanks.
#17
Ok then.
#18
New version of the patch to allow updates to ignore type. As per normally I'll backport after d7 version is rtbc.
#19
+++ b/resources/node_resource.incundefined@@ -313,13 +313,18 @@ function _node_resource_update($nid, $node) {
+ // If no type is provided use the existing node type.
+ if (empty($node['type']) && isset($old_node->type)) {
+ $node['type'] = $old_node->type;
+ }
+
// Validate the node. If there is validation error Exception will be thrown
// so code below won't be executed.
I think there is no need to check whether $old_node->type is available as we did node_load before.
Also we can avoid checking whether content type is valid if we loaded it from old node:
<?php// If no type is provided use the existing node type.
if (empty($node['type'])) {
$node['type'] = $old_node->type;
}
else {
// Validate the node. If there is validation error Exception will be thrown
// so code below won't be executed.
_node_resource_validate_type($node);
}
?>
Everything else I am happy with. Thanks for great work @sanduhrs and @marcingy!
#20
Reroll with changes above
#21
@marcingy, what do you think about attached patch. Code is just moved around but in my opinion looks bit clearer.
#22
Looks good would like Kyle to commit this and give a once over - I'll happily roll a d6 version of the patch once this is committed.
#23
Port it!
#24
Reroll for d6
#25
Would be nice to get this into 3.2 ;)
#26
looks great.
#27
actually this break in 6.x
#28