-
Notifications
You must be signed in to change notification settings - Fork 398
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
[WIP] Introducing data-mapper and unit of work. #795
Conversation
@marcj interesting work in progress! You should probably write clear commit messages though.. One suggestion: do not call
http://martinfowler.com/eaaCatalog/repository.html More comments soon. |
Interesting work, well done! I can't wait to play with it. |
yeah, I will rebase it later and fix some typos in the pr description. The repository used here is actually exactly what you pastet although its not yer fully implemented. |
$distfinder = new Finder(); | ||
$distfinder->in($dirs)->depth(0)->files()->name($fileName . '.{php,inc,ini,properties,yaml,yml,xml,json}.dist'); | ||
$distfiles = iterator_to_array($distfinder); | ||
$distFinder = new Finder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO you should try to avoid such kind of CS changes.. the diff will be already huge and hard to review because of the changes in a lot of places all over the lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO some few CS changes shouldn't hurt in a PR that has several ten thousand line changes.
the whole change makes a lot of sense and will propel2 bring up to a new level. great job so far. |
Interesting work. A few remarks:
|
good job ! |
Also, your branch isn't testable, so there is no way to see the generated code.
|
As a user and fan of Propel, I have to agree with @fzaninotto above regarding finishing Propel 2 first and then push this new stuff in a 2.x or 3.0 release. It has been way too long since the documentation on the propel web site is all about v 2.0 but v 2.0 is nowhere to be found. |
I haven't yet fully defined the use case of repository, but it should act as middle man. Query classes are only there to fire custom queries directly at the underlying database, bypassing unitofwork and all events (if you use ->update or ->delete)
See
It uses the global configuration, as currently in Propel 2.
"right direction" defined by whom?
Indeed, they are meaningless. They weren't meant to have meaning just for me as backup commits. I've rebased it now. Btw, it's already almost a one man project.
I really know the benefits of Propel over Doctrine, that's the reason why it's almost compatible. As you see the schema definition became even easier for relations. If you use optional active-record then it behaves almost the same like at the moment, so you don't need to know anything about "em".
I won't develop on a library that uses per default a lot of anti patterns. Those decision are 3/4 years old and meanwhile there were to less contribution to finish this monster project. Just release now a old architecture, full of smelly code and old patterns, just to release something does not make sense. However, there's no point in postpone such fundamental changes just because it would push the release some weeks backwards, also because we have still a lot of bugs to fix where the contribution is at the moment to less. It appears I'm the only person that understand Propel's architecture completely till the closest corner and contributing fixes to complicated issues. If this would stop Propel 2 is dead.
Please stop complaining. It's a WIP, nobody said it is workable. |
/** | ||
* @param AbstractBuilder $builder | ||
*/ | ||
function __construct(AbstractBuilder $builder, Behavior $behavior = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public
is missing here :)
Btw, no need to review the code yet in detail. Will be changed much. Would rather talk about the whole concept and architecture, so we use the terms under the right definition. |
=> +1 Lets be realistic: noone uses Propel >= 2.* for production purpose actually. Releasing a "stable" version of code implies that someone will have to guarantee a long time support or at least an active support. And nobody want to support smelly code and old patterns. |
I really like this refactoring. A very modern approach to our problems. But I'm worried about the long delay until we can go stable. From a developer perspective this is really great, but from the perspective of a propel user this doesn't make happy. |
Let me rephrase what I wrote: "right direction" is wrong. I should have written "a direction I, as a user, would understand and support". So I don't understand this direction, but you're the leader, so do whatever you want. As a former core developer, it makes me sad. |
Sorry, I don't wanted to make you feel sad. What exactly don't you like? Maybe it's just not correctly described, because I guess we have really the same thoughts about Propel and its benefits. |
I agree @fzaninotto and @tayhimself, people I know use Propel because they don't like EM and it's really different than doctrine and I think they were waiting for a stable version 2 with all the already known features @ClementGautier : unfortunately projects are using Propel 2 and are stuck on a specific version because many BC where introduce between alpha 2 and 3. BTW using an alpha version was a known risk (and now I know, a really bad idea) so I can't complain about this modification but after many years of development I wasn't thinking it's possible to introduce so many changes. I think most of Behaviour have to be rewrite with this modification, am I wrong ? |
Just to be clear: The whole |
What makes me sad is that you're dismissing a remark for the only person who checked out your code and tried running it. What makes me sad is that you don't see the benefit of delivering vs having something clean. What makes me sad is that you took over a project, then call it "full of anti-patterns" and pivot radically without notice. What makes me sad is that current Propel users have been waiting for Propel2 for a long time, and it will never come out as it was advertised. But once again, seeing the time you spend on this project, you totally have the right to make it what you want and not what the users want. And if this is a Doctrine clone, so be it. |
I don't dismiss your response. I responded to each point you made.
We have neither of it. Builder classes are currently extremely unmaintainable (ObjectBuilder.php 6000+ lines of code, QuerBuilder 1900, TableMap 1400), which means is not clean at all and a extremely pain to fix there bugs. And we can not even deliver it because it would be immediately dead after release because we'd work on Propel 3 right after the v2 release. I'd just make no sense if we want to follow modern architectures and design patterns.
So, when one takes over a project closing eyes from flaws is the way to go? I'm sure you know also the flaws of Propel and saying it loud shouldn't make you sad. It's just the truth.
That makes me sad as well. This wasn't my fault. Some years ago it was introduced to allow POPO, which is not the case right now before this PR. It never would have been released the way it was advertised, even before this PR.
Sorry, what are you talking? Don't get me wrong, I highly respect you and your work, but I can't believe you said this. You know what all users want? They don't want better code base, faster performance, more decoupled classes with more clear separation of concerns and better testability of their models? I highly doubt that. I agree, Propel users want active record - and they can still use active record the way as before even with this PR. So I don't see the point what this PR brings users don't want to have. Maybe you can explain in more detail?
Propel 2, even after this PR, is completely different to Doctrine. Using successful and widely adopted patterns like unitOfWork under the hood doesn't mean Propel 2 will be a Doctrine clone. It's far far away from this plain wrong statement. |
@marcj if you think I could help, you just holler. Some easy and boring stuff.... Anyway, as Propel user, what I really matter is:
|
So, I was waiting for Propel 2.0 but I couldn't wait so I went with Doctrine. Maybe, for fans of Active Record, I think what @marcj said is true: in a data-mapper you have a clear separation of concerns and I think it's great to have it in propel. I also think you all are not being fair with @marcj b/c since he said that the current, and more stable, way of use propel will be kept I don't see any problem in he trying new things. Even if the old one is not going to be supported by him. If it wasn't for him, there wouldn't be even a possibility of a 2.0 release. So, be nice. That being said, this PR is something that could bring me back to Propel. +1 from me. |
What's the point? It's like proposing the DIC PSR. It makes no sense as it's very specific to a particular framework/library IMO. |
@hhamon Not really. UOW and DataMapper have more a complex interface than a |
Does it really make sens to make ORM interoperable? Are you really dealing with multiple ORM on the same project in real life? |
this disussion should not take place in this issue. please open another one or maybe even use another channel rathen then discussing on an issue tracker. |
No.
Indeed. A ORM interoperability nobody needs. |
…inheritance in new POPO style.
…to little methods. Removed old builder classes
$carRepository = $configuration->getRepository(‘\Car’); //provided usually per services/DI.
$car = $carRepository->find(23);
$brand = new Brand(‘Ford’);
$car->setBrand($brand);
$session->persist($brand);
$session->persist($car);
$session->commit();
//or
$car->save(); //cascade save as usual. why should we call |
The latest state of Propel2, written 2015-06-27: http://propelorm.org/blog/2015/06/27/propel2-current-state-and-future.html |
Well, you're obviously right :) |
From the blog post from 2015-06-27:
Any chance of a 6 month update this month instead? |
Not yet, pretty busy and wild times in my personal life. Not much time for open-source at the moment. |
Is there a way we can help moving this pr forward? Maybe we can split the remaining tasks into several smaller issues and then distribute the work :) |
Mh yeah, let me upload my current version. I'm going to create then a new branch here and create some issues to distribute the work. @cristianoc72 was working on something too iirc. |
Branch data-mapper has been created to play with it or continue easier development across the team. Test-suite is 10% green. Now we need to make some sub-tasks based on that. :) |
👍 thanks. I'll take a look over it :) |
Sure! I've ported I18nBehavior and I'm working on one-to-many relation. I'm
|
Feel free to write me in Skype if you got into trouble 8-) |
Nice to see this going forward, thanks guy :) |
@marcj when release date (odm)? I can't wait to see that. :) |
Welcome to the real future of Propel2.
This pull request is rather large and is going to introduce a heavy change in the way Propel 2 works internally and externally.
We announced in several blog posts some years ago a more strictly decoupling of domain model logic and the actual persistent logic, so we support having POPO’s as model classes. The whole architecture is now based on no static methods anymore. The only static method used is to provide active-record facility.
This implements now such a decoupling with the extraction of the logic, we had injected in the object classes directly before, into Repository and Persister class through small peaces called Components. Generated code that is related to a actual persisting method has been moved to the Builders at Platform classes which can modify all builded classes. The glue together is through the UnitOfWork class.
With this decoupling I also decoupled SQL and Propel, so in the future it’s easier to support noSQL databases and even PHPCR.
How this works
Using POPO’s means using a instance of UnitOfWork which knows everything about living objects. It’s kinda the same as in doctrine, but more compatible to Hibernate’s way of doing it.
You have a $configuration object which knows anything about the connection and known DatabasesMaps. It creates also the necessary unitOfWork (called Session) to be used in your user land code.
We have now more builder classes:
This is new. It creates the trait for each model which can be used to activate
active-record on a entity.
Old TablMapBuilder. Creates the class with all mapping information and
highly optimized methods used in persister and formatter classes.
The actual entity class builder, which is reduced to the absolut basics.
No big change to old QueryBuilder except the splitting of those extremely
big classes in several handy peaces (Components).
This is new. It implements basically the repository pattern also known
in Doctrine, which provides some basic methods to retrieve objects and handle
objects. It also contains the methods which were before in the object model itself behavior methods.
The ObjectBuilder generates the really basic entity class with injected trait of the ActiveRecordTraitBuilder if active-record is enabled. RepositoryBuilder generates a new base class
<EntityName>
Repository which should be mainly used in controllers instead of static stuff of query::create(), means we have now$repository->createQuery()
.What changed?
Schema
Through the decoupling of SQL it’s necessary to remove all workflow based on RDBMS, which means:
<table>
to<entity>
<column>
to<field>
<foreign-key>
to<relation>
foreign-key > reference
is now optional. If not defined but is necessary(in case of RDBMS) it is automatically mapped to the Primary Key of the
foreign entity.
<relation>
has now afield
attribute instead of a phpName.Configuration
platformClass moved to since we support now several different database vendors at the same time. It makes no sense to provide a —-platform option to build all models with the same platform, because it would generate the wrong code for entities using different platforms. (imagine a database for mysql and a database for mongodb).
Entity class
The actual entity class does not have any parent class anymore. At wish a active-record trait is injected or can be used to provide active-record methods like save(), delete() etc. This is completely optional. Lazy loading is achieved through a Proxy class, which will be returned in case the entity instance is created through database queries. The entity class contains basically only the properties and getter and setter methods.
Implementation details are hidden now per default. Means if you have a relation from Car to Brand then Car doesn’t have a property
brand_id
nor its setter and getter method, but onlybrand
.Code generation
I’ve basically deleted all those extremely big builder classes and started from scratch, copying peace for peace if needed into separat component classes. I also
created several traits which help you as component author with some handy methods which were in the AbstractBuilder classes before (which was too big as well).
To achieve this I’ve also introduced a OOP way of generating PHP classes, methods, properties and so on. It looks like:
It generates PHPdoc blocks as well automatically. Double definition of methods are the past with this, although it’s possible to overwrite it acidentely through using incompatible behaviors that generate the same method. However, it’s now possible to detect easier if a method has been declared already or not.
Also the builder classes itself are now incredible slim:
Unit of Work
The unit of work pattern is living in Runtime\Session\Session class and provides us now finally the session-per-request pattern. Active-record is still session-per-operation since each save() call creates a new session and database transaction.
Compared to doctrine it’s similar, except the fact that we highly utilize the unit of work pattern to gain extremely more performance. For example we use not a simple topological sort based on mapping information (like doctrine) but a grouped topological sort based on the actual living entity instances. This gives us the ability to fire bulk INSERTs and in general more optimized queries. Compared to doctrine we gained with our unit of work 5 times faster execution times (for 10k inserting rows with simple relation). See benchmark chapter. I’m using this library https://github.com/marcj/topsort.php to sort anything by entity dependencies and grouping by equal type allowing us to have bulk insert/update.
Repository
The repository pattern gives us the ability to hook during runtime into entity lifecycle events. We have events like pre/post save/update/insert/commit. In a behavior you can still inject code into those events by just returning actual php code in the old behavior hook method. However, it’s sometimes handy to hook during runtime in some events, which is now possible. The $configuration has those event dispatcher - you can hook into all events there as well, which aren’t bound then to any repository/entity.
Examples
Why not just use Doctrine
Because we have now a much more simple way of defining entities and we are incredible much more faster during code generation and fully utilizing the unit of work pattern. Also we have way less magic in our classes.
Doctrine's UnitOfWork has 3.000 LOC, we only 100, although this will increase to few hundreds because of the missing circular references check.
Benchmark
Compatibility
Static creation of query classes are still possible. It uses the same static global configuration object as active record instances. This means the test suite with all its static invokations will still be compatible.
ToDo
[x] Implement Fetch
[ ] Implement lazy loading completely
[x] Implement INSERT
[x] Implement UPDATE (almost done)
[x] Implement DELETE
[ ] Implement circular references persisting fallback
[ ] Convert more behavior to new architecture
[ ] Implement Annotations support
[ ] Make test suite green again
[ ] More tests for repository and unitOfWork
I'm working on it now since some weeks and it will take some weeks. I hope to get some stuff done during the symfony conf.