On line 422 in services.module (v 1.8.2.88.2.14 2009/12/11) there's this line:

$result = call_user_func_array($method['callback'], $args);

$args is an array of arguments to the callback, for example:

Array
(
    [0] => 5
    [2] => Test
)

Following the previous logic, the key denotes the position of the argument in the function signature. However, call_user_func_array() cannot use keys for argument position. In effect, what happens is this:

callback(5, 'Test');

not this:

callback(5, null, 'Test'); as intended.

I've been googling and pouring over the relevant pages at php.net without finding anything to support the idea of indexes as argument positions in the function called. It looks like this will have to be implemented manually by creating an array of the same length, then combining the two.

CommentFileSizeAuthor
#11 715044-services-callback.patch468 byteskylebrowning
#10 echo.tar_.gz727 bytesygerasimov
#8 715044-services-callback.patch666 bytesygerasimov

Comments

marcingy’s picture

marcingy commented
Status: Active » Closed (won't fix)

Marking as won't fix and if this is a real issue then it is an issue with php/drupal core rather than services.

solipsist’s picture

solipsist commented

Can you explain your reasoning behind deeming this an issue with PHP and not Services? Even if it could be considered a bug in PHP, a workaround in Services is needed since call_user_func_array() doesn't work as expected. I'm prepared to write a patch.

solipsist’s picture

solipsist commented

I can see a case for not considering this an issue. A well-defined method wouldn't order arguments without regards to whether they're required or not, in which case this won't be an issue.

However, assuming we have this function:

function($one, $two = null, $three = null, $four = null, $five = null, $six = null) { ... }

Using the JSON-RPC server, which can take arguments by name, we can pass just a few of the arguments, say argument 1, 4 and 6:

Example 1:

{
  one : 1, //required
  four : 4, //optional
  six : 6 //optional

}

Instead of having to pass a bunch of NULL values just to match the function signature (which is internal to the actual implementation and should not be exposed through the API):

Example 2:

{
  one : 1, //required
  two : null, //optional
  three: null, //optional
  four : 4, //optional
  five : null, //optional
  six : 6 //optional

}

Using example one above, the way it works now the function call would be function(1, 4, 6), which is not what we intended, we'd rather see this: function(1, null, null, 4, null, 6), without having to resort to doing example two.

However allowing calls by parameter name and hiding argument order would mean the server implementation would have to assume default values for the arguments not included in our call, more precisely: two, three and five, since we cannot dynamically get the function signature while the code is being executed.

A solution would be to include a #default key in hook_service for each argument defined. Basically, every argument set #optional = TRUE could also have a #default key defined.

solipsist’s picture

solipsist commented
Status: Closed (won't fix) » Active
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Active » Postponed

I can see the point being made here, and I understand how it can cause problems and would be nice to fix. However it just seems like too major a change to be made this late in the release cycle (I hope to roll a stable release next week or possibly sooner) especially since it can easily be worked around easily by the caller. If you want to work on a patch for 2.1, I'd be more than happy to look at it. Marking this postponed for the time being.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Version: 6.x-2.0-beta1 » 6.x-3.x-dev
Status: Postponed » Active

I definitely think this is a good change for 3.x, and it ties into #388236: Update Method Declaration to be in Drupal Style in terms of naming.

iTiZZiMO’s picture

iTiZZiMO commented

this function makes also problems on system resource, so i had to disable it

i had to disable the following code part (line 139, services.runtime.inc)

/** Call default or custom access callback
if (call_user_func_array($controller['access callback'], $access_arguments) != TRUE) {
global $user;
return services_error(t('Access denied for user !uid "@user"', array(
'!uid' => $user->uid,
'@user' => isset($user->name) ? $user->name : 'anonymous',
)), 401);
}
**/

no i can get a session id via path/to/endpoint/system/connect

is there any possibilty to fix this issue?

ygerasimov’s picture

ygerasimov commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new 715044-services-callback.patch666 bytes

I this is really critical issue when it comes to passing arguments to resource callback. Find attached patch that just fix this particular case, but the problem is deeper.

I think it would be nice to not use not keyed arguments arrays at all. Also to pass arguments array to resource callback and not every argument separately.

I understand that this is cruicial changes that change API so I would like to hear comments from maintainers whether it is nice feature to implement.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Postponed (maintainer needs more info)

I think this has been fixed in 3.x-dev that I committed today, can anyone verify? If not, explain to me what is going on here.

ygerasimov’s picture

ygerasimov commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new echo.tar_.gz727 bytes

No. The issue is not fixed in current 3.x-dev version. Please install attached echo resource. When you enable it try to see the result of the call:

http://_host_/_endpoint_/echo?arg2=2

You will see that value of $_GET['arg2'] gets passed to argument $arg1. So I reply will be:

<?xml version="1.0" encoding="utf-8"?> 
<result><arg1>2</arg1><arg2/><arg3/></result>
kylebrowning’s picture

kylebrowning commented
StatusFileSize
new 715044-services-callback.patch468 bytes

I think this patch is a lot cleaner and solves the problem.

One thing you never did was set the default value for the info of the argument

  $args[] = array(
    'name'           => 'arg1',
    'type'           => 'string',
    'optional'       => TRUE,
    'source'         => array('params' => 'arg1'),
    'description'    => t('Argument 1.'),
    'default value' => NULL,
  );

Either way, this new patch takes into account if you set it null, or it does not exist.

[EDIT] Typo fixes

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Reviewed & tested by the community

This patch works properly (that is much nicer place to fix) and does fix the bug

kylebrowning’s picture

kylebrowning commented

This issue has been fixed and I will update the documention for the api per ygerasimov's suggestion

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.