Tharos / LeanMapper

Lean Mapper is a tiny ORM based on powerful Dibi database abstraction library for PHP.
MIT License
87 stars 35 forks source link

EntityReflection provider #141

Closed janpecha closed 5 years ago

dakujem commented 5 years ago

I like these changes :+1:

Allow me to make one request:

  1. Please allow the PropertyFactory to use a custom set of filters, passes, methods etc.
    • either use an interface+injection approach instead of the static method calls
    • or at least allow for static extension of PropertyFilters (i.e. "mixin"), allowing to swap the particular implementation

Sidenote:

Even though I would most probably approach the "EntityReflection issue" differenly, using a factory for the whole EntityReflection instance, your implementation is equally as good and serves the purpose of extensibility.

// what I would do
static::$reflections[$class][$mapperClass] =
    static::getReflectionFactory()->create($class, $mapperClass)
;

I'm also not sure if calling the interface implementor a "provider" is a good idea, as it does not provide a thing, rather it providers for another class, but maybe I'm just picky. :-)

All in all, good job!

janpecha commented 5 years ago

Please allow the PropertyFactory to use a custom set of filters, passes, methods etc.

If I understand correctly, it isn't needed - just call new Property(...) and set filters, methods,... via constructor.


using a factory for the whole EntityReflection instance

Yeah, factory was first idea but it isn't so easy. It requires rewrite architecture of EntityReflection completely. This PR is "compromise solution".


I'm also not sure if calling the interface implementor a "provider" is a good idea

IMHO it's ok, because it provides informations for reflection. Alternative we can rename interface to EntityReflectionFactory and rename methods to createProperties(), createGetters() & createSetters(). Maybe it will be better.

dakujem commented 5 years ago

But it is not possible to change the PropertyFactory ... or is it? Is there a way to customize the filters e.t.c.? I can't see it just by looking at the code.

The "provider" name is better then. You are right, it provides for something else.

janpecha commented 5 years ago

Ok, so you want:

1) create Property from annotation via PropertyFactory 2) overwrite filters,... with custom values

I will think about it.

janpecha commented 5 years ago

@dakujem I added new parameter callable $factory into PropertyFactory::createFromAnnotation() (https://github.com/Tharos/LeanMapper/pull/141/commits/2902d9e235633a46cd3a8850be1f3aae3365594b). Is it ok for your needs?