-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
Expressions over Embeddables #28
base: 2.0.x
Are you sure you want to change the base?
Conversation
Somehow the build doesn't work with hhvm-nightly (something with Composer?), but it works on the stable hhvm. |
@@ -34,6 +34,8 @@ class Comparison implements Expression | |||
const GT = '>'; | |||
const GTE = '>='; | |||
const IS = '='; // no difference with EQ | |||
const SIM = '~'; // similarity: loose comparison; |
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.
Wondering if we can actually re-use CONTAINS
here instead. I wouldn't introduce more operators than necessary.
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.
How would the semantics of CONTAINS
apply to comparing Value Objects?
Using CONTAINS
for objects implies that you refer to internal properties, but that is something that should be avoided.
We want an operator that can say new Street("High Street 1") == new Street("High Street 1")
. That is something else than, for instance, new Street("High Street 1") contains "High Street 1"
if that's the behaviour you're thinking of.
@Ocramius Any chaing to get it merge? This is also fundamental when you work with custom mapping type. |
What is the status on this? |
Doctrine 2.5 will bring support for Embeddables. Embeddables are almost perfect for implementing Value Objects for doing Domain-Driven Development, but strict comparisons in Doctrine make this difficult. With Value Objects one can build rich and expressive domain models. It would be great if there was support for this in Collections.
The
ExpressionBuilder
currently contains the following notice: "You have to use scalar values only for comparisons". My proposal violates that. So it is the question if there should be proper support for Value Objects or not.Example of desired behaviour
This will not work, because the equality operator will do a strict comparison. In other languages, one can override equals() and getHashcode() inside the Value Object itself to get the desired behaviour, but we cannot do that in PHP.
Solutions
Currently, I think there are three solutions:
We can check if the argument that is passed to
eq()
is really an object withis_object()
. If it's not an object, we do a strict comparison as before (===
), otherwise we do a loose comparison (==
).However, this could potentially break client code! I don't think it should, because comparisons currently only allow scalar types, but you never know how clients use the code.
This option has one advantage. The same solution (loose/strict comparison based on object/scalar type) can be applied to methods of ArrayCollection ike
contains()
andremoveElement()
. Those methods currently do not provide the correct behaviour for Value Objects. But this is risky to change, because sometimes clients might want to only check the references. Perhaps it is an idea to introduce an optional boolean$strict
as an argument for those functions?Let comparing Value Objects remain impossible. Instead, let the client code compare the scalar properties of a Value Object. This feature is already discussed in DDC-3016 Support using embeddables in criterias with ArrayCollection #27.
However, this forces anyone using Value Objects to make the properties accessible through getters. It should not be neccessary for Value Objects that they expose their inner value. So I do not consider this a satisfiable solution.
My proposed solution is to introduce another operator that performs a loose comparison. I introduced a method
sim()
(i.e., similarity) with operator~
.This is the safest option, because it cannot break any client code.
I included 4 unit tests:
2 tests that cover the current behaviour of
eq()
andneg()
for objects (reference checking). There was no test for that behaviour yet and I think it's important to make it explicit.2 tests for the new expressions
sim()
andnsim()
.Of course support to DQL should be added for sim() and nsim(). I think the DQL implementation should transform sim() to an AND of eq()s for each property of the Value Object. (This looks like the second solution listed above. For DQL this is possible, because when querying, it is always possible to compare the inner properties of a Value Object. In fact, the Embeddable does not even exist in the database.)
I look forward to hearing your thoughts on this matter. Do you think this can work? Or is there a better solution?