json-api-php / json-api

Implementation of JSON API in PHP
https://json-api-php.github.io/
MIT License
183 stars 18 forks source link

Fix missing data if it was passed later using attachTo method #94

Closed seriy closed 5 years ago

seriy commented 5 years ago

Example that will work after this commit:

$error = new Error(); (new Error\Code('code')->attachTo($error);

$jsonapi = new JsonApi(); (new Meta('key', 'value'))->attachTo($jsonapi);

f3ath commented 5 years ago

Thanks for the PR. It's not quite clear what it purpose is tho, could you give a bit more details please? Also some tests seem to fail after this change.

seriy commented 5 years ago

In the current version, when adding data using the attachTo method, they disappear after adding the Error object to ErrorDocument

f3ath commented 5 years ago

Travis builds were failing. I fixed those and took the liberty of updating your PR as well so we can see if the tests are still failing.

f3ath commented 5 years ago

Example that will work after this commit:

$error = new Error(); (new Error\Code('code')->attachTo($error);

$jsonapi = new JsonApi(); (new Meta('key', 'value'))->attachTo($jsonapi);

This use case also goes against immutability which is one of the features of this implementation. All dependencies are supposed to be passed to the constructor of the object being built. If you need to build something dynamically, gather the dependencies first and then pass them using the array spread operator, like this

<?php

$error_members = [ new Id('1'), new AboutLink('/errors/not_found')];
if ($need_meta) {
  $error_members[] = new Meta('foo', 'bar');
}
$error = new Error(...$error_members);

Does this make sense?

f3ath commented 5 years ago

In fact, the attachTo() method is a part of Attachable interface which is marked as @internal. That means that despite being public it must not be used in the client code.

seriy commented 5 years ago

Thank you, now everything has become clear. Maybe it makes sense to add @internal to the attachTo method to notify user not to use it?

f3ath commented 5 years ago

Makes sense to me. Thanks.