search

woohoolabs / yang

The efficient and elegant, PSR-7 compliant JSON:API 1.1 client library for PHP
MIT License
168 stars 13 forks source link

ClassDocumentHydrator::hydrateSingleResource() enforces use of stdClass #25

Closed holtkamp closed 5 years ago

holtkamp commented 5 years ago

See: https://github.com/woohoolabs/yang/blob/acdd0a11383bd23f405a4d42f4cea22f8f41eb57/src/JsonApi/Hydrator/ClassDocumentHydrator.php#L29

Which prevents the use of a custom hydrator which returns other objects.

PR here: https://github.com/woohoolabs/yang/pull/24

holtkamp commented 5 years ago

@kocsismate do you need meer information on this issue and PR? Or are the intents clear?

kocsismate commented 5 years ago

Hey @holtkamp Sorry for the delay!

Yes, the intents are clear (and I agree with it completely), my only problem is (and it's the reason I haven't answered yet) that technically, it's a BC break. :/ I am not sure though that the change breaks any compatibility in the practice. What do you think about the BC aspect of the change?

holtkamp commented 5 years ago

technically, it's a BC break.

mmm, is it?

An object is "looser" than specifically an instance of stdClass...

All instances of stdClass are an object, so AFAIK this is not a BC break but a bug fix :neckbeard:

I think this stdClass return type was simply "forgotten", so I added a test to never forget it again 😄

So a patch version 2.2.1 would suffice to release this.

kocsismate commented 5 years ago

I said that it's a BC break mainly because if someone overrides the hydrateSingleResource() then they will face an issue. At least below PHP 7.4 😎 https://3v4l.org/YaolE

All in all, let's merge your PR, it brings quite some benefits while its BC break aspect doesn't seem much in practice. Thank you for your work!

holtkamp commented 5 years ago

I said that it's a BC break mainly because if someone overrides the hydrateSingleResource() then they will face an issue. At least below PHP 7.4 😎 https://3v4l.org/YaolE

aaah, did you mean it like that. Mmm, yeah that is true.. Fingers crossed nobody did that yet 😉

Thank you for your effort!