Make ResourceBuilder and RichTextParser extendable

[arondeparon]

Goal

Allows users to implement their own resource builders and rich text parsers on top of existing functionality.

Blockers

The delivery client currently has a public getResourceBuilder() method that sits unused. By default, it returns the private $builder property.
All the internal code does not use getResourceBuilder(), but the $builder property directly, which makes extending only possible when extending everything.

Implemented solution

Stop using $this->builder directly, and use getResourceBuilder().
Direct property access is now replaced by public methods. This is a backwards compatible change, since these methods were previously unused.

[arondeparon]

This issue should resolve #7

[dborsatto]

For what it's worth (which isn't much, but still) I still believe that making the delivery client extendable is still the wrong approach. What you're trying to achieve is to have the resource builder and rich text parser do something custom, but you're trying to get there by extending the client (and therefore, needing to extend the resource builder and rich text parser).

The correct approach in my opinion would be to update the resource builder to somehow be able to define custom mappers (most likely by updating Contentful\Core\ResourceBuilder\BaseResourceBuilder in the contentful/core library). The rich text parser already supports the definition of custom node mappers by calling Contentful\RichText\Parser::setNodeMapper(string $nodeType, NodeMapperInterface $nodeMapper), so there should not even be the need to update that part. The resource builder should be changed to provide some similar method (something like $resourceBuilder->registerMapper(string $resourceType, MapperInterface $mapper)), so instead of having to extend multiple things and hook into processes that are not really meant to be exposed, you rely on a safe and tested public API of the classes you want to work with.

[arondeparon]

For what it's worth (which isn't much, but still) I still believe that making the delivery client extendable is still the wrong approach. What you're trying to achieve is to have the resource builder and rich text parser do something custom, but you're trying to get there by extending the client (and therefore, needing to extend the resource builder and rich text parser).

The correct approach in my opinion would be to update the resource builder to somehow be able to define custom mappers (most likely by updating Contentful\Core\ResourceBuilder\BaseResourceBuilder in the contentful/core library). The rich text parser already supports the definition of custom node mappers by calling Contentful\RichText\Parser::setNodeMapper(string $nodeType, NodeMapperInterface $nodeMapper), so there should not even be the need to update that part. The resource builder should be changed to provide some similar method (something like $resourceBuilder->registerMapper(string $resourceType, MapperInterface $mapper)), so instead of having to extend multiple things and hook into processes that are not really meant to be exposed, you rely on a safe and tested public API of the classes you want to work with.

I see your point, and feel the same way. The proposed solution in the PR feels like a "quick fix" that can be integrated relatively quick without negatively impacting existing implementations. I felt this was a better approach, since this issue has been open for a pretty long while and I did not want to create a massive overhaul in the library.

Still though, a future PR could implement on your proposed solution.

[dborsatto]

Hold on, I think the base resource builder already supports this. Let me check.

[dborsatto]

Yeah, there's already a working solution for this (which I had implemented but forgot about it). It is documented for the CMA SDK, but it's the same code underneath so it works for this too.

$builder = $client->getResourceBuilder();

$builder->setDataMapperMatcher('Entry', function (array $data) {
    switch ($data['sys']['contentType']['sys']['id']) {
        case 'blogPost':
            return \My\Custom\Mapper\BlogPostMapper::class;
        case 'author':
            return \My\Custom\Mapper\AuthorMapper::class;
    }
});

Basically the method setDataMapperMatcher accepts 2 parameters: a resource name (Entry or Asset for the CDA), and a callback which will get the raw array, so the user can implement any check (most likely on the $data['sys']['contentType']['sys']['id'] property) and return a FQCN of a mapper class that will handle the conversion.

Basically, this PR is unnecessary as there already is a supported way of doing this operation 🙂 the problem is the lack of documentation, not the lack of the feature. I remember the 2016 issue and I remember that during the CDA SDK refactoring I did in early 2018 (which I had mentioned in the original issue) I made sure to fix some of the outstanding problems, including the lack of options for extending rhe resource builder.

[Sebb767]

Hi @dborsatto , thank you for chiming in again! Your contribution is very much appreciated. I'll try to get your suggested method documented, which is necessary either way.

@arondeparon does this work for you?

[arondeparon]

Thanks. I have looked into this and it seems that although this should make it possible to create custom mappers.

For my personal use case, this still seems unusable though. Instead of creating a mapper for each content type, I simply define value models in my application that extend a certain base class. I then want a generic mapper to map the Contentful fields to the public properties of my value object. I have already gotten this to work using a custom query builder that pipes everything through my formatter, but I don't think I can get this to work with with this mapper pattern.

I could get this to work by defining some kind of map that tells the mapper which content types refer to a given class name, but I simply want it to work out of the box.

I think that this could work by extending the Builder object itself, but at this point I am doubting whether that is worth it, because I have already gotten it to work in another way.

[dborsatto]

A mapper is simply an object with a method that turns an unserialized response from the API into *anything* you want, you are 100% free to implement any logic you want, in both the mapper (take a look at those that the SDK ships with for a general idea) and in the matcher callable that you feed to the resource builder. There's no need to filter by content type, that was just a common use case. You can do pretty much anything you want with this, I fail to understand how this mechanism is any different from your needs and how extending the client/resource builder would solve anything that this doesn't...

[arondeparon]

A mapper is simply an object with a method that turns an unserialized response from the API into anything you want, you are 100% free to implement any logic you want, in both the mapper (take a look at those that the SDK ships with for a general idea) and in the matcher callable that you feed to the resource builder. There's no need to filter by content type, that was just a common use case. You can do pretty much anything you want with this, I fail to understand how this mechanism is any different from your needs and how extending the client/resource builder would solve anything that this doesn't... Il mer 24 feb 2021, 17:03 Aron Rotteveel notifications@github.com ha scritto:
…
Thanks. I have looked into this and it seems that although this should make it possible to create custom mappers. For my personal use case, this still seems unusable though. Instead of creating a mapper for each content type, I simply define value models in my application that extend a certain base class. I then want a generic mapper to map the Contentful fields to the public properties of my value object. I have already gotten this to work using a custom query builder that pipes everything through my formatter, but I don't think I can get this to work with with this mapper pattern. I could get this to work by defining some kind of map that tells the mapper which content types refer to a given class name, but I simply want it to work out of the box. I think that this could work by extending the Builder object itself, but at this point I am doubting whether that is worth it, because I have already gotten it to work in another way. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#298 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAXDO54LR733Z2GWL2ZGFDTAUPNDANCNFSM4YAVAGSQ .

I understand. It's simply a matter of how I like to structure my code. The solution can definitely be implemented my creating mappers for all my content types, but I do not want an extra object for each value model. I want a generic mapper that maps Contentful data to my value models by introspecting the value model.

To clarify:

By default, the data flows somewhat like this: CF client -> CF builder -> CF mapper -> object.
In my codebase, I abstract this away by using model classes that extend upon a base class that forwards static calls the a custom builder. This custom builder connects to Contentful, returns the data, passes it through my JSON mapper and transposes the data upon the original object.

Example:

class NewsItemModel extends ContentfulModel
{
    /** @var string */
    public $title;

    /** @var string */
    public $content;
    
    /** DateTimeImmutable */
    public $date;
}

In this case ContentfulModel implements a bit of magic that allows me to use expressive, Eloquent-like syntax like this:

$items = NewsItemModel::where('fields.pinned', true)->orderBy('fields.date', true)->get()`

Which then results in a collection of NewsItemModel objects. In this case, data flows more like this:

model -> custom builder -> CF client -> CF builder -> CF (base) mapper -> custom builder -> generic mapper -> model

As you can see, this results in an extra mapping step, but since this is abstracted away, I feel that this is not a very expensive tradeoff.

The same end result is possible using the CF mappers, but like I mentioned before, these mappers are called from a different context, which makes is very hard to determine which value model a given entry should map to.

[dborsatto]

The same end result is possible using the CF mappers, but like I mentioned before, these mappers are called from a different context, which makes is very hard to determine which value model a given entry should map to.

Well yeah, but that implies that data from Contentful can take different forms depending on the context from which it is retrieved, which given that one of the benefits of using the CDA is that you get structured data, does not feel like a great solution to me. There should always be an entity object that offers a base representation of some data (regardless of where it is stored), and then you can create different value objects from this entity according to the rules of the specific subdomain you're working in. Creating these value objects straight from the source data to me feels like directly tying these objects to the raw data instead of what such data is actually supposed to represent in your application's domain.

Anyway, going back to the main topic, it seems to me that you're trying to do something that kind of goes beyond what the intended use case for the SDK is/was. From the start it was always designed to fit more in a "data mapper-like" architecture (Rouven and then I both were/are more Symfony and Doctrine people), whereas your approach seems to me "active record-like" (given you use Eloquent as example of what you want to achieve), so I believe that's where the main issue lies with your use of this library 🤔