Open dakujem opened 5 years ago
Thanks, I will look at it more during week. Some questions:
Entity
Why you need useMapper
? Override of getReflection()
isn't enough? I have custom getReflection
& EntityReflection
on some projects and I didn't find a issue.
EntityReflection, Property, PropertyFilters,...
Can you provide a example of your entities please? What features are you using in your entities?
How you extend PropertyFilters
, PropertyPasses
and others? Do you add new properties & methods to classes or you only assign values into existing properties?
This is very interesting to me. I have EntityReflection
classes with injected properties, custom getters/setters, custom $internalGetters
,... but I have never need copy & paste code from EntityReflection
(maybe getFamilyLine()
) or modify Property
class.
But I agree some reflection classes can be more "user-friendly" (for example would be nice move $definition
parsing from constructors and keep only assigning of properties).
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. I will answer some I know the answer right away though.
Why you need useMapper? Override of getReflection() isn't enough? I have custom getReflection & EntityReflection on some projects and I didn't find a issue.
Because we're calling getReflection with some added arguments. We need to add those to the call in useMapper
.
Result
Imagine $entity->foo
holds the value of "bar"
, freshly loaded from the database.
// 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 $book->author = 5
it got flagged as modified, while, again, in sense of data ,there was no change at all.
Events
For example when one persists an entity with no changes, we have a special event for the occasion. We also have a special event fired once the cascaded persistance we implemented has finished.
Sidenote:\ Some of our extensions are simply just the most straight forward way to doing what we needed, in the light of the "dead" LM, 3 years ago. They may not be optimal. I can look at those cases specifically to find out if we could change our implementation...
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.
Sure, private props are good in early stages of development. But LM is on version 3, and its code has come to a stable state. There has been no rapid development for years.
Sorry for delay, I was very busy.
The entity gets flagged as changed when the underlying data has not changed at all.
I think other behavior can be achieved without big changes in Result
. See PR #140.
Events
Ok, we can change it to protected
. Personally I prefer separate events dispatcher for whole application but I agree it can be useful in some cases.
What good use is there in private props (or methods) in libraries anyway?
Private props and methods mark internal implementation. Everything else is public interface (because protected
visibility is kind of public
visibility) and public interface has a lot of disadvantages. So if we change visibility of all props and methods to protected
, every future change will be BC break.
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 Result::extractIds()
. This change is safe, because method is marked as private
and I can release it in minor version. But if it will be protected
I can change nothing, because it will be massive BC break.
So protected
visibility is last option if there isn't better solution.
Well, the PR #140 helps a bit, but the Result::$modified
property is private, so it's not accessible from a descendant anyway, so when one wants to extend it, he needs to copy most of the class' code.
protected visibility leads users to "hacks"
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. It comes at the expense of exposing private props.
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, $internalGetters
,...
Hello @janpecha Jan,
while i don't have enough time to go over it thoroughly, i definitely like the approach! It is a good and brave direction, the changes are heading.
I have 2 remarks that I will post to #141 directly.
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).