Skip to content

Conversation

seriy
Copy link
Contributor

@seriy seriy commented Sep 9, 2019

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
Copy link
Contributor

f3ath commented Sep 9, 2019

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
Copy link
Contributor Author

seriy commented Sep 9, 2019

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

@f3ath
Copy link
Contributor

f3ath commented Sep 9, 2019

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.

src/JsonApi.php Outdated
@@ -8,15 +8,13 @@

final class JsonApi implements MetaDocumentMember, DataDocumentMember, ErrorDocumentMember
{
private $obj;
public $version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes the object mutable. Why is it necessary?

@f3ath
Copy link
Contributor

f3ath commented Sep 10, 2019

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
Copy link
Contributor

f3ath commented Sep 10, 2019

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
Copy link
Contributor Author

seriy commented Sep 10, 2019

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 f3ath merged commit 4fb3c7a into json-api-php:master Sep 10, 2019
@f3ath
Copy link
Contributor

f3ath commented Sep 10, 2019

Makes sense to me. Thanks.

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

Successfully merging this pull request may close these issues.

2 participants