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

Paginator links property is either empty array or filled object #433

Open
tomvo opened this issue Jan 7, 2018 · 7 comments
Open

Paginator links property is either empty array or filled object #433

tomvo opened this issue Jan 7, 2018 · 7 comments
Labels

Comments

@tomvo
Copy link

tomvo commented Jan 7, 2018

As per this question in a wrapper library spatie/fractalistic#36 (comment), I'm noticing difference in the response for the ArraySerializer for when the pagination has links or not.

When there are no links for the pagination the meta property looks like this:

"meta": {
        "pagination": {
...
            "links": []
        }
    }

When there are, it looks like this:

"meta": {
        "pagination": {
...
            "links": {
                "next": "[url]"
            }
        }
    }

You can see that in an empty resultset the links property is an array and when set, it's an object. This causes issues for devices consuming our API.

This issue is caused by the default ArraySerializer creating the links property as an empty array ([]), see here: https://github.com/thephpleague/fractal/blob/master/src/Serializer/ArraySerializer.php#L105.
The solution to this would be to create an empty ArrayObject instead of a normal Array.

Of course I can fix this for my case with overloading the paginator method but would you accept a tested PR for this or is this too breaking?

@ejunker
Copy link

ejunker commented Mar 13, 2018

I also have this issue. It would be nice if it was consistent and always returned an object for the links and returned {} instead of [] when there are no links.

As @tomvo mentioned it looks like this could be solved with a one line change in ArraySerializer.php

Change from:

$pagination['links'] = [];

to

$pagination['links'] = new \ArrayObject();

@squareborg
Copy link

I agree with this, One of our App guys has issues when consuming this with Swift. Is there a reason this cannot be fixed since it appears to be quite trivial?

@willishq
Copy link
Member

It appears to be quite trivial until you see under the hood, this is using associative arrays. Because of that, its very difficult to determine if an empty array is an empty associative array or an empty object.

I did try to do a fix for this over a year ago using \ArrayObject() but it created more problems, it was like going down a rabbit hole and I didn’t have THAT much free time at the time.

I will revisit this, it is a known issue and one we’ve been pondering for way to long now.

@willishq
Copy link
Member

Ignore me, I was talking about this issue for empty objects in the data namespace. Everywhere that data is created, it creates it as an empty array. Fixing that issue creates problems.

Fixing this issue does not, in my opinion. If you’ve written code to treat an empty array as an empty object, and you get an empty object, would that cause breaking changes? Only if somebody is relying on a property being an array to determine if an object is empty. How likely is that?

I would love to fix this but it’d probably need a major version bump to appease people who have hacked in workarounds that would break with the fix, but then it’s not worth doing this fix without the other \ArrayObject() foxes because people would just be unhappy at a half-finished job.

How about… I add another serializer: DataArrayObjectSerializer And put this functionality in there? Anybody have any issues with that?

@lordrhodos
Copy link

just hit the same issue when testing our OpenAPI specs against the API output. Would love to see an empty object here as well 👍

@lordrhodos
Copy link

How about… I add another serializer: DataArrayObjectSerializer And put this functionality in there? Anybody have any issues with that?

I have solved this by extending the ArraySerializer and overriding the pagination method, so if the library ships with an alternative serializer one could use to achieve these changes that would be great.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 4 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants