pomm-project / ModelManager

Model Manager for the Pomm database framework.
MIT License
66 stars 27 forks source link

Custom FlexibleEntity classes #6

Closed tlode closed 9 years ago

tlode commented 9 years ago

Since issue #5 I've looked into FlexibleEntity and found that some of the get-methods and one has-methods do not return identical results. You can reproduce this by adding the following 2 tests in Unit/Model/FlexibleEntity.php. These tests reflect the current state and not the expected one!

    public function testGetters()
    {
        $entity = new PikaEntity(['pika' => 'whatever']);
        $this
            ->string($entity->get('pika'))
            ->isEqualTo('whatever')     // shouldn't this be 'WHATEVER'?
            ->string($entity->pika)
            ->isEqualTo('WHATEVER')
            ->string($entity['pika'])
            ->isEqualTo('WHATEVER')
            ->string($entity->getPika())
            ->isEqualTo('WHATEVER')
            ->array($entity->fields())
            ->isIdenticalTo(['pika' => 'whatever'])
            ->array($entity->extract())
            ->isIdenticalTo(['pika' => 'whatever', 'pika_hash' => md5('whatever')]); // shouldn't this be 'WHATEVER'?
    }

    public function testHasMethods()
    {
        $entity = new PikaEntity(['pika' => 'whatever']);
        $this
            ->boolean($entity->has('pika'))
            ->isTrue()
            ->boolean($entity->hasPika())
            ->isTrue()
            ->boolean(isset($entity->pika))
            ->isFalse() // should also be true!
            ->boolean(isset($entity['pika']))
            ->isTrue()
            ;
    }

For the get-methods I am not quite clear how results are expected. Should get('pika') return 'whatever' or the result of getPika()? Should fields() return the original value or the modified? how about extract()?

In my opinion all getters should return the same result, but there also should be some kind of method like fields() which returns the original values. extract() should return the modified values and could have an optional argument to get original values.

chanmix51 commented 9 years ago

In fact, get('pika') returns the internal raw value. getPika() triggers the asked method if it exists or is trapped by __call which calls get('pika') otherwise. This is important since it must exist a way to access entity's internal raw values, by exemple to build conditions for database queries whereas overloaded accessors are mostly used to format and present data.

The test using the extract() method outputs a lowercase whatever because there are no methods hasPika(). This is why the raw value is output instead of using the overload getPika().

The third test case is testing PHP's isset() method which returns false because there are no such real attribute in the FlexibleEntity instance.

tlode commented 9 years ago

Thanks for taking the time (again).

I am not happy with this design, but if this is the expected behavior for common use cases, I would like to propose and discuss another approach which would make FlexibleEntities even more flexible as they yet already are.

As you mention somewhere in the documentation FlexibleEntity is one of the keystones of pomm. In my opinion it should primarily work in a very basic and intuitive way, so that it does not need much of documentation. Its interface should be clean and simple, but it also should give users more flexibilities to extend it for their own use cases.

In your opinion would it be possible to introduce a FlexibleEntityInterface and a very simple base class (for example FlexibleEntityBase) which would just implement all the access methods and methods which are necessary to suite the model manager (hydrate, getRawValue & co) and the public interface? Then in CLI there could be a configuration option making it possible to define your own FlexibleEntityBase sub class, which will be used to generate the code. The default of course would be the yet existing FlexibleEntity which would extend from FlexibleEntityBase and implement all the behavior it currently have on top of the basic methods.

This is yet of course just a rough idea, but I am very curious if you and others would like that idea and want to go on thinking about it. I would also like to contribute to a first version, since I am the one who would love to see it :-)

I am sorry this issue now has changed topic. Feel free to close or move it somewhere else if you like.

chanmix51 commented 9 years ago

That's an excellent idea ! :+1:

chanmix51 commented 9 years ago

I did browse the code that needs FlexibleEntity instances, most of the needs are:

There may be other needs (like object instanciation in Model::createEntity()) but since this method is overlodable this should be acceptable. Maybe moving this method in a trait would be a way to make people able to use their own entities in their Model classes with this method rewritten in a custom trait.

There is work mostly located in:

As soon as I get time to get into it, I will create a FlexibleEntity branch with the WIP.

chanmix51 commented 9 years ago

See flexible_entity branch. Would that suit the job ?

tlode commented 9 years ago

Thanks for for being already involved so much :-)

I like that the interface is simple. But I found in some places (mostly WriteQueries) that the interface is still been accessed through get() and has(). These methods would also have to be declared in the interface. Or should I replace them with the new fields('x') signature?

Would it be more convenient if there is a FlexibleEntityBase class which implements the basic methods? FlexibleEntity or other custom entities would then just extend and add its behavior on top of these basic methods. The benefit would be writing clean and easy custom entities. Of course the dark side of this would be the extra layer in between...

If you are ok with a FlexibeEntityBase class I would start splitting up the code between FlexibleEntity and the base class?

chanmix51 commented 9 years ago

imho, let's start with no base class and experiment what we do always need. This way the base class will be built upon real needs and not theory.

yes, get() and set() must be replaced by fields() and hydrate().

chanmix51 commented 9 years ago

I have updated the WriteQueries trait in 24c08ff.

We can already feel that the base class will hande the status.

tlode commented 9 years ago

I've done exactly the same in WriteQueries on my side :-) Is there any way we can better synchronize? :-)

chanmix51 commented 9 years ago

Try to use the ModelManager with your own entities, spot the problems and I'll fix them :-)

chanmix51 commented 9 years ago

I have pushed a StatefullEntity abstract class so no need to care about entity state.

tlode commented 9 years ago

Thank you for your work! I've made a CustomFlexibleEntity which would be as simple as I need it.

chanmix51 commented 9 years ago

Ok, from what I read, we can provide a FlexibleContainerTrait that will handle the fields attribute and implement ArrayAccess and IteratorAggregate.

tlode commented 9 years ago

yes, since CustomFlexibleEntity is only a slightly reduced FlexibleEntity version.

Alternatively it could be sufficient to just implement the final get, has, set, clear, add methods plus the IteratorAggregate in the FlexibleContainerTrait. FlexibleEntity or some custom version can implement the ArrayAccess and magic methods as they prefer...?

chanmix51 commented 9 years ago

Done in 44b4fa4