Fix for argument source handling in REST Server

So I remember committing this in order to fix a bug a while back here

https://github.com/heyrocker/services/commit/08ae9ab8b5ddbc9934e3a43cd5b...

I don't think it was related to the commit you posted (or if it was I couldn't see it.)

Unfortunately I don't remember what problem I was trying to fix there, even though I left a comment on it. It looks to me like it was trying to grab all GET arguments as a parameter in situations when it shouldn't. Does this make sense to you? Sorry I don't have more details, it was several months ago.

Both of these patches result in 39 failed tests. Either the tests break after using this, or this patch is really not needed.

Im not entirely sure I understand the issue?

Both of these patches result in 39 failed tests. Either the tests break after using this, or this patch is really not needed.

Im not entirely sure I understand the issue?

Applying these patches causes node_resource, among others to fail.

FIrstly, function _node_resource_access($op = 'view', $args = array()) returns false because $args is now an array with index 0 thats equal to an array of ['node'].
All of the code is expecting that the $args(which should probably be called $node all things considered) that its the only argument we need.

Second, _node_resource_create fails because its expecting $node to come through, but instead its now array(1) {
["node"]=>
<(3) {
["title"]=>
<(4) "test"
["type"]=>
<(5) "story"
["body"]=>

It doesnt make any sense to to say $node = $args['node'], when we can have the argument build function handle the correct argument

I think the way we have it makes much more sense. Each parameter that the function requests, its gets, as opposed to all of them and in some array.

Maybe all of our resources functions need to be changed to accommodate this, but if thats the case, this patch needs work.

If I understand this issue correctly then I surely think that it should be corrected.

However - we're rather late in the development cycle - we've gotten a few rc:s out there and this is quite a big API change so I think we would need to ease the changes needed by users as much as possible. We could perhaps add code that emulates the current "wrong" behavior in the resources?

Who would like to make a patch? Hugo? Kyle? Could the one of you who would like to roll a patch assign this issue to himself?

Ill fix the tests

ok, how about this as a patch for d7.

Looking at the code and the commit refered to in the issue description then I don't see how any of these patches restores the old functionality of:

// When the type is 'array' or 'struct', we assume that it requires
// the posted data as one big chunk (as in when posting a new node.)
// Otherwise, retrieve each piece of data as individual named 
// arguments (as in when posting an action.)
if ($info['type'] == 'array' || $info['type'] == 'struct') {
  $arguments[$i] = $sources[$info['source']];                
}
else {
  //added so Default values in modules work as expected, otherwise they might 
  //get set to NULL which is different than 0
  //IE taxonomy_service_get_tree
  if(isset($sources[$info['source']][$info['name']]))
  $arguments[$i] = $sources[$info['source']][$info['name']];
}

Neither of these two patches makes any good use of the argument type - why are we defining a type if we don't care about it? If we only go by name or by source type then the argument type is useless? Shouldn't we perhaps take the argument type into consideration?

Couldn't an alternate solution to this be to add some new data sources instead? The new data source could explicitly be unsupportive of data types and always return the full source - like what Hugo wants. I attach a patch for that suggestion.

Edit:

Also - how are these changes affecting the way the resources works in other servers like XML-RPC of AMF?

IM not sure will that leaves this patch....

After thinking about it I'm with hugo on this one - the first patch here is what we should go with. It just need to be extended with changes in tests and resources to accommodate the "new old" behavior as well as to make the resources still accept the old kinds of argument (something that the resources should do in themselves - the rest server doesn't have to do anything about it).

So - we need changes in tests and resources as well before RTBC.

Whats wrong with the patch I provided? It allows the old arguments to still be accepted, and it requires no changing one resource or test.

@kyle: That it prohibits any parameter to contain an array with a parameter of it's own name which could lead to very weird bugs

Can some confirm my understanding of this issue is correct?

If a resource has 3 parameters I can pass in array('parm1', 'parm2', 'parm3') vs posting parm1, parm2, parm3 as seperate strings and then the server converts the array into arg understood by services?

Are both options are allowed depending on the server? If so then I'm not massively concerned and would say the old style servers (xml-rpc, amfphp) can happily remain with args being specified at a string level as I can't see many site posting the same request to xml-rpc and json or indeed switching between the protocals.

Thats what my patch in #12 fixes. It accepts args the old way, and the way Hugo intended them to be accepted, theres nothing else that needs to be changed.

Hugo thanks for your illustration that makes sense and was kind of what I was getting at but I failed to explain it very well. I agree we need to use an approach that does not affect backwards comptability as there are now sites based on on 3.x services (and personally from the work I have been doing in tests I like the current approach but making the calls lighter and hence more flexible also makes sense).

I think the patch proposed by Kyle is a sensible starting point as it takes the current approach and extends it with Hugo patch propose initially in this issue, it does have some style issues and could be simplified as an elseif statement.

It might be worth getting heyrocker opinion on this issue so as we can get this resolved and make services even better :)

As I said in #18 I think that kyles patch introduces some inconsistencies by prohibiting any full object argument from containing a key with the name of the argument itself. That is not something you would expect the code to do and thus it can lead to nasty errors that are hard to debug.

The full object should take precedence over a named parameter to ensure a consistent behavior. That means that we can't fix the backwards compatibility in the argument resolver but needs to fix it within the resources themselves.

I 'm therefor suggest that we go with Hugo's original patch and patch our own resources to be backwards compatible. I'm attaching a patch that fixes some of our resources.

Can you write a test for this. I still dont understand after our conversation in IRC. Bumping to critical as this is a launch blocker.

Rerolled a patch containing the original approach and #24.

* Argument handling as suggested
* All resources backwards compatible
* Adjusted tests accordingly
* All tests passed before and after modification

All of your tests pass but only for the new way, How can we be sure the old way still works?

@kylebrowning: The old way can't work any more - it's a regression that it's impossible to support any longer (And yes - I need to take some time to show you that in the form of a test - but I haven't had the time to do that yet)

I thought we decided that the old way must continue to work since we are in release candidate.

Oh yeah - forgot that we was going to achieve that at a resource level - true - that's what should be tested - that the current resources works with old type of requests. So we need to duplicate all the tests of the affected resources :)

instead of duplicating, it might be possible to have each test call itself but pass in a value like true for old way. So the first pass through tests the new way when those tests are done, call the same tests again but pass in TRUE, and then test the old way.

Or maybe thats too complicated, but doubling our tests just sounds like well have way too many tests :(

Duplicating would be less time consuming. And it'll be easier to remove them in future versions.

ok just duplicate them then. we also need this for 6.x as well :(

Rerolled the patch with legacy tests included.

Just finished the tests. All pass without errors.

I will try to backport this to D6 when the patch for D7 is ready to go.
Review anyone?

This look great. All tests pass, and it works well. thank you!

Does anyone know something about the status of simpletest in D6?
It seems it doesn't install autoload module and generates a bunch of failures when testing services.

hrmm, the tests all pass for me. on php 5.2 and php 5.3

Allright, attached is a tested patch for D7 and an untested one for D6.
I tried testing D6 on a fresh Drupal install in my DEV environment and on two different freshly installed VMs (Ubuntu 10.04, Ubutu 10.10) without success.

Could someone else take over, please?

Works for me! (d 6.20)

There was one minor fix for Drupal 6. Ive fixed that and committed the patch in #41, All tests pass.

Now on to d7.

Fixed, committed in both d6 and d7.

Hi, sorry for bothering you guys about this closed issue. However, I just upgraded to services 6.x-3.0-rc2 and run a couple of tests to retrieve node resources. So basically a simple http://mydomain.com/rest/node/666. This did not end up successfully and i got no results returned...

I tried to figure out what the problem is and came to function _node_resource_access().

At beginning of the function there's param $args:

Array
(
    [0] => 666
)

Just after the first condition if (isset($args[0]['node'])), variable $args contains:

Array
(
    [0] => 6
)

There's apparently no array key node available. Nevertheless, the isset condition appears to return true and the new $args[0] value is taken from $args[0]['node'] (which is 6? interesting...).

Can't explain why isset() returns true despite that args[0]['node'] does not exist (apparently "is not set"). What did it for me was adding array_key_exists() to the condition (is_array() is required otherwise array_key_exists returns a warning as "The second argument should be either an array or an object"):

  if (is_array($args[0]) && array_key_exists('node', $args[0]) && isset($args[0]['node'])) {
  	$args[0] = $args[0]['node'];
  }

I've only checked node resource so far, maybe it's the same for other resources too.

Duplicate of #1236316: Retrieve requests for node, user, comment file resource return empty result

Can you please please provide a patch?

Finding that this commit breaks everything took some time this afternoon.

hook_services_resources() needs updating. I would even go so far as to update the release notes with how critical of an API change it is.

Should I open this back up or open a new issue?

@mradcliffe: New issue - this one has been solved - any issues it might has caused should be posted as follow up issues. You might link to them from here though if you want to make sure that everyone here knows about them.

Why the data array size is limited to 1 on line 374

count($data) == 1

Why there is this control?