Static calls provoke bad code and low usability #545

Closed
kix opened this Issue Feb 20, 2014 · 10 comments

Projects

None yet

3 participants

@kix
kix commented Feb 20, 2014

Given a basic Propel setup, the classes generated by model:build command are mostly supposed to be used statically, e.g.:

use MyBundle\Propel\Model\UserQuery;

$userQuery = UserQuery::create()->findPk(1);

This is quite bad, because if I'm in a Symfony2 environment for example, then there is basically no way for me to write ORM-agnostic code that would rely on persistence services rather than static calls.

Whenever I want to write a class that's abstracted from persistence (when I have not decided yet whether to persist it or not), Propel does not allow that: each and every class has to be a Propel model!

Also, I won't ever be able to define persistence as a dependency for my services, which then means that I cannot ever say that service A does persistence work and service B does not. In general, this also makes hard to write bundles and other code that would not have a hard dependency on Propel.

Static calls provoke smelly code. If I was using plain PHP templates, and I was unaware of concern separation principles, then it would be quite easy for me to run queries right in the template. I could even construct an instance of my query right inside the view!

And if Propel queries were somehow pushed into the DI container, such a trick would be much less possible, `cause it would involve pushing the whole DI container from my controller into the view.

I suggest to find a way to somehow forbid Propel generators to allow saving model instances with save() calls and to remove create() factory methods from queries. Such a change could be handled by Propel or PropelBundle configuration.

@marcj
Member
marcj commented Feb 20, 2014

So, what is your questions/concern?

You don't have to use those static methods, they are only helper stuff. new UserQuery() is possible as well.

@kix
kix commented Feb 20, 2014

@marcj I suggest adding an option for Propel/PropelBundle that would disable the generation of static factory methods. Thanks for your suggestion on new UserQuery — this seems like a good way of putting queries into Sf2 DI container as services.
But it doesn't solve the save() model method problem: these method calls could still be scattered all over the code. I understand that this is an ActiveRecord implementation and that save() is essential in this pattern, yet it would be great if by changing an option I could disable this behaviour too. Or is there a way to put save() out of model instances too?

@kix
kix commented Feb 20, 2014

@marcj, thanks! That's seems close to what I needed, yet the doc states that it'd disable the mutator/setter methods too, and that would make the model unusable.

@marcj
Member
marcj commented Mar 3, 2014

Nah, then the documentation is wrong. When readOnly is true all getter/setters will be created. Where did you read that?

@kix
kix commented Mar 3, 2014

@marcj, as of Propel 1 doc, take a look here: http://propelorm.org/Propel/reference/schema.html#table-attributes
It states:

readOnly suppresses the mutator/setter methods, save() and delete() methods

And for v.2: http://propelorm.org/reference/schema.html#table-attributes

@marcj
Member
marcj commented Mar 3, 2014

Mhhh, @willdurand @cristianoc72. In current Propel2 all getters/setter are created when I have readOnly="true" except save&co.

@willdurand
Member

It would make sense to remove setters as well.

@marcj marcj closed this Apr 15, 2014
@marcj
Member
marcj commented Apr 15, 2014
@kix
kix commented Apr 15, 2014

@marcj, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment