Send arrays and call_user_func_array() conflict?

[GoogleCodeExporter]

We are trying to convert from AMFPHP to SabreAMF v1.3.234, and we noticed
some strange behavior when passing arrays through SabreAMF.

For example, our Flex project will call something like this on our server:

$data = array( 
              'filter_data' => array( 
                                     'script' => 'myScript', 
                                     'name' => 'myname' ) 
             );

getData( $data );

This worked great with AMFPHP, however with SabreAMF, it sends the
arguments to the callback function like this:

array( 
              'filter_data' => array( 
                                     'script' => 'myScript', 
                                     'name' => 'myname' ) 
             );

Which on the surface looks fine (no change from the source array), except
for the fact that call_user_func_array() drops the first level of keys in
the array, so by the time the data gets to the actual getData() function,
it looks like this:

array( 
       'script' => 'myScript', 
       'name' => 'myname' 
); 

Its missing the 'filter_data' key entirely, so it obviously causes an error.

This is our original call_user_func_array line:

call_user_func_array( array( &$serviceObject, $methodName ), $arguments )

So first I tried wrapping arguments in an array() like so:

call_user_func_array( array( &$serviceObject, $methodName ), array(
$arguments ) )

Which worked for cases where an array is sent, but it fails for cases when
an array isn't sent, and just plain arguments are used, since SabreAMF
passes those to the callback function as an array already.

So the only solution I could find was this 3 character change to Message.php

Index: Message.php
===================================================================
--- Message.php (revision 2789)
+++ Message.php (working copy)
@@ -154,7 +154,7 @@
                      $body['data'] = $body['data']->getData();
                      $this->encoding = SabreAMF_Const::AMF3;
                 } else if (is_array($body['data']) &&
isset($body['data'][0]) && is_object($body['data'][0]) && $body['data'][0]
instanceof SabreAMF_AMF3_Wrapper) {
-                     $body['data'] = $body['data'][0]->getData();
+                     $body['data'][0] = $body['data'][0]->getData();
                      $this->encoding = SabreAMF_Const::AMF3;
                 }

But I'm not sure if that is the proper way to go about this, or if that
change will break other cases?

Original issue reported on code.google.com by mike.ben...@gmail.com on 7 Sep 2009 at 6:16

[GoogleCodeExporter]

Hey Mike,

I feel like I've gone back and forward with this issue. When calling the 
service in
one way, the arguments will be the first level array, when calling it one other 
way
the arguments will be 1 level deeper. 

Would you be able to work around it, by always wrapping your data in an array 
from
within flex? 

I'm just not sure at this point your fix will break the functionality for other 
people.

For instance.. will this work for people using the flex messaging library.

Original comment by evert...@gmail.com on 7 Sep 2009 at 6:50

[GoogleCodeExporter]

The issue is what is the fool proof test to determine which way to treat the
arguments? Short of making massive changes to all of our Flex code of course?

I don't quite understand why two different data types would be treated 
differently,
especially if the "recommended" method to use is call_user_func_array. It would 
seem
to make most sense that passing them directly to this function should "just 
work". It
seems that is at least how AMFPHP works too. 

You could always make it an option so the developer can choose which method they
prefer, then at least it would be a simple IF statement.

Original comment by mike.ben...@gmail.com on 8 Sep 2009 at 12:37

[GoogleCodeExporter]

I wish I had a better answer than this. If you're able to pinpoint AMFPHP's 
exact
behaviour, and it matches the behaviour of your patch, I'm willing to add it to 
the
next release. 

I don't have a lot of time to spend on this project anymore unfortunately, so 
any
help with the research is appreciated. I am definitely interested to implement 
the
'correct' behaviour. 

Original comment by evert...@gmail.com on 8 Sep 2009 at 2:27

[GoogleCodeExporter]

The patch I attached does make SabreAMF behave like AMFPHP at least for this 
case. We
have a codebase with a few million lines of code using AMFPHP currently that we 
are
in the process of switching to SabreAMF for several reasons, but without this 
patch
virtually every single array we send to the server from Flex is missing the
critically import first level of keys. It also keeps behavior consistent with 
sending
non-array data. 

It seems to me that if a developer didn't want the first level of keys to be 
sent,
they simply wouldn't specify them in the first place, would they not? Of course 
the
first level of keys is passed to the callback function, but call_user_func_array
strips them off.

As far as our testing is concerned, this patch replicates AMFPHP's exact 
behavior
when it comes to arrays.

Original comment by mike.ben...@gmail.com on 8 Sep 2009 at 3:26

[GoogleCodeExporter]

I understand this will fix the issue for your case, but I'm worried there are 
edge
cases where this will fail. As a workaround you could first check if the array 
has
any non-numeric keys. If it does, you know you'll need to wrap the array in 
another
array.

I realize this is not a perfect solution, but I'm just worried this will break 
the
functionality for other cases. I also don't have the resources to test this for 
the
various cases flex may send over the data.

Original comment by evert...@gmail.com on 8 Sep 2009 at 5:33

[GoogleCodeExporter]

Looking at this issue a little more it appears that SabreAMF goes out of its 
way to
make array handling incompatible with call_user_func_array() by silently 
dropping data.

I'm not sure we can support both the existing case and the proposed new behavior
automatically without adding a bunch more overhead, so instead would you be 
willing
to compromise on a config setting that would make array handling compatible with
call_user_func_array? This way we can preserve 100% backward compatibility and 
at
least give others the option. 

I can't see any non-SabreAMF work-around that isn't going to be slow or error 
prone. 

The question is what is the best method to have the developer set this option 
and get
it passed through to the message handler? Should we just duplicate the same 
method
that onInvoiceService is set?

Original comment by mike.ben...@gmail.com on 18 Sep 2009 at 6:19

[GoogleCodeExporter]

Mike, SabreAMF already has one other behaviour changing option, namely:

SABREAMF_OBJECT_AS_ARRAY

if this is set, it will treat objects as arrays. If you want to do a similar 
approach
for this different array behaviour, I'll accept the patch.

If there's ever going to be a 2.0, the behaviour you're looking for will 
definitely
be the default.

Original comment by evert...@gmail.com on 22 Sep 2009 at 7:12

[GoogleCodeExporter]

Okay, I'm not a big fan of using defines like this, rather than just setting an
object variable, but sometimes its just better to keep things consistent.

Any suggestions for a name?

SABREAMF_CALL_USER_FUNC_ARRAY
SABREAMF_CALL_USER_FUNC_COMPAT
SABREAMF_RAW_ARRAY
SABREAMF_RAW_ARRAY_HANDLING

Original comment by mike.ben...@gmail.com on 22 Sep 2009 at 7:52

[GoogleCodeExporter]

Mike,

I tend to agree with you; but for the same reason you mentioned I'd also prefer
staying consistent. If you'd look at some of my newer code, you wouldn't find 
any
defines.

Since the bug is basically not getting the 'true' list of arguments (e.g.: 
wrapped in
an array) in AMF3, how about something like:

How about SABREAMF_AMF3_ALL_ARGUMENTS

Original comment by evert...@gmail.com on 22 Sep 2009 at 8:06

[GoogleCodeExporter]

Thats better than any of my suggestions so far, do you prefer any of these:

SABREAMF_AMF3_PRESERVE_ARRAYS
SABREAMF_AMF3_PRESERVE_ARGUMENTS

I don't really care either way, it would just be nice to come up with a name 
that
suits the function as close as possible.

Original comment by mike.ben...@gmail.com on 22 Sep 2009 at 8:33

[GoogleCodeExporter]

yes! SABREAMF_AMF3_PRESERVE_ARGUMENTS 

Original comment by evert...@gmail.com on 22 Sep 2009 at 8:36

[GoogleCodeExporter]

Also.. would it be helpful for you to have svn access? 

Original comment by evert...@gmail.com on 22 Sep 2009 at 8:36

[GoogleCodeExporter]

It definitely would be if you'd prefer to have me directly check-in my changes 
rather
than post the patches here.

Original comment by mike.ben...@gmail.com on 22 Sep 2009 at 9:37

[GoogleCodeExporter]

I added you to the contributor list! thank you sir!

Original comment by evert...@gmail.com on 22 Sep 2009 at 10:24

[GoogleCodeExporter]

This is committed as #246:

http://code.google.com/p/sabreamf/source/detail?r=246

Original comment by mike.ben...@gmail.com on 23 Sep 2009 at 7:30

[GoogleCodeExporter]

Saw it, thanks!

Original comment by evert...@gmail.com on 23 Sep 2009 at 8:33