[WIP] Refactor rdf type factory to accept a list of mappers per type name

[adou600]

This is needed by symfony-cmf/cmf-sandbox#143 and symfony-cmf/create-bundle#24 to allow to use a custom RdfTypeFactory and DoctrinePhpcrOdmMapper for the route documents. Before this, a new class document (CmfRoute) was needed just to get and set locale values.

I will do some cleaning and ask for the merge then...

CC @dbu

[lsmith77]

i guess this CS issue was there already .. but the opening parenthesis should be on the previous line

[flack]

@adou600: I tried reading the linked issues, but I can't really make heads or tails of it. I get that you need different mechanisms for loading different types of content, but I wonder if it wouldn't be cleaner to do so in the mapper implementation. I.e. something like

    public function prepareObject(TypeInterface $type, $parent = null)
    {
        if ($type->getRdfType() == 'foo')
        {
            return $this->prepareFoo();
        }
        else
        {
            return $this->prepareBar();
        }
    }

I guess this doesn't work out of the box because the mapper interface doesn't get all the information you need to make the decision, but I'm curious as to what exactly is missing. If it can be easily added, I would rather modify the mapper interface than the RdfTypeFactory, because from the implementor's point of view, it is kind of a black box, and it would be better if all the logic relevant for the application would be accessible through one point (i.e. the main mapper implementation). Or is it somehow necessary that site builders specify different mappers per type in their application configuration?

But maybe I'm missing the point entirely. Could you post some sample code where I can see in what context this is required?

[dbu]

we could inded solve our current issue by extending the mapper. but this soon becomes clumsy. once we have lots of content with some needing specific mapping situations, this custom mapper needs to know about all of them. with reusable components, most of them optional, this will not work. imagine i have a mixed setup. some entity is an array, another is doctrine orm and yet another doctrine phpcr-odm. now i would need to write my custom "chainmapper" or something that knows my entities and forwards to the right mapper for each type.

i really think allowing to specify the mapper in the metadata is the right approach to handle the situation generically.

[flack]

I think a ChainMapper which actually serves as a service container of sorts for the actual mapper implementations doesn't sound too bad, and could be part of the main repo, since I don't think it would have to have any implementation-specific details, but that may only be a matter of personal preference.

There's two things I still don't understand about this PR:

First, you mention that the mapper to be used should be specified in the metadata. But for this, the drivers and the configuration format would need to be expanded, no? Because right now, you would have to collect the mapper instances in an array and pass them to the factory in your own integration code, or am I missing something?

Second, there is one place where the default mapper is used unconditionally: https://github.com/flack/createphp/pull/48/files#L0R55

Wouldn't that mean that the default mapper implementation would somehow have to know about all the different types of content that may exist? E.g. if the default mapper inherits the canonicalName method from AbstractRdfMapper, the doctrine-specific logic would not be run, and if it were the other way around, I guess that the doctrine mapper would throw some kind of error when it's called with a class it doesn't know about (but that last part is speculation)

[dbu]

i think both approaches have their plus and minus, but we can go with the chain mapper. but if we do, i think we should drop the getMapper method from TypeInterface. it suggests that you could have a mapper per type, whereas if we follow the chain mapper approach there is only one global mapper.

[dbu]

well, global = per rest service and type factory - its still injected so people could build 2 completely independant instances of the createphp library in the same application if they really need to.

[flack]

Well, if we don't need the method, we can drop it. But I don't really see the difference between the factory implementation and the chainmapper implementation in this regard: I mean, they will both allow you to register a default mapper and mappers per type, so semantically, nothing really changes, it's just that the code is in a different place. At least, that's how I imagine it, e.g.:

class ChainMapper implements RdfMapperInterface
{
    public function __construct(RdfMapperInterface $mapper)
    {
           $this->_defaultMapper = $mapper;
    }

    public function registerMapper(RdfMapperInterface $mapper, $typename = null)
    {
        if (is_null($typename))
        {
             $this->_defaultMapper = $mapper;
        }
        else
        {
              $this->_mappers[$typename] = $mapper;
        }
    }

    public function getPropertyValue($object, PropertyInterface $property)
    {
        $mapper = $this->_determine_mapper_to_use($object);
        return $mapper->getPropertyValue($object, $property)
    }
    // and so on....
}

Or something similar. To implement your own mapper selection logic, you would then only have to extend the chainmapper and override the determine_mapper_to_use method. Of course, you could do the same with RdfTypeFactory (if ArrayLoader is changed to remove the hardcoded instantiation), but then, there are two places you have to fiddle with during integration.

I mean, the more I think about it, the more similar the two approaches seem to me. In the end, I think both will do the job just fine, so I'm not fixed on any particular way to do it. I would just like to keep it simple for integrators, i.e. they should have to know as little as possible about how createphp works to be able to use it.

[dbu]

i think the chain mapper is better in the sense that we do not need the rest of the application to constantly ask for the mapper and its obvious how the objectToName should be handled. that really is not solved with the factory deciding.

i think the mapper is really the place to also have logic to determine dynamically what mapper to use, i.e. between doctrine orm, odm and possible other backends.

the only real difference i see is that with the factory approach, the createphp metadata would need to define the mapper, whereas with the chain mapper, this is some sort of configuration injected into the chain mapper. i prefer the later, really.

[adou600]

I discussed with @dbu about this chain mapper option. It seems better, for me mainly because it covers all the cases and we don't have to use the default mapper in some methods...

I will start to work on this in a new PR, in March.

[wouterj]

ping?

[dbu]

adou did an internship at liip when he did this PR, but he did not get around to finish this PR. after that, he told me that he found no more time to work on it in his spare time. and i did also not find the time to wrap this up.

[flack]

superseded by #58