jorge07 / symfony-6-es-cqrs-boilerplate

Symfony 6 DDD ES CQRS backend boilerplate.
MIT License
1.07k stars 187 forks source link

Update Credentials.php #80

Closed alirezarahmani closed 4 years ago

alirezarahmani commented 6 years ago

I think you forget make them private

jorge07 commented 6 years ago

Hi @alirezarahmani. Thanks for open this pull request. This is an interesting topic. This PR make sense, it will avoid mutations in the object so we can avoid side effects. But in another hand, when an object is used only as dto (data transfer object), like this case or the application commands, is common make the properties public so you don't need to define the getter. It avoid lot of "useless" code in large codebases but it need the commitment of all the team members to never, never mutate an attribute.

In open source projects, it totally make sense, as in php the attributes can't be defined as readonly like in typescript, so it will avoid side effects.

This PR need return types in the getters, probably some more refactor in the places this object is used and fix the tests.

cermakondrej commented 4 years ago

@jorge07 Also I would like to point out that yout current code is only working thanks to php not being strict on typing.

When you execute GetTokenHandler, your MysqlUserReadModelRepository::getCredentialsByEmail executes oneByEmail, which returns UserView. However, the Credentials ValueObject inside hasn't been properly hydrated and which result into something like this: Screenshot from 2020-02-18 22-42-09

So when email in this Credentials Value Object is then accessed, its converted to string while accessing this public property, which already is string. This poses no problem, because your Credentials properties aren't strictly typed, and are typed only through annotations :)

Which means, that if those properties would be accessed with getters and their return type would be enforced, this particular UseCase Would break down due to this hydration problem :)

jorge07 commented 4 years ago

/stale