Skip to content

Conversation

Art4
Copy link
Owner

@Art4 Art4 commented Oct 9, 2015

This PR brings a big change how objects would be injected. The goal of this PR is to set all relevant classes to final to protect them from overriding. The final classes will allow me to react for changes in future JSON API releases without to worry about BC breaks.

All changes:

  • (Nearly) Every object is set to final. Extending would not be possible anymore. Implement the interfaces instead.
  • Every object has get his own Interface like DocumentInterface, Resource\CollectionInterface and so on
  • PaginationLink was renamed to Pagination.

I will provide some examples in the docs, how own objects must be injected in future.

@Art4 Art4 added this to the 0.5 milestone Oct 9, 2015
Art4 added a commit that referenced this pull request Oct 12, 2015
@Art4 Art4 merged commit adbfdde into master Oct 12, 2015
@fcastilloes
Copy link

Sorry I didn't say anything before, but thinking about extensibility, this solution leaves me with the responsibility of maintaining myself all validation and parsing of the resources (copying and pasting whenever I update the dependency), which partly defeats the idea of being extendable.

Would it be possible taking validation and parsing away from the resources?. That way a parser and a validator could use the interfaces to parse and validate resources, which could be reduced to only implement access logic, and could be easily extended to add, for instance, the setters I need without affecting parsing and validation which I suppose is what you want to protect. I could try to help implementing it.

If not in scope, would you consider making final just the methods and not the full classes?

@Art4
Copy link
Owner Author

Art4 commented Oct 22, 2015

Thanks for your comment. I'm thinking about a solution, but I'm not sure I fully understand your usecase. I'd be glad if you could provide a code example.

Would it help you, if every resource implements a setter interface, so you can composite a resource and set your own data into it? So your own resources would look like this:

namespace My\Own;
use \Art4\JsonApiClient\Document;
use \Art4\JsonApiClient\DocumentInterface;
class Document implements DocumentInterface
{
    public function __construct($data, $manager)
    {
        $this->document = new Document($data, $manager);
        // set more data to the document
        $this->document->set('foo', $bar);
    }

    // Implement DocumentInterface::get()
    public function get($key)
    {
        return $this->document->get($key);
    }
    // Implement getKeys(), asArray() and has() the same way.
    // This methods could also move to a Trait.
}

@Art4 Art4 deleted the DataContainer branch October 22, 2015 10:11
@fcastilloes
Copy link

I've just started a project to extend your library, so I can give you a use case. I want to be able to do something like this:

$jsonapi = '{"meta":{"info":"test"}}';

$factory = new \Art4\JsonApiClient\Utils\Factory([
    'Document' => 'FCastillo\JsonApiBuilder\Document',
]);

$manager = new Manager($factory);
$document = $manager->parse($jsonapi);

$item = new \FCastillo\JsonApiBuilder\Builder\ItemBuilder('test', 'user');
$item->setAttribute('email', 'test@example.com');
$item->setAttribute('foo', 'bar');

$document->setDataItem($item);
echo $document->asString();

To output this:

{
  "meta": {
    "info": "test"
  },
  "data": {
    "type": "user",
    "id": "test",
    "attributes": {
      "email": "test@example.com",
      "foo": "bar"
    }
  }
}

This is already done, and it works. My problem is that I want to add methods like setDataItem() to Document, so I have to add a new class to the factory. I'm OK with that, but as Document is final and it has the validation and parsing implementation, I have to copy and paste it in my Document class. You can see it in my implementation of Document.

You can see that it contains the logic in the original construct and parseData methods because I can't extend the class, so if you update Document to adapt to a new version of the document, I have to update my class as well, along with any other one I need to replace.

My preferred solution would be to keep these final classes as small as possible and to avoid logic related to the spec in them, so I would suggest a strategy similar to the factory already in place and add a parser and a validator.

This way the constructor could just set the manager and the container and call the factory, which would first call the validator and if valid, then call the parser and populate the container with the objects created in the factory. This way I could totally replace the Document class or any other one, and as long as I implement the interface, I get parsing and validation just calling the factory.

I hope this explains better my use case, let me know if there's still something missing.

By the way, I also had to copy the DataContainer to just add a remove() method. It feels like overkill to me. In this case I would really appreciate if the class has final methods instead of be final itself.

Thanks a lot.

@Art4
Copy link
Owner Author

Art4 commented Oct 22, 2015

Thank you for providing your code. Looking through your project I would say that your needs are out of scope of my JsonApiClient. This project a handles/validates an incoming json api body. It is not build to create a json api body.

What you need is a mix of a client and server. You can find several implementations on jsonapi.org. Personally I prefer the Fractal library. It isn't listed there, but I like the transformer concept.

I forked your project and implements your use case without even replace a class from JsonApiClient. See the example.php: https://github.com/Art4/json-api-builder/blob/api-builder/example.php

$ php example.php
It works!
Result:   {"data":{"type":"user","id":"test","attributes":{"email":"test@example.com","foo":"bar"}},"meta":{"info":"test"}}
Expected: {"data":{"type":"user","id":"test","attributes":{"email":"test@example.com","foo":"bar"}},"meta":{"info":"test"}}

@fcastilloes
Copy link

Thanks a lot, I tried the implementations on jsonapi.org some time ago and I didn't find anything I was comfortable with, but Fractal feels like a good fit.

I'll try the combination then. Again, thank you very much for your time and support.

@Art4
Copy link
Owner Author

Art4 commented Oct 22, 2015

You're welcome. Feel free to open an issue or mention me in your project, if you have more questions.
Good luck to your project. 👍

@Art4 Art4 mentioned this pull request Jan 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants