symfony / symfony-docs

The Symfony documentation
https://symfony.com/doc
Other
2.15k stars 5.1k forks source link

Remove references to entity generation, replace by proper entity modelling #8893

Closed Majkl578 closed 3 years ago

Majkl578 commented 6 years ago

Hello,

there are lots of users using doctrine:generate:entities nowadays. This must stop.

Symfony should provide its large user base decent article on designing/modelling entities in a proper way, with business methods and as few setters as possible, not by generating them from database. Such generated entities lead to a tediously known anemic domain model, something that does more harm than good: breaks encapsulation and completely undermines the purpose of ORM. It'd probably be even better to not use ORM at all and use DBAL directly, the result would be very similar without runtime overhead.

Note that Doctrine 3.0 will remove support for code generation entirely, so the better we educate the users, the better.

This issue is a direct response to discussion in doctrine/DoctrineBundle#729.

Thanks.

javiereguiluz commented 6 years ago

OK, but let's discuss about this specifically for Symfony apps, not for Doctrine or software design in general. My question: if I remove all my setters in all my Doctrine entities, will the rest of Symfony components keep working perfectly as before? For example, the Form and Serializer components? Thanks!

ostrolucky commented 6 years ago

Form and Serializer components will keep working without getters/setters if you make your properties public. won't work with properties because of Doctrine proxies

I don't ever use doctrine:generate:entities, so I won't miss it. However, I use getter/setter generation in PhpStorm. Setters for instance are useful to ensure application doesn't set wrong data types to properties. It also allows to build fluent interface (I'm fan of it, I know ocramius isn't, don't care). Getters are useful to force users to use setters (otherwise properties would have to be public).

IMO getters/setters/public properties are necessary for RAD of CRUD applications. Most of the applications on the market are of this type.

Personally I use this approach with combination of domain specific features. I don't see a problem to have normal getters/setters in conjunction with domain specific methods in my entities. However, I don't like strict DD design. It's very tedious to write and I wish others would stop forcing it on us.

I also don't see how entity generator prevents users to "properly" model entites. If you guys don't like getter/setter generation, just remove that functionality. It's still useful for generating classes and properties.

javiereguiluz commented 6 years ago

By the way, in the "Getting Started" guide of Doctrine itself you can read this:

When creating entity classes, all of the fields should be protected or private (not public), with getter and setter methods for each one (except $id).

See http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/getting-started.html

sstok commented 6 years ago

The "problem" with setters is that they make the Model (Entity) anemic (as explained by @Majkl578 ). they don't communicate any meaning, your models should "speak" the language of the application (updatePassword, disable/enable) not setEnabled() πŸ€¦β€β™‚οΈ

But newcomers tend to struggle with this as it requires a good understanding of the domain and how you interact with it. But most of them are introduced with the easy and Rapid/CRUD building approach of getters and setters.

For CRUD it's good enough, but in a well designed application the Symfony framework is a detail you only care about at a later time (thus don't use CRUD, but DTO, Commands or something).

Most of this information comes from Cleancode from Robert C. Martin (Uncle Bob). highly recommended read for everyone.

Personally I use this approach with combination of domain specific features. I don't see a problem to have normal getters/setters in conjunction with domain specific methods in my entities. However, I don't like strict DD design. It's very tedious to write and I wish others would stop forcing it on us.

Can you give an example? I always try to use clear methods, but nothing forbids me from using a setter when there no better method available.

https://github.com/park-manager/park-manager/blob/master/src/Component/User/Model/User.php#L155

kironet commented 6 years ago

It's really confusing. They are forcing people to not to use anemic and in the same time anemic is forced to people by their docs.

Majkl578 commented 6 years ago

if I remove all my setters in all my Doctrine entities, will the rest of Symfony components keep working perfectly as before? For example, the Form and Serializer components? Thanks!

Yes, I realized slightly after posting this issue that these components leverage the same antipattern as well, unrelated on Doctrine. It should change there as well, but I can't really talk on that end since I have barely any experience with those. To answer your question: It depends. It will of course break the automation of i.e. Form, but it's easy to fix with DTOs/DataMapper (as shown in the article on Twitter). And as a benefit, the code will be cleaner and much less coupled. I don't really take an argument "it's too much writing" these days since we all use some IDE that has autocompletion, provides interface implementation etc., plus closures and anonymous classes (could be used as anonymous implementor of DataMapper).

Form and Serializer components will keep working without getters/setters if you make your properties public.

Please, no public properties, that takes us to PHP 4 age. There are better means.

I don't see a problem to have normal getters/setters

I don't see a problem with having getters (if their existence makes sense - not all fields are to be exposed), they don't change behavior. Setters on the other hand are different story, that's what this is all about.

It's really confusing. They are forcing people to not to use anemic and in the same time anemic is forced to people by their docs.

Please don't be confused, there is nothing to be confused about. We have to start somewhere, and it's here. This issue is about Symfony's documentation, Doctrine is going to work on the same issue as well (see cross-linked issue above).

It should also be noted that ORM 2.6, to be released soonβ„’, will still have EntityGenerator. It will be deprecated in 2.7 and removed in 3.0 next year. There is plenty of time to work on improving documentation on all sides of the barricade.

mvrhov commented 6 years ago

@Majkl578 You are citing the twitter post without linking to it!

GwendolenLynch commented 6 years ago

You are citing the twitter post without linking to it!

Well, the post referred to, both above and in the Twitter conversation was Value Objects in Symfony Forms by @webmozart

There is also his talk from Symfony Con Barcelona where he covers all this too.

javiereguiluz commented 6 years ago

Update: in my original comment I said "we're not going to make this change" where "we" means "the Symfony Docs team". I've changed it to "I'm against making this change" ... to be clear that this is my personal opinion.


I hope you understand my comment well: we're not going to make this change I'm against making this change. This breaks everything in Symfony for a dubious gain.

However, what we could do is to add a new article explaining how to use this "proper Doctrine modelling" in Symfony apps for those developers who want to use this. That would be a really helpful resource.

Levure commented 6 years ago

IMHO doctrine:generate:entity is quite usefull when you are migrating a legacy application with an existing database to a new Symfony application.

Why do you want to remove this tool ? Making things more complicated than they already are ?

alcaeus commented 6 years ago

@Levure honestly, how often do you reverse-engineer entities from an existing database?

Majkl578 commented 6 years ago

@Majkl578 You are citing the twitter post without linking to it!

You're right, sorry, I confused issues - it was mentioned in Doctrine's issue, not here.

I hope you understand my comment well: we're not going to make this change.

This makes me sad. How I understand this is that one person here likes messy code, therefore wants to close this issue down.

This breaks everything in Symfony for a dubious gain.

Not true. Especially since there is plenty of time to make anything in Symfony not break with clean code.

IMHO doctrine:generate:entity is quite usefull when you are migrating a legacy application with an existing database

This is one-off operation and you mostly need only mapping. You don't need whole PHP class full of setters.

zorn-v commented 6 years ago

I like that you want to REORGANIZE a principe. I realy think it is by good thinks. BUT Now you like a propel. Mark yourself as "alpha" and go on. (BTW ActiveRecord I like more, but integration...) Lets break usual things, like unable to regenerate entities while adding field to db (and use Atom for ex.). And ALL BE OK (and you want that field JUST IN ONE PLACE of code). Great improvement... Seems I go away from PHP...

PS. I'm totally dont understand argument about IDE

codedmonkey commented 6 years ago

I'm glad this issue has been raised, but in my opinion info on this topic is also lacking in the Doctrine documentation. The ideas raised in this thread alone sound very semantic and are underdiscussed in the Symfony community (at least the part I see). If the Doctrine documentation on entities can be improved, the Symfony documentation can focus more on using those models in the associated components like Form.

Ocramius commented 6 years ago

@codedmonkey https://github.com/doctrine/doctrine2/pull/6902

donquixote commented 6 years ago

(moving my comment from #7460)

A side note about "Anemic domain model". The idea that "anemic domain model" === bad is not as clear as some may think.

Martin Fowler said in 2003 that he does not like it.

A number of recent articles suggest that an Anemic model may very well be superior to a rich domain model with regards to testing and "SOLID" principles.

It is worthwhile to read some of this.

The main idea, as I see it:

How this applies to Symfony and Doctrine, I leave for others to discuss. As said, I have no intention to change the outcome of this discussion.

sstok commented 6 years ago

In reply to articles @donquixote shared:

This is a great example of what DDD is not about, have some of these authors actually read the books on DDD? Domain Driven Design is about an information Domain, at the hearth of the system (it even says so on the cover).

Having a method named movePlayerNorth() doesn't tell me anything, what is north in this direction, also is there a single Player, who is the player? Normally you'd pass it with the method. And why is there no "getter" method to get the current position to verify if the new position is correct.

There is absolutely no Domain concept in the whole article.

If calling a method on the GameState does something special, it should be traceable trough one of the injected dependencies (or is the author using a Singleton of sorts?), it doesn't just end here, if I store something I must able to ask the state, unless this is not part of the model and handled somewhere else.

I see no purpose in the whole article other then trying to convince me that a Ridge Domain Model is bad, but without a proper explanation. Also a pure function is a function that always gives the same result with the same arguments, it has nothing to do with a Model (DTO, Anemic nor Domain), a Model is about something that holds a state or value, that's whole freaking purpose of DDD, to Encapsulate a Domain state and make it's communications explicit. Not about pure functions.

He could have simple explained that Anemic Models are not all bad, that are situations were they are considered acceptable (like CRUD with RAD) for small applications or game development (as there are technical limitations that make DDD unpractical in this situation).

https://blog.inf.ed.ac.uk/sapm/2014/02/04/the-anaemic-domain-model-is-no-anti-pattern-its-a-solid-design/

class Customer : DomainEntity // Base class providing CRUD operations

This is so bad I cannot use words. (image removed) If your using plain-old CRUD in a Domain Model it's not a Domain Model, period! And using a ActiveRecord in a (Domain) Model, that is new level of failure. I, I, give-up... this is stupid, just stupid πŸ’€ (Edit: Anyone who have read the books of DDD knows this goes completely against separation of concerns and pretty much leaves the Domain coupled to Infrastructure, making such bold statements without knowing DDD is... ughh).

ActiveRecord is already about a Persistence Model, you don't put a Persistence Model inside an Domain Model and then complain it doesn't work, the author could have figured this themselves. ActiveRecord is an Infrastructure detail that in the sense of DDD belongs to another layer then the Domain. Yes, you can use ActiveRecord for persistence but it requires a manual mapping process between the Domain Model and the ActiveRecord Models (which is why ORM is favored for DDD, btw).


There is nothing wrong with using a Model that is considered Anemic, if DDD does not fit the design as there is no actual Domain or there are to many technical limitations. Or if the Application is absolutely to be short lived or starting from scratch with a proper Domain in-place doesn't require to much work.

But please for the love of sanity don't call it a Domain Model! Fowler called it an Anemic Domain Model because it's badly designed and contradicts Domain Driven Design. It is an anti-pattern, but a simple Model that is Anemic is not, just don't call it a Domain Model, OK? πŸ‘

Edit: I changed my wording a bit, It wasn't my intent to insult anyone; I was horrible shocked about some of the bold statements in the articles that completely missed there point.

donquixote commented 6 years ago

@sstok There is no need for insults.

The main question discussed in these articles, or what I take away from it, is which of the following two options is preferable:

  1. Classes for entities and domain objects also contain a lot of behavior / logic related to this entity / domain object. Sometimes they might need injected dependencies to do those things. Also all pluggable behaviors must be somehow injected into those entity objects.
  2. Classes for entities and domain objects are slim, and most of the behavior and logic lives in their own objects.

The articles I mentioned label these two options as RDM and ADM. You might want to disagree with that, fine. To me it seems quite consistent with the way Fowler used the term ADM in 2003.

In both of these options you can make a stupid example and then point out how bad it is. Some of the examples in some of these articles might indeed be a bit strawman-y. But I find the arguments convincing, or at least it shows that Fowler's post from 2003 is not universally agreed on nowadays.

a Model is about something that holds a state or value, that's whole freaking purpose of DDD [..] But please for the love of sanity don't call it a Domain Model!

If you use "model" as a term for the class itself, the class that holds the data, then yeah fine. If you use "model" to describe the architecture that maps real-world stuff into code, then I see nothing wrong with still calling it a model. I think this is how the authors of those articles use the term.

They call it anemic, if behavior and data live in separate classes/objects, but the entire collection of classes is still a model in this second meaning of the term.

sstok commented 6 years ago

@sstok There is no need for insults.

It was not targeted at your personally but the articles (and how I think about them) πŸ‘ (I should have made this more clear, sorry). I agree with the points you make, nothing wrong with an Anemic Data (not Domain) Model.

My only problem is with the way the linked articles try to explain there point, and how the authors failed to explain this (ActiveRecord? and then in the second example the author uses a Factory). I am (can we say) allergic to misinformation and improper explanation from these so-called experts, especially when telling me that a Ridge Domain Model is bad, and then giving poor examples.

I am open to discussion, and I can understand and agree to a difference of opinions, I believe something till someone can provide arguments to prove otherwise πŸ‘

(The DDD books actually talk about situations where DDD is not a suitable, which is why I don't believe the authors have actually read the books on DDD and merely base there opinions on something they heard from somewhere, it's dangerous and leads to misinformation and flame wars (I actually used to believe ORM is inherently bad because someone once told me that πŸ˜‘ )).

But to be clear, it was not my intent to insult you (or your believes).

donquixote commented 6 years ago

But to be clear, it was not me intent to insult you (or your believes).

Ok, then take my first sentence as defending those who are not here :) The picture you posted certainly does spread some negative energy.

My own motivation is this: There is package code, and there is application-specific code.

I think it is advisable to have a lot of reusable package-like code, and only a thin application-specific layer on top of that, which does nothing more but combine the building blocks from package code. Especially with a CMS, if you plan to build various sites which are all similar-but-different, then you want most of your logic to live in domain-agnostic components, not in domain-specific code.

As for object types, you have

If all your application-specific behavior is living in the classes that also hold your data, and that are instantiated per "entity", but you still want to compose this from reusable package-level components, then you will have to repeat the same composition for every new instance of a domain object. You could say they become "Frankenstein compositions" (I just made this up). Perhaps the entity instance is now encapsulated from the outside (not sure), but this does not buy you much.

With ADM, you would still have the equivalent composition in your service-level objects. But I think composition on this level is a lot more pleasant than having it in every entity.

javiereguiluz commented 6 years ago

@sstok expressions like --> this is stupid, just stupid and the image you posted don't help to maintain calm discussions. Of course I know you from Symfony Slack and I know that you are not a troll and you meant no offense. In any case, we don't have a Code of Conduct yet (it will be approved in the coming days) so we can't do anything about this ... but please, keep calm when discussing with others. Thanks!

sstok commented 6 years ago

Will do ❀️ Like I said, I have some serious issues with the contents of the linked articles, that's the reason for my excessive reaction, not to excuse my actions.

I changed my reply a bit, explaining it wasn't my intent to insult anyone (but I can't find a respectful word to describe my disbelief 🀫 ).

OK. I will shut-up now πŸ™‡πŸΌβ€β™‚οΈ πŸ˜ƒ (or at least be less aggressive or condescending).

maryo commented 6 years ago

As the name of the antipattern implies, anemic model means that the model lacks something very important like blood lacks enough healthy red blood cells or hemoglobin which is always bad if you want to live properly without external services like hospitals. I like the name of the antipattern as it nicely explains itself in a metaphore.

There are very rare situations where having only getters and setters should not be considered anemic because it is complete already and does not need to have more behavior than to exist and does not need to protect more invariants, it's just a data structure (something like simple Settings object for example). But this is inherent more to OOP itself than to DDD. It's an OOP antipattern and it's always bad.

There is always a better approach even in those reusable packages. There are certainly situations where using a hack or a bad practice is a good tradeoff for solving someone else's bad decision but that still does not make it a good practice.

In Symfony I use Commands and https://github.com/thephpleague/tactician-bundle. I bind these commands to Symfony Forms so the default PropertyPathDataMapper is mostly good enough. For value objects I use my custom form types where I use custom DataMappers by having the form types implement the DataMapperInterface, anonymous class could be also fine. I think this part of the Form component and the DataMapperInterface could be better designed, mapFormsToData(FormInterface[]|Traversable $forms, mixed &$data) . From my point of view it would be better to get rid of the pass by reference argument and the Traversable argument has no ArrayAcces so you need to convert it to array first using iterator_to_array to get to the elements you want. But otherwise it works fine and one does not need to use them if the one is OK with not having value objects bound to the form and creates them inside command handlers. Maybe a command bus could be a candidate for a new Symfony component. I haven't used Serializer component yet but serializing should work fine. Deserialization of entities is not a good idea I think, deserialization of commands should also be OK i think.

donquixote commented 6 years ago

As the name of the antipattern implies, anemic model means that the model lacks something very important like blood lacks enough healthy red blood cells or hemoglobin which is always bad if you want to live properly without external services like hospitals. I like the name of the antipattern as it nicely explains itself in a metaphore.

Some things are actually better if they are are bloodless and inanimate. It makes their behavior more predictable. E.g. you don't want your car keys to obtain life and jump out of your pocket.

There are very rare situations where having only getters and setters should not be considered anemic.

An object with only getters and setters can be an indication of poor design. If that's your definition of anemic, then I think I agree with you.

But should an entity object be able to load related / referenced entities from the database? I would argue an object that can do this has too much blood, not too little.

A good middle ground might be to let an entity object be responsible for a limited amount of internal or object-level constraints / invariants, which do not require injected services (or very few of them, perhaps?). Whereas "external" behaviors and constraints, and transactions between entities, would have to be handled by external services.

With this approach don't necessarily end up with entities which have only getters and setters. But you might want to call it anemic because it has fewer responsibilities.

So, perhaps this is just a debate about terms, in which case I have to apologize for the distraction.

donquixote commented 6 years ago

So, perhaps this is just a debate about terms, in which case I have to apologize for the distraction.

Perhaps #7460 would have been a more suitable place for this side-track. I just came here because the other one was closed as duplicate.

sstok commented 6 years ago

Maybe a command bus could be a candidate for a new Symfony component.

Actually, we now have the Message (or Messenger) Component πŸ‘

But should an entity object be able to load related / referenced entities from the database? I would argue an object that can do this has too much blood, not too little.

Loading entities from a database (or persistent storage to be more exact) in the Domain layer is an Infrastructure detail, so this would violate the separation of layers. You can make an Entity hold other entities (Aggregate) keeping the information together as you can't for example have a InvoiceRow without an Invoice to hold them, if you create the object but assign the invoice later your InvoiceRow would be an an invalid state as it's invariant is violated (a row cannot exist without an invoice).

Which is indeed against OOP principles in general.

Yes, you can make the $invoice a mandatory argument of the InvoiceRow constructor, this would fix it. It was merely an example πŸ˜‹

A good middle ground might be to let an entity object be responsible for a limited amount of internal or object-level constraints / invariants, which do not require injected services (or very few of them, perhaps?). Whereas "external" behaviors and constraints, and transactions between entities, would have to be handled by external services.

With this approach don't necessarily end up with entities which have only getters and setters. But you might want to call it anemic because it has fewer responsibilities.

Actually this is perfectly fine 😁 in fact it's Domain Driven Design (or to a degree) πŸ€ͺ. Many people misbelief that a Domain Model (Entity/Value object) is about doing everything (my former self included), that's in fact not the case at all.

A Domain Model (or Domain Object to be exact) is about encapsulating business knowledge (and process) into a model (in the broad sense), persistence and services do have there existence in-here but in the rightful place. Persistence is handling in Infrastructure (the Domain only defines an interface), while Services (depending on there purpose) reside in the Domain or Application layer. The Domain Services handle what cannot be done in a Entity/Value object because it doesn't belong there (like changing other entities), but is still strictly related to the business process.

The Application Services are usually more leaning towards the interaction (UI) part and e-mail messages (to name something).

Nothing personal, but if you haven't read any of the books on DDD (in particular the ones from Eric Evans), I suggest you do πŸ‘ when I got started with DDD most of my knowledge and information came from articles on the web, which give a good impression and are still helpful for some unclear cases, but don't cover all the topics what DDD is about, including why it was invented in the first place, muddying the waters and leaving me with half broken implementations and unworkable models. An Anemic Model would have, in fact worked much better then πŸ˜…

An anemic domain model as Fowler explained is about Entities without any own responsibility, all the actual business logic is placed in Services only, robbing the Entities from responsibility they are able to handle with ease (like a correct status), which basically turns them into Data transfer objects that are persisted.

I think most people misunderstand this and fear that a Domain Model is just to complex and that an Anemic model is easier to work with, or that they try to hard, putting everything in Entities. Granted, DDD is complex not something one can easily grasp (I am actually still learning the deeper details), and requires deep understanding of a business process. It's an almost completely new way of thinking that challenges us in many ways.

As I said before, an anemic data model is not bad perse, but it's not a Domain Model for sure.

I just came here because the other one was closed as duplicate.

No problem, it's good to have conversations like these πŸ‘ (my attack of before was completely unnecessary! It wasn't the first time I reacted, like this (but hopefully the last), and I am trying to more respectful and calm; Heck I am the one who wrote the (initial) constructive criticism guide for contributors 😳 ). I hope this doesn't discourage you from having conversations like these πŸ’š

maryo commented 6 years ago

So, perhaps this is just a debate about terms, in which case I have to apologize for the distraction. I believe it is. But i think it is quite important to agree that an anemic domain model is always bad practice in OOP applications so Symfony deciders feel the need to present better examples inside docs and so on.

Some things are actually better if they are are bloodless and inanimate. It makes their behavior more predictable. E.g. you don't want your car keys to obtain life and jump out of your pocket.

:-D. Yes. The intent of the key is probably very different.

An object with only getters and setters can be an indication of poor design. If that's your definition of anemic, then I think I agree with you.

Exactly. It's an indicator. But there are situations where having classes with getters and setters only is OK because they are actually not fully-fledged objects. Their intent is just to carry information. But the domain objects with their behavior still must exist. If they don't then it is closer to procedural programming where you don't send messages between objects but manipulate with datastructures (= calling setters or setting public properties on "entities") by calling procedures (mostly by calling methods on so-called service objects).

But should an entity object be able to load related / referenced entities from the database? I would argue an object that can do this has too much blood, not too little.

I would agree.

A good middle ground might be to let an entity object be responsible for a limited amount of internal or object-level constraints / invariants, which do not require injected services (or very few of them, perhaps?)

Sort of. You don't need to put everything into entities. But the object should always be consistent and it should protect all it's invariants ie never get into an invalid state.

Actually, we now have the Message (or Messenger) Component OH. You're right. I forgot this new component.

And although I like DDD I think we don't need to refer to it since these principles are inherent to OOP.

donquixote commented 6 years ago

If they don't then it is closer to procedural programming where you don't send messages between objects but manipulate with datastructures (= calling setters or setting public properties on "entities") by calling procedures (mostly by calling methods on so-called service objects).

For me the main benefits of classes and interfaces vs procedures are dependency injection (and thus, behavioral parameterization), named contracts for behavior (interfaces), and values with type and internal constraints (value objects). The first point you also get with functional programming (injecting e.g. parameterized closures instead of objects), the second part (contracts) you don't, at least with PHP, which has no way to enforce a callable signature. Also classes can have nice and descriptive names, closures do not.

You can get very far with (immutable) value objects and one-or-few-method "atomic" (also immutable) behavior objects. This will tick all the "solid" boxes, it lends itself to all the compositional patterns, etc. It does not really follow the traditional "messages between objects" and "object = behavior + data" idea about OOP.

A radical thought is that at some point you don't even need those mutable and "rich" entity objects anymore. If we are honest, the "real" entity is living in the database. An entity object is only ever a snapshot of what was in the database at a given time. And modifying an entity object only modifies the snapshot, until it is saved to the database again. A lot of what is typically done with mutable entity classes could also be done with an immutable EntitySnapshot value object (the two things might not be so different technically, but conceptually they are). I am not sure how far you could fly with such a concept, or where you would hit a wall. You also might not call it DDD anymore, perhaps.

So, where am I going with this?

Sort of. You don't need to put everything into entities. But the object should always be consistent and it should protect all it's invariants ie never get into an invalid state.

I think the key would be to distinguish internal object-level invariants from external application-level invariants. If we call the thing "entity snapshot" instead of "entity", then this distinction becomes more obvious.

sstok commented 6 years ago

which has no way to enforce a callable signature

I am properly misunderstanding something, but since PHP 7.1 (or 5.6, I keep forgetting) we have the callable type hint πŸ‘

You also might not call it DDD anymore, perhaps.

In DDD the persistence on object model is a detail (the Domain only holds an interface for the repository) but the actual business logic lives in a Domain Service (Entities is not concerned with this).

Mutable and "rich" entities / domain objects, even if you do everything right, do not score the best in terms of "solid" and perhaps other criteria of complexity, if compared to value objects and atomic behavior objects (if someone has a better term for this, let me know).

Ah now I am getting (no pun) you πŸ˜€ you are talking about the mutable state of objects.

The way I understand it, a value object is equal when it holds the same value as another value object. But an Entity is about something that exists uniquely, you only have one User with the ID and therefor the Entity is mutable, you can make the User immutable but pretty much leaves you with two Users who can no longer be unique identified.

It also depends on the usage context, if in the current context a user is nothing more then an ID an immutable value object is correct, an entity doesn't make sense here as there no such as an unique User entity within this context.

This was also the "trap" I felt for when getting started with DDD, make everything immutable. In general immutable is indeed better then a mutable object, but sometimes this is simple not possible or logical.

Edit: An Entity usually also children which only exists within the context of the Entity (Aggregate), like an Invoice with rows. In general I agree that Value objects are perfectly valid here as the rows don't have to be uniquely identifiable (only within invoice, which can be handled by the Invoice itself) πŸ‘

donquixote commented 6 years ago

I am properly misunderstanding something, but since PHP 7.1 (or 5.6, I keep forgetting) we have the callable type hint.

This does not tell you about the signature or semantic contract of the callback. You can pass any callback into this parameter. An object-valued parameter with an interface type hint is much more specific. See e.g. psalm/issues/580. Note that this is about signatures only, whereas interfaces also give you a semantic contract, so this would be more like phpDocumentor2/issues/1689, or see also https://softwareengineering.stackexchange.com/questions/286942/is-the-semantic-contract-of-an-interface-oop-more-informative-than-a-function?rq=1

The way I understand it, a value object is equal when it holds the same value as another value object. But an Entity is about something that exists uniquely, you only have one User with the ID and therefor the Entity is mutable

And this is already a lie built into the architecture, because multiple requests can have their own "article:4" object, which are always a snapshot of the user database record at a given time. In some case you might have an id-less article object, if it is just a draft existing in memory. Or you might have multiple instances with the same id, but distinct "revision id". Of course you are correct, having an id is the common distinction between value object and domain object / entity. To do this with value objects, you might have an id-less "article value" value object, and then an "article snapshot" which contains an id + an "article value" object. If you only have an "article value", you don't know whether this is a draft or a revision or from a "real" up-to-date entity record. But even if you keep the id as part of the structure that also holds the values, you might still find a way to implement and treat and think of it more like a value object than an entity.

sstok commented 6 years ago

This does not tell you about the signature or semantic contract of the callback. You can pass any callback into this parameter. An object-valued parameter with an interface type hint is much more specific.

πŸ‘

Or you might have multiple instances with the same id, but distinct "revision id". Of course you are correct, having an id is the common distinction between value object and domain object / entity.

Very interesting πŸ€” But I guess you can solve this with an aggregate where each revision is a child of the article, they all have the same id but a different revision id. Secondly, each revision cannot exist without the other (otherwise they are just loose entities) πŸ˜› but this is hard to judge as we are talking about theories here without an actual Domain based on a deep understanding πŸ˜„

Thanks for this discussion. All in all it depends on the situation and problems your solving, an immutable data model is has it's pros and cons, and a mutable model like wise πŸ€·β€β™‚οΈ but this surely was an interesting talk.

tvogt commented 6 years ago

there are lots of users using doctrine:generate:entities nowadays. This must stop.

Or you could stop telling people how to code. If you don't like it, good for you but why should I care? And why do you care how I write my code?

Ocramius commented 6 years ago

And why do you care how I write my code?

  1. you don't code alone, and if you did, then you wouldn't bother commenting here in first place.
  2. your code has a real impact on your work environment and on anyone that has to do with it, it's not just slamming rocks together
  3. all the "bad practices" (as recognized by those who analysed the up/down-sides) being promoted by open source software reflect badly on the ecosystem, leading to further bad practices, loss of time/money, loss of reputation and generally to bad developer satisfaction.
  4. all the "bad practices" (see above) being promoted by open source software get back at said open source software like a boomerang. "If we support shit, we have to deal with shit" is a more accurate way to describe the phenomenon.

Nobody is telling you how to code: you can draw your own conclusions from reading any documentation on any project at all, but if we teach bad practices in first place then we're making a disservice to the community at large.

javiereguiluz commented 6 years ago

This comment is not directed at any specific person or any specific previous comment ... but I think this discussion is slowly going into some territory we don't want it to go.

Just a friendly reminder: in the Symfony community is mandatory to be nice to everybody and that includes people you disagree with. Here are some useful links:

Thanks!

Ocramius commented 6 years ago

but I think this discussion is slowly going into some territory we don't want it to go.

I think this is exactly where the discussion should go: we had these kind of "betterment-oriented" pull requests multiple times on symfony/symfony-docs, and they usually get pushed back due to the ecosystem still being oriented at producing code quickly rather than producing code that lasts or that can be relied upon years later without constant work.

symfony/symfony has an excellent code-base with numerous eyes looking at every patch, which makes the final result reliable and stable: the practices proposed in the symfony documentation don't go in that direction.

The target audience is obviously different, but the damage done by the practices that are put under discussion is very real and evident in many projects.

Since core members of the ORM team (OP) are telling that something is wrong and requires some kind of adjustment, I'd suggest keeping the discussion focus exactly on that, maybe even figure out why the docs lead to these downstream problems.

javiereguiluz commented 6 years ago

I think this is exactly where the discussion should go

I wasn't talking about the technical side of the discussion. I was talking about the tone, which was slowly increasing in the wrong direction.

dominikzogg commented 6 years ago

Anemic models vs. Domain modals have both pros and cons. Telling someone that one of them is bad practice in general is wrong in my point of view.

Anemic models lead to invalid states (if not post validated), leads to a worse understanding of the domain. Domain models needs a pre validation or they fail at the DX of a api consumer by exposing one error after the other instead by returning them all. Or RPC like apis like /user/changepassword

Ocramius commented 6 years ago

Domain models needs a pre validation or they fail at the DX of a api consumer by exposing one error after the other instead by returning them all.

This is actually good DX, since it tells you an exact failure for the scenario you try to implement. You can also easily accumulate errors for anything trivial (VO shape, identifier existence), but anything with side effects is a no-go. A business interaction is not just a n UPDATE on a DB.

Or RPC like apis like /user/changepassword

I strongly encourage using RPC APIs for any kind of domain interaction (any at all). Keep PATCH, PUT, POST and DELETE with CRUD-only applications, and use restful resources only for reads (which is where REST really shines).

The idea of using REST for business interactions is pants-over-head silly.

codedmonkey commented 6 years ago

Discussions on how REST API's should look can be endless at this point in time, so let's refrain from going there. Ultimately I don't see a huge difference in an anemic model vs a rich domain model, the lines blur anyway the smaller the entity is. To push people in the right direction we should make a page where we advise on common pitfalls like:

Instead of doing $entity->getType() === Entity::TYPE_FOO everywhere to check for correct typing, create a new method on the entity so that you can do $entity->isFoo().

Instead of making a new business contact entity from scratch everywhere, create a new method $contact = $business->createContact('dodgy donald', 'Some Avenue 1', 'Washington DC') to simplify the creation of new entities.

I think if we teach people the most basic interpretations of the rich domain model they'll have a better chance at making informed decisions for their own domain model.

Ocramius commented 6 years ago

@codedmonkey ack. Indeed, couldn't resist the "but it's not RESTful" tease, so I digressed. The API suggestions you made are exactly what the issue is about.

tvogt commented 6 years ago

I agree that Symfony should not encourage bad practices by making them the best or only way to use the framework. But if I want to use Symfony for RAD, why can't I? Maybe I am exactly looking for a prototyping tool and what I write today will be thrown away or refactored anyway.

I think there is too much desire to force people down a specific path in this approach. And even if you are entirely sure that this path is the best path - do you really think you can say that with certainty for every person in every situation all the time?

There are a lot of things I like in Symfony. There are many things I would wish for. And there are some that drive me crazy. Having to explicitly write getters and setters is one of the things that makes me go "WTF do I have a computer for?" - someone took a maybe good idea and pushed it until it ended up in crazy country.

Encourage good practices. But don't sabotage users just because they have a different opinion on what is right. Always assume that they know their situation better and are the better judges. Give people the opportunity to make informed choices.

JarJak commented 6 years ago

You shouldn't write getters and setters anyway :)

tvogt commented 6 years ago

Because I would never in any universe want to do something as simple as setting a variable to a value, right? I absolutely have to come up with some "business logic" so that "setLocked(true)" becomes "set_a_lock_on_this_thing()".

JarJak commented 6 years ago

I mean, if you want RAD and use autogenerated setters without additional logic, then just make properties public.

alcaeus commented 6 years ago

@tvogt You don't want setLocked(true) to become set_a_lock_on_this_thing() - you want it to become lock(), with setLocked(false) becoming unlock(). This also gives you the chance to do different things in those methods (depending on your domain, locking and unlocking could have very different consequences) without wrapping the entire code in a conditional:

public function setLocked(bool $lock): void
{
    $this->locked = $lock;

    if (!$lock) {
        // Trigger additional business logic to inform something
    }
}

In general, the Single Responsibility Principle already suggests that the setLocked method is the wrong approach, since it handles both locking and unlocking.

tvogt commented 6 years ago

@alcaeus you are nitpicking on a detail. I could have picked "setNumberOfCats(2)". You are trying to tell me that I should write "remember_I_have_one_cat()" through "remember_I_have_nine_cats()" ? Of course not. Again, sometimes I really just want to set a variable, or read a variable. Maybe @JarJak is right.

Variables aren't even the main concern. I really, really liked the code-generated relationship methods. $entity->AddOtherEntity($otherentity) was very nice and useful.

Maybe I just missed the last five hypes on programming paradigms. After all, I've been writing PHP code since version 3. Probably just got stuck in stuff that simply works.

Ocramius commented 6 years ago

Again, sometimes I really just want to set a variable

Keyword "sometimes"

JarJak commented 6 years ago

@tvogt I think you are able to generate them with Maker Component (correct me if I'm wrong).

Ocramius commented 6 years ago

@JarJak that needs to be avoided: switching from one bad tool to another tool that just allows you to repeat the same mistakes.

JarJak commented 6 years ago

@Ocramius I know it's bad, but Maker Component is where people can find similar (still wrong in terms of good practices) functionality. And IMO Maker is good RAD / prototyping tool. I don't think everyone should be pushed by having to use Rich Entities. Even if they are treated as good pattern, they don't fit in every case.