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

Extensibility improvements #133

Open dakujem opened 5 years ago

dakujem commented 5 years ago

Hello, let me first thank you all for creating and maintaining this wonderful tool.

The problem

We used LeanMapper in several projects some 2 years back and hit a concrete wall when we wanted to extend it. We really thought the project was dead back then. We had no other choice but to create a complete copy (fork) of LM just to change the visibility of a couple of private props and methods to protected. We needed to use LM in a way that was not standard and deviated from it's original purpose. Never the less, extensibility should be one of the primary concerns of any good library.

Possible solution

I can propose props and methods that should be shifted from the darkness of the private possession into the generous protected sphere. This would be the beginning.

private -> protected

Things to consider

I know that with adding more stuff to the protected or public visibility more concerns about the future/backward compatibility arise. But the library is quite stable already and until now it was not really possible to extend it anyway :wink:

Possible benefits

I've already mentioned the extensibility is the primary benefit this change would bring. There could be more maintainers / contributors. We have heavily extended the library and I am willing to share our ideas and solutions and help this library progress.

Let us discuss this. Then I would prepare a PR.

By "we" I mean myself and my colleagues from Via Aurea

janpecha commented 5 years ago

I agree - in some cases (if it will be usefull) we can change visibility from private to protected. For example in EntityReflection (alternatively, I have been thinking about some IEntityReflectionFactory).

We had no other choice but to create a complete copy (fork) of LM

Is it on Github or you have private fork?

dakujem commented 5 years ago

It is a private fork. But I believe I could persuade the owner to transfer it to GitHub. But it really mostly consists of private to protected visibility swaps. Currently, I have to manually compare the differences between the github master repo and our fork, which is tedious and I really don't want to do it again. I would prefer to make these changes directly to the master library of LM, because I believe they are harmless to the development of LM, and can bring the extensibility I described above for users willing to extend it even more, be it privately or publicly.

We have an extended library that builds upon this private fork, from which some parts and ideas could even be beneficial to the original LM and could be backported. This can obviously only happen once the classes are extensible.

I have to check for the full extent, but Entity, EntityReflection, Events and Result are the main culprits. If you are interested, I can do it and prepare a PR. Let me know.

mibk commented 5 years ago

@dakujem Can I ask what kind of extensions have you made and why they were needed? Just a brief example is enough. Thank you.

dakujem commented 5 years ago

Well, for example:

I don't want to explain why the need arised. It just did. And it may for others. We are using LM for bigger systems and those changes were necessary for the workflow we have.

mibk commented 5 years ago

@dakujem OK, thank you.

janpecha commented 5 years ago

Change private to protected is only one from ways how to achieve extensibility. Would be fine if you create test for every your use case and we can try to find optimal solution. Currently we doesn't know what you exactly need and why.

dakujem commented 5 years ago

True, but changing the visibility is the most straight forward and least painful path for the current state LM is in. It would otherwise require much more reworking.

All we needed in fact, was the ability to access some of the private properties, to be able to modify them and call the private methods not to duplicate the code.

This issue is a proposal for cooperation. If you are open to these kind of changes, that will not affect the library itself in any way I can think of, I will create a PR so that you can see the full extent of what I suggest. I just don't want to waste my time doing that if you resolutely refuse right now.

Let me know.

janpecha commented 5 years ago

I agree - I welcome every change that moves LM forward. PR would be great. Then we can discuss and try to find optimal sollution - in some cases it can be change of visibility, in some cases something else.