Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EntityReflection provider #141

Merged
merged 4 commits into from Feb 10, 2019

Conversation

Projects
None yet
2 participants
@janpecha
Copy link
Collaborator

commented Oct 7, 2018

No description provided.

@janpecha janpecha self-assigned this Oct 16, 2018

@janpecha janpecha referenced this pull request Oct 22, 2018

Open

IEntityReflectionFactory #2

@janpecha janpecha changed the title WIP: EntityReflection factory WIP: EntityReflection provider Nov 16, 2018

@janpecha janpecha changed the title WIP: EntityReflection provider EntityReflection provider Nov 16, 2018

@janpecha janpecha added the RFC label Nov 16, 2018

@janpecha janpecha added this to the Version 3.4.0 milestone Nov 18, 2018

@dakujem

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

I like these changes 👍

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

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 4, 2018

Ok, so you want:

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

I will think about it.

@janpecha

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 24, 2018

@dakujem I added new parameter callable $factory into PropertyFactory::createFromAnnotation() (2902d9e). Is it ok for your needs?

@janpecha janpecha force-pushed the inlm:pr/entity-reflection-factory branch from 496d615 to f1b6528 Feb 10, 2019

@janpecha janpecha removed the RFC label Feb 10, 2019

@janpecha janpecha merged commit f1b6528 into Tharos:develop Feb 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@janpecha janpecha deleted the inlm:pr/entity-reflection-factory branch Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.