json-api icon indicating copy to clipboard operation
json-api copied to clipboard

Relationship data generator is invoked more than once if data is empty

Open lindyhopchris opened this issue 4 years ago • 16 comments

I've been getting this error:

Exception: Cannot traverse an already closed generator
src/Parser/RelationshipData/RelationshipDataIsCollection.php:132

It only occurs when relationship data is empty. The cause of the bug is these lines here: https://github.com/neomerx/json-api/blob/master/src/Parser/RelationshipData/RelationshipDataIsCollection.php#L130-L131

As it does not assign an empty array to $this->parsedResources before iterating over the resource, the parsedResources property will remain null. Therefore when it is called a second time, there is no cache to use and the closed generator is used.

I'll submit a PR with a test reproducing the problem and a fix.

lindyhopchris avatar Feb 03 '21 15:02 lindyhopchris

@neomerx see you haven't been active on Github since March last year. Hope everything is ok.

Hope you can tag the fix as 4.0.2. If you need help maintaining the package going forward, do let me know.

lindyhopchris avatar Feb 03 '21 16:02 lindyhopchris

Thanks! I also started my own fork as I need to add the missing pieces to the API, such as pagination and filtering.

mahagr avatar Feb 19 '21 11:02 mahagr

@mahagr looks like I'm going to have to run a fork too, as I'm not sure that PR is going to get merged. My fork is here: https://github.com/laravel-json-api/neomerx-json-api

lindyhopchris avatar Feb 19 '21 13:02 lindyhopchris

Hey @neomerx, do you need some help maintain this repo?

PabloKowalczyk avatar Jan 30 '22 17:01 PabloKowalczyk

I am wondering the same. Otherwise, it would make sense to have an official fork instead of everyone doing their own.

mahagr avatar Jan 31 '22 09:01 mahagr

Hey it seems this project is not maintained anymore. Did you agree which fork should be the "official" one?

gfemorris avatar Apr 01 '22 14:04 gfemorris

I can create and maintain something like json-api-php/json-api.

PabloKowalczyk avatar Apr 01 '22 14:04 PabloKowalczyk

@lindyhopchris created a fork and already has v5.0.0 in it maybe he wants to or you can adapt his changes?

gfemorris avatar Apr 01 '22 15:04 gfemorris

Yeah I'm happy to maintain my fork because it's a critical part of Laravel JSON:API. I'm not up for doing any re-writing, more just keeping it up-to-date for PHP versions and with the spec if that does change (though it's taking them ages to get to 1.1 any way, so doubt we'll be talking about 1.2 any time soon).

lindyhopchris avatar Apr 01 '22 16:04 lindyhopchris

Making it (fork) framework/library agnostic should be better for long term maintenance.

PabloKowalczyk avatar Apr 01 '22 16:04 PabloKowalczyk

Yeah to be clear, even though it's in the laravel-json-api organisation, it'll always be left as framework agnostic. That's because I wrap it in another package to tie it into the Laravel JSON:API implementation - i.e. there's a laravel-json-api/encoder-neomerx package that does that wrapping.

So my fork will always remain framework agnostic.

lindyhopchris avatar Apr 01 '22 16:04 lindyhopchris

But name can be confusing for newcomers :)

PabloKowalczyk avatar Apr 01 '22 18:04 PabloKowalczyk

Yeah can see that. FWIW I'm planning on keeping my fork for the time being. If there comes a point where there's another fork that is fully supported and maintained, then I can switch to that in the future.

lindyhopchris avatar Apr 01 '22 19:04 lindyhopchris

Alright, I'm going to look into switching to laravel fork myself when I get back to the API.

mahagr avatar Apr 04 '22 07:04 mahagr

Thank @lindyhopchris .. i will switch to your fork next time i'll do any upgrades.. thanks for your work.

gfemorris avatar Apr 05 '22 08:04 gfemorris

Hello guys, it's been a while but I started my fork of this library - https://github.com/jsonapiphp/jsonapi. It started with v1.0.0 which is the same code as v4.0.1 of this lib, so you can change them easily. My fork will concentrate on performance and jsonapi specification compatible.

PabloKowalczyk avatar Jun 11 '22 15:06 PabloKowalczyk