Skip to content
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

ActiveRecord instantiate way #5771

Closed
Ragazzo opened this issue Oct 25, 2014 · 28 comments
Closed

ActiveRecord instantiate way #5771

Ragazzo opened this issue Oct 25, 2014 · 28 comments

Comments

@Ragazzo
Copy link
Contributor

Ragazzo commented Oct 25, 2014

I think it is better to create new active records when fetching them from the repository without calling the constructor, it can be done through the reflection . The big advantage here is that we dont need to spread our application logic and separate it from simple fetching entities that has nothing to do with real logic, and we dont need to override instantiate() method.
I dont think there will be a lot of problems with speed, since we can save already processed classes reflections and just clone new object from one created by reflection. Even more, i dont think that there will be involved more then 10 entities per request, usually it is not so big . Doctrine 2 uses other approach but very similar and this , afaik it is also used in phpunit and other fw to create class not through reflection
What do you think @qiangxue @samdark @cebe ?

@cebe
Copy link
Member

cebe commented Oct 25, 2014

I think it is better to create new active records when fetching them from the repository without calling the constructor

why? this is completely unexpected behavior even if it was possible...

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 25, 2014

why? this is completely unexpected behavior even if it was possible...

actually because new User(new Profile()) and new Order(new Customer()) has nothing to do with instantiate() internal AR method . Domain logic leaking into infrastructure because of AR problems, that actually can be avoided

even if it was possible...

i mentioned 2 solutions in first post

@cebe
Copy link
Member

cebe commented Oct 25, 2014

I am more interested in the why than in the how. Please elaborate about your use case. What is new User(new Profile())? why would you do that? Please describe your problem before describing a possible solution that is hard to grasp without knowing the problem.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 25, 2014

This was examples of problem when i want to use constructor, but cant without overriding instantiate() method and making is_null checks in constructor, that is actually wrong, as i said in this way my logic now leaks into infrastructure layer of framework but it should not. I use constructor for domain logic things, each order should have customer -> new Order($customer), user should have a profile (that acts like VO to an aggregate of user) -> new User(new Profile()) and so on, some basic things to keep correct class invariant and object state .
The other way around is to use Doctrine2 that is what i do, but i think this feature can be implemented in Yii2 since AR is not simple mapper but the unit that should hold domain logic

Now what i am doing :

$profile = new Profile();
#add some attributes, etc

$user = new User($profile);
#perform some user management

$user->create(); //saving and raising event 
class User extends ActiveRecord
{

    public function __construct(Profile $profile = null)
    {
        #save in create() method and raise event UserCreated
        if ($profile) {
            $this->profile = $profile;
        }
    }

}

This is bad, because instantiate makes me put the required class dependency to null.

This a common issue to ORM and usually handles it in the way i've described, so the user dont want to worry about fetching entities and that they will spoil their domain . You never pass arguments to AR constructor or what was the question since as for me it is very often used ?

@lynicidn
Copy link
Contributor

@Ragazzo i up it #4256 #4270 and #4078

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

@lynicidn you suggested a little bit different thing , however i see your point , yes general purpose is to avoid unneded creating of models through constructor when fething , modern ORM should not do this , yes ;D

@cebe
Copy link
Member

cebe commented Oct 26, 2014

It is quite common to do initialization in init() method which is called in constructor so I do not think we should skip this step.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

what kind of initialization ? Cant it be done via afterFetch method or similar like it is done in Doctrine2 ?

@cebe
Copy link
Member

cebe commented Oct 26, 2014

Initialization that is used for every record, meaning newly instantiated as well as fetched from the database is best placed in init() method. Imo it is wrong to skip the constructor when creating an object. __construct is expected to be called every time a new object is created and not sometimes.

When your business logic has to ensure that a model is always related with some other record, you already have this ensurance by using a NOT NULL constraint on the database field, don't you?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

Imo it is wrong to skip the constructor when creating an object.

dont understand, i gave you correct solutions almost on every your argument. Why then having before / after save and so on methods ? Can you give me correct init method example, meaningful one ? Is that something personal here that you dont want to accept my suggestions ? ;D

When your business logic has to ensure that a model is always related with some other record, you already have this ensurance by using a NOT NULL constraint on the database field, don't you?

we are talking about different things here, i am talking about domain while you see entities relationships only as user_id -> id and so on.

__construct is expected to be called every time a new object is created and not sometimes.

when you will stop seeing AR only as simple mapper but as a unit for logic and entity you will understand what i am talking about ;) When you create new entity and pass to it needed dependencies on other entities it is part of domain, but when you fetch it you dont need it to do it again. Just dont think of AR as simple class

@cebe
Copy link
Member

cebe commented Oct 26, 2014

dont understand, i gave you correct solutions almost on every your argument.

I understand that it is possible to create a class without a constructor. My point is that if I write a constructor, I'd expect it to be called every time an object is created (see question below).

Is that something personal here that you dont want to accept my suggestions ? ;D

I am just trying to understand your idea, nothing more. It seems we have a communication problem so that you do not find the words to explain in a way that I understand or I am not able to express my questions in a way that you understand ;)

we are talking about different things here, i am talking about domain while you see entities relationships only as user_id -> id and so on.

I do not really get why you are passing objects to the constructor. As far as I understood these objects are related records, aren't they? Maybe you could show some more code or link to some resources that explain this kind of programming style.

As far as I understood you are proposing to pass related objects to constructor to ensure the relation is always there. However when fetched from the DB you propose to skip the constructor. This way you circumvent your own logic of ensuring an object is always passed, don't you?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

I am just trying to understand your idea, nothing more. It seems we have a communication problem so that you do not find the words to explain in a way that I understand or I am not able to express my questions in a way that you understand ;)

Well, i have, i am not sure if you will understand them since their from DDD and not sure if you up to it (not an offense of course), the problem that i want to avoid is persistence ignorance problem, when storage directly impacts on your entities, as we can see here we have some big problems with that )

I do not really get why you are passing objects to the constructor. As far as I understood these objects are related records, aren't they? Maybe you could show some more code or link to some resources that explain this kind of programming style.

Simple example is new Post($author) and not as $post = new Post(); $post->author_id = $user->id some more collections handling and other things in Doctrine2 and this i am not sure if any other examples needed, i thought it is clear ) like when you create some class and it is an entity depending on other entity you pass all needed arguments to the constructor ) When you think to much about id and string and integer your code just too much data-centryc, you dont have any ids in your contracts in domain and so on

This way you circumvent your own logic of ensuring an object is always passed, don't you?

no, when i create new User($profile) or new Post($autor) i create it because of contracts and domain model, and not because of it is simple cool class dep, i do it to keep class invariant and so on in domain
But fetchig object from repository has nothing to do with creating it like on domain level

@cebe cebe added this to the 2.1.x milestone Oct 26, 2014
@dapatrese
Copy link

What about using some kind of factory methods instead of constructors when creating models on domain level?

$order = Order::create( Customer::create() );

instead of

$order = new Order( new Customer() );

This could be a project specific convention on top of Yii standards that is easy to implement without changing the Yii code itself.

As far as I understood it, the DDD repository concept does not exactly map to Yii's ActiveRecord and ActiveQuery (and also relies on non-Yii features like identity map)?

@cebe
Copy link
Member

cebe commented Oct 26, 2014

and also relies on non-Yii features like identity map

you can implement an identity map by overriding the instantiate method btw.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

What about using some kind of factory methods instead of constructors when creating models on domain level?

no, factory only needed when it is a real situaton for it, there are some holywar things going on this one like you only should create entities through factories and so on, but you always should think by your head . You can create them only when it is needed by demand from other BC entities or if this creating process is complicated

This could be a project specific convention on top of Yii standards that is easy to implement without changing the Yii code itself.

the point is that Yii2 follows AR and it is just another way of mapping and so on, we dont want to make it full copy of real DDD ORM like Doctrine2 with full PI, identity mappings and so on. If you feel you need it, it is better to use Doctrine2 ;D

The question for this issue is only to avoid this vital and main PI issue - reconstituting entity through the constructor. Afaik java allows you to have different constructors so there is no problem there, in php you do it by unserializing from string or reflection

@dapatrese
Copy link

I totally agree that Yii2 AR and Doctrine2 are two different approaches.

When you call it a "vital" issue, does that mean that the current constructor implementation would also break the alternative approach using factory methods?

BTW: I'm far from advocating any holy "must only be done like this because it must" mantra. Factory methods just seemed like a pragmatic solution ;-)

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

does that mean that the current constructor implementation would also break the alternative approach using factory methods?

dont quite understand what are trying to say. However lets stick closer to Yii2 and this issue, and dont get far away, since discussion could get hijacked

@dapatrese
Copy link

I was olny tying to say: If the way constructors are used in current ActiveRecord implementation prevented common approaches like factory methods, then I would agree that there is a vital issue with constructors. But this is not the case, right?

Both processes, fetching existing models AND creating new models, might want to claim the constructor for their needs. We should imho refuse it to both of them and use separate methods for each.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

Both processes, fetching existing models AND creating new models, might want to claim the constructor for their needs

yes, as was said in java you can have different constructors so there is no problem there

We should imho refuse it to both of them and use separate methods for each.

we should not refuse it to the domain layer, so it can use it for their needs, however fetching and other things should be done avoiding calling excplicitly constructor, see some links above.

I was olny tying to say: If the way constructors are used in current ActiveRecord implementation prevented common approaches like factory methods, then I would agree that there is a vital issue with constructors. But this is not the case, right?

in your example and to what you are trying to refer Order::create() and so on, will not work if it is factory from domain layer since it always using entities construcotr, if it is one fron infrastructure layer and is used for fetching and so on, than it will use unserialize or reflection, that is two different solutions (constructor vs serialize/reflection) for yours first question about refusing both methods

@dapatrese
Copy link

So far it boils down to the question: What is the better tradeof?

A) Using serialize/reflection for infrastructure layer (for everyone)
B) Using Order::create() instead of new Order() in domain layer (if someone likes the concept of entity constructors and needs factory methods as a workaround)

While A certainly looks better for DDD, I'd still prefer B as a simple general purpose implementation, as Yii2 is not a genuine DDD framework.

Do you see advantages of a serialize/reflection approach other than enabling entity constructors?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

Do you see advantages of a serialize/reflection approach other than enabling entity constructors?

it is not vs in the way you specified it, it is the understanding that in your domain you have entities that can define different relationships and owning types. Serialization/Reflection currently used in php because it dont have ability to overload constructors like in other programming languages, and you cant define 2 constructors. If it was possible then we could use special constructor for creating entities without touching original entity constructor that is used for the domain layer.

As for question B as was said, you should use factory only when creating object is really complicated, you can google some more question about it. However it is not a general question of current topic, so lets skip it for now

@dapatrese
Copy link

I dont want to discuss when to use factories or not. My point is, that serialize/reflection is introducing some complexity. So I'm interested if these approaches also have any are other advantages.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

My point is, that serialize/reflection is introducing some complexity.

It dont introduce complexity since you will not be using it , it is only used when fetching entity , internally by fw. And it looks like you totally missed the point about PI and other things discussed above ) Lets stop for now and wait some other developers . Thanks

@klimov-paul
Copy link
Member

The whole idea does not seem right to me.

Avoiding invokation of the constructor, while fetching existing records, sounds like a hack. With such appraoch we attempting to get something from programming language, which it does not actually provide.
Indeed, from the domain point of view creating new AR object (invoke constructor) and fetching exiting records from DB are different things, and it whould be nice if in second one contructor will not be called.
However, while this may be logical for people familiar with DDD, for others this can be inconsistent behavior. Usually developer understands that each object instantiation invokes constructor and usually he relies on it. It whould be surprising for him to find out that is not always true for AR.

Another point is overriding constructor is not a common practice for Yii. Yii objects and components are configured via public fields and virtual properties. Just remember your application config:

[
    'controllerPath' => '/path/to/controllers', // virtual property
    'components' => [
        'db' => [
            'dsn' => 'yii\db\Connection', 
            'dsn' => 'somedsn', // public field
        ],
    ],
]

Widgets are created in the similar way:

<?= DatePicker::widget([
    'model' => $model,
    'attribute' => 'from_date',
    'language' => 'ru',
    'clientOptions' => [
        'dateFormat' => 'yy-mm-dd',
    ],
]) ?>

Using constructor to pass some configuration is just not "Yii style".

In the particular example:

$user = new User(new Profile());

situation can be solved via link() method or simple validation rule. It will not be so pretty, but still it seems fine from domain point of view:

// $profile can be an attribute with  validation rule:
$user = new User();
$user->profile = new Profile();
$user->save(); // no saving if $profile is invalid.

// Or we can use `link()` method
$user = new User();
$profile = new Profile();
$user->link('profile', $profile);

Also, I don't think 'Doctrine' is a good example in this matter: it processes relations in different way, so it uses a different notation and syntax, which is not common for Yii.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 26, 2014

@klimov-paul actually link can not be used, since entities should be in valid state always, and profile should be passed to constructor, so user will not be able to create User without profile. It is a contract limitation, similar like in your Yii2 classes you pass objects to constructors or if it is an array you check that everything is in place

Another point is overriding constructor is not a common practice for Yii. Yii objects and components are configured via public fields and virtual properties. Just remember your application config:

i dont think that there will be some problems here, since the last one argument can always be $config = [] that will be passed to parent constructor.

Also, I don't think 'Doctrine' is a good example in this matter: it processes relations in different way, so it uses a different notation and syntax, which is not common for Yii.

It is a different ORM, it keeps point on domain yes, however i think that some basic persistence ignorance issues can be solved by Yii2 too. I dont think it shoul be very database coupled, however as you noticed i dont demand to implement all features or something like that.

You noticed that for Yii2 developer it will be very unsual to skip constructor when fetching, but is not it a point to ask the question of why it was skiped ?, in this awy developer will understand that it is two different processes and so on. I think that asking questions and learning is good , while you saying that Yii2 developers should not think and only use what is provided by fw )

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Oct 27, 2014

@samdark can you vote on this one ? If "No", then more then 50% developers are against and i will close this one, to not to introduce any confusion ;)

@samdark
Copy link
Member

samdark commented Oct 27, 2014

I haven't seen this one. Can't vote on it now. I need to understand it first...

@samdark
Copy link
Member

samdark commented Oct 27, 2014

Moved to new issue since discussion here went in totally wrong direction.

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

No branches or pull requests

6 participants