WIP: Extensibility improvements (PR)

[dakujem]

:bulb:
This is a "work in progress" PR to show the extent of changes. It is a bare minimum of changes that I propose and that enables extensions noted in #133. I will need to try our extensions with these changes to see if the changes are sufficient. Please, do not merge yet.

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.

I suggest generally refrainig from using private modifier unless explicitly allowing for extensions in a different way.

---

Entity

• access to $currentReflection and useMapper allows for a different implementation of getReflection method, with added parameters

EntityReflection

• access to props and methods allows for extension of EntityReflection without the need of completely copying its code
• allows for custom entity-props parsing, caching or entity-props injection
• allows for easy definition of custom $internalGetters, useful for project-wide extensions

Note:

The main problem here is that EntityReflection class is tightly coupled with the Entity class.
In cases where tightly coupled classes serve the purpose of encapsulating logical parts in separate classes it makes sense to define its props and methods as private.
However, in LM the design is not flawless, because Entity::getReflection may be reimplemented to return a different implementation of EntityReflection, while other parts of LM rely on the fact that both Entity::getCurrentReflection and Entity::getReflection return an instance of EntityReflection class.
The above effectively renders EntityReflection class useless when inheriting because all it's props are private and extending it requires a complete copy of its code anyway.

The best solution here would of course be an interface and ability to inject a reflection factory instead of the tight coupling.

There is simply no excuse for using private props and methods in this class.

Property, PropertyFactory, (also PropertyFilters, PropertyMethods, PropertyPasses, PropertyType, PropertyValuesEnum)

• the same issue as with EntityReflection applies
• allow for Property class extension without the need to copy masses of code

Result

• in order to (slightly) modify the policy of flagging entries "changed" in 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

• allow for custom events, again, without a complete copy

Other extensibility improvements

PropertyFactory PropertyFactory received a change allowing developers to define a custom class instead of Property that the property factory creates. This is necessary because the property factory is tightly coupled to Entity and can not easily be changed. Even if it could be changed, the PropertyFactory::createFromAnnotation method is huge and it would not be optimal to duplicate the code in order to just provide an alternative implementation to the original Property 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).

Note that this new feature looks much better in PHP 5.6 syntax with ::class constructs and proper variadic functions.

[janpecha]

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?

[dakujem]

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.

[dakujem]

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.

[janpecha]

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.

[dakujem]

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.

[janpecha]

EntityReflection - what do you think about #141 ? It allows to create own provider and change everything - properties, getters, setters, $internalGetters,...

[dakujem]

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.