Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON structure changed between v0.19.2 and v0.20.0 #551

Closed
dwightwatson opened this issue Mar 27, 2022 · 13 comments · Fixed by #553
Closed

JSON structure changed between v0.19.2 and v0.20.0 #551

dwightwatson opened this issue Mar 27, 2022 · 13 comments · Fixed by #553

Comments

@dwightwatson
Copy link

I've noticed that in upgrading from v0.19.2 to v0.20.0 the output of my JSON responses changed. Reading through the release notes I can't spot what change would have caused this new behaviour so I'm not sure if this is expected behaviour or not. I'm a little confused because the release notes seemed to indicate this was just updates for supported PHP versions.

0.19.2

^ array:1 [
  "data" => array:2 [
    "id" => 2
    "subject" => array:4 [
      "id" => 2
      "code" => "ARTS1000"
      "name" => "Public-Key Multi-State Functionalities"
      "institution" => array:4 [
        "id" => 5
        "name" => "Owen Heaney DDS"
        "short_name" => "Prof. Mireille Harber PhD"
        "abbreviation" => "Lavinia..."
      ]
    ]
  ]
]

0.20.0

^ array:1 [
  "data" => array:2 [
    "id" => 2
    "subject" => array:1 [
      "" => array:4 [
        "id" => 2
        "code" => "ARTS1000"
        "name" => "Persevering Clear-Thinking Graphicaluserinterface"
        "institution" => array:1 [
          "" => array:4 [
            "id" => 5
            "name" => "Marge Sporer"
            "short_name" => "Marshall Ankunding"
            "abbreviation" => "Camryn B..."
          ]
        ]
      ]
    ]
  ]
]

Note that previously the default included subject and institution were nested directly - now instead they are nested under an "empty" string. I am using a custom serializer which is intended to use data for the root level item only.

namespace App\Http\Responses\Serializers;

use League\Fractal\Serializer\DataArraySerializer;

class RootSerializer extends DataArraySerializer
{
    /**
     * Serialize a collection.
     *
     * @param  string  $resourceKey
     * @param  array   $data
     * @return array
     */
    public function collection($resourceKey, array $data)
    {
        return is_null($resourceKey) ? $data : [$resourceKey => $data];
    }

    /**
     * Serialize an item.
     *
     * @param  string  $resourceKey
     * @param  array   $data
     * @return array
     */
    public function item($resourceKey, array $data)
    {
        return is_null($resourceKey) ? $data : [$resourceKey => $data];
    }
}
@chris-rs
Copy link

chris-rs commented Apr 1, 2022

I have exactly the same 'root' serializer. Seems the $resourceKey is now always an empty string, while until v0.19.2 it was FALSE or NULL.

@chris-rs
Copy link

chris-rs commented Apr 1, 2022

Investigated some further: $resourceKey holds the value you can supply as third parameter when calling collection/item methods. While serializing the $resourceKey will be the supplied value if $data contains the root otherwise it will be an empty string.
This indeed changed in v0.20.

If you adjust the above code to below it will serialize as expected:
return empty($resourceKey) ? $data : [$resourceKey => $data]

@matthewtrask
Copy link
Contributor

@dwightwatson since the RootSerializer is custom and extending the DataArraySerializer, can you provide more information such as how you are calling so I can try to recreate this on my machine?

cc/ @chris-rs

@dwightwatson
Copy link
Author

I'll have re-confirm because it's been a long time since I wrote this code - but I believe the intention was that the root level item in the response was nested under a data key, but every other includes is nested directly under it's name - not under another data key.

You can see in my example above you used to be able to get data.subject.id. Without my custom serializer it would have been data.subject.data.id which was what I was trying to avoid.

Let me know if that makes sense - alternatively perhaps I can put together a sample repo that demonstrates the break?

@chris-rs
Copy link

chris-rs commented Apr 4, 2022

@matthewtrask the serializer works like @dwightwatson describes. I also implemented this years ago. The trick is that we like to add the 'data' value in front of root items and strip it in nested structures.
I think that the problem is here that the code was refactored with typed parameters.

Until v0.19:
public function collection($resourceKey, $data)
$resourceKey can be FALSE or the supplied key value and NULL if empty, which is in my humble opinion an unwanted flexibility in PHP. Since a while type hinting can be used which prohibits this.

Since v0.20:
public function collection(string $resourceKey, array $data): array
$resourceKey is now forced to hold string values which is a good thing! But this breaks with old implementations that expect it to be FALSE, NULL or the supplied key value. The 'good' thing about the v0.19 implementation is that you could detect root items from nested items in $data. $resourceKey only held the key value (or NULL) if $data contains the root, otherwise it is FALSE.
Since v0.20 we only get the string $value (supplied key or NULL) and have no way to detect root items anymore.

Hope this makes sense.

@matthewtrask
Copy link
Contributor

alright, let me try to write a test to see if the fix I have in mind will work. Will probably be later today for me after work.

@matthewtrask
Copy link
Contributor

@dwightwatson "alternatively perhaps I can put together a sample repo that demonstrates the break?" do you mind doing this? Even just something simple would be super helpful. im trying to write some breaking tests but not quite getting it to break the way you are describing. Im probably just missing something super obvious but the sample repo would be a huge help if possible.

@dwightwatson
Copy link
Author

Thanks for your patience while I put this together. Here's a pretty basic PHP repro that has distilled the framework code out: https://github.com/dwightwatson/fractal-issue

The tests pass on the 0.19 branch, but they fail on the 0.20 branch.

You can see the only changes in the branches apart from a junit report I accidently committed are:

  • Bumping the Fratcal version with Composer
  • Adhering to the require typehints on the seralizer and transformer attributes

Hope this helps, please let me know if there is more I can do to help here.

@matthewtrask
Copy link
Contributor

@dwightwatson this is fantastic, thank you so much for this and thank you both for your patience as I look into this,

@matthewtrask
Copy link
Contributor

@dwightwatson @chris-rs alright I have a solution. Not super happy with it, but it works. I will write a test in the main library today and cut a 0.20.1 release for yall to have. In fact I think I can just take your test from the example @dwightwatson if that works for you.

@dwightwatson
Copy link
Author

Yeah of course, happy for you to use that code however you need.

@matthewtrask
Copy link
Contributor

@dwightwatson @chris-rs thank you both for your help and patience. here is the new release that should fix the issue!

https://github.com/thephpleague/fractal/releases/tag/0.20.1

@dwightwatson
Copy link
Author

Looks good to me - thanks so much getting to the bottom of this and shipping a fix! 🤟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants