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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Extensibility improvements (PR) #139
base: develop
Are you sure you want to change the base?
Conversation
Visibility changes in: Entity, EntityReflection, Events, Result, PropertyFactory, Property, and some Property-related classes
other property-related classes can be defined as well
Thanks, I will look at it more during week. Some questions: Entity Why you need EntityReflection, Property, PropertyFilters,...
This is very interesting to me. I have But I agree some reflection classes can be more "user-friendly" (for example would be nice move Result It's interesting. What behavior you need for flagging of changed entries? Events What kind of custom events you need? |
Sorry, i don't have time to answer to all your questions right now, but I will come back to you later.
Because we're calling getReflection with some added arguments. We need to add those to the call in
Imagine // when you assign the same value
$entity->foo = "bar"; The entity gets flagged as changed when the underlying data has not changed at all. We have extensions where one can assign an ID to the relationship and it works as intended. To do that, we also needed to alter the mentioned "flagging", because if a Book was related to Author ID 5 and one called
For example when one persists an entity with no changes, we have a special event for the occasion. Sidenote: But it all boils down to a simple question: Why not allow better extensibility, when there is a simple way? It does not affect the library in a bad way. If you think it does, let us discuss the reasons. |
What good use is there in private props (or methods) in libraries anyway? In case one has an implementation that nicely sits behind an interface and all its uses can be swapped for custom implementations of the interface, then it is fine to use private props. Otherwise the only effect private props and methods have is the annoyance of developers when they find out they are unable to achieve the behaviour they need. It closes the doors to being used by broader audience.
|
Sorry for delay, I was very busy.
I think other behavior can be achieved without big changes in
Ok, we can change it to
Private props and methods mark internal implementation. Everything else is public interface (because In addition, protected visibility leads users to "hacks" and to broken apps after every minor update. Real example: I'm currently working on a PR and I need change return value of So |
Well, the PR #140 helps a bit, but the
This is only an indication that users are missing something and trying to implement the missing something by "hacking". If they were enabled to achieve their needs using propper approach, for example injection and interfaces, then there would not be such a need to "hack" anything. This proposal is about enabling better extensibility based on the current library state with minimal effort. Also, I do not believe that protected props are part of public API. While I agree that changing protected props should be done wisely and with considerations, it is not something that should be marked as a BC break. It should be released as a minor change, not a patch. So yes, it restricts changes a bit, but not that much, provided the changes are clearly mentioned. |
EntityReflection - what do you think about #141 ? It allows to create own provider and change everything - properties, getters, setters, |
PR related to issue #133
Method and props visibility changes
Below is a list classes, respective changes, the result of the changes and why they are needed.
Entity
$currentReflection
anduseMapper
allows for a different implementation ofgetReflection
method, with added parametersEntityReflection
EntityReflection
without the need of completely copying its code$internalGetters
, useful for project-wide extensionsProperty
,PropertyFactory
, (alsoPropertyFilters
,PropertyMethods
,PropertyPasses
,PropertyType
,PropertyValuesEnum
)EntityReflection
appliesProperty
class extension without the need to copy masses of codeResult
Result::setDataEntry
we needed to copy 1000 lines of code, while now the same result should be achievable by reimplementing the single method only.Events
Other extensibility improvements
PropertyFactory
PropertyFactory
received a change allowing developers to define a custom class instead ofProperty
that the property factory creates. This is necessary because the property factory is tightly coupled toEntity
and can not easily be changed. Even if it could be changed, thePropertyFactory::createFromAnnotation
method is huge and it would not be optimal to duplicate the code in order to just provide an alternative implementation to the originalProperty
class. The reason behind this is that the annotation parsing and entity properties are key feature of LM and allowing users to further improve their capabilities is essential.This is the most straight-forward solution that has negligible impact on performance (considering the annotations are parsed once per each entity class).