Fix loose of information when GraphNode convertion

[tolbon]

fixes #700
Maybe a patch to this issue #700

[tolbon]

I miss something wait please

[tolbon]

That's good now

[yguedidi]

Not sure about this solution... I'll investigate when I have some time.

[tolbon]

I am just thinking about that but I need to test nested request.
What do you think for better solution ? Merge all extra variable in ['data'] ?
The only problem with my solution is : you have graphNode with extra variable and an array 'data' with the rest of variable
And you do
´´´php
getField('data')['name of field in data']
´´´

[yguedidi]

Thanks @tolbon for this PR! Can you please rebase it and apply those small fixes? Could you also update the changelog file? Thanks

[yguedidi]

I'd add a line above to separate if's

[yguedidi]

I'd move this comment above the if

[yguedidi]

@tolbon @SammyK I still think this in not the right way to fix the issue, but mine is a BC break (keeping the type of fields, array or object, instead of always get arrays on json_decode).
As this fix doesn't break anything, I'm OK with it! Thanks @tolbon and sorry for the delay!

[yguedidi]

Can you also add a test covering this case so we avoid any future regression? thanks

[yguedidi]

@tolbon can you update this PR? I'm about to tag a new release soon. Please rebase, update the changelog and add tests.
I'd test 2 cases: one with only the data key, and one with data and other fields.

[jonny-no1]

This pull request now says

@tolbon wants to merge 3 commits into facebook:master from unknown repository.

I've googled it and it sounds like the branch was deleted on the fork. There also hasn't been a reply from them in over a year. I'll open a duplicate PR with @tolbon's fix and my test from #1034.

[tolbon]

@foaly-nr1 If you can do this very helpfull, thanks.

[jonny-no1]

I've created a duplicate of this PR under #1085. The tests fail so this is not a fix for #700.

There was 1 failure:

1) Facebook\Tests\GraphNode\GraphNodeFactoryTest::testAGraphNodeWithARootDataKeyWillConserveId
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'id' => '123'
-    'name' => 'Foo McBar'
-    'link' => 'http://facebook/foo'
+    'data' => Array (...)

I suggest to reconsider #1034 which offers a solution.

[tolbon]

ok I remember test with that

{
  "id": "123456",
  "picture": {
    "data": {
      "is_silhouette": false,
      "url": "https://scontent.xx.fbcdn.net/p50x50/foo.jpg"
    }
  }
}

yours is certainly better and resolve more case 👍

[yguedidi]

@foaly-nr1 thanks! How I see the fix, to be generic and don't destructure the Graph response:

• if only a data key and no other keys (like picture in @tolbon above example): use the data for the node ($data = $data['data'])
• otherwise, no changes, data key is a normal field like any other

[yguedidi]

@foaly-nr1 @tolbon what do you think about this? :)

[tolbon]

It's ok for me

@foaly-nr1 do you have true facebook call where your test is like that ?

[jonny-no1]

@yguedidi yes that would work as it would preserve an id element that is next to a data element. I do believe that is the right way forward as it minimises data modification. Quite a big backwards compatibility break though?

I can update #1085 accordingly.

[yguedidi]

@foaly-nr1 good catch! It is a BC break.. I'll think about a way to fix the bug in a BC way when I have time (in fact I already have an idea that I need to confirm, but in France it's time to sleep). Meanwhile, any proposal to achieve that result? :)

[yguedidi]

@foaly-nr1 my solution is a totally different solution from the proposed one, is it OK for you if I open my own PR?
EDIT: OK, basicly my solution is the same than the one you proposed in #1034, but extending it to more than just the id key. I'll continue the discussion in the other PR.