Fixed a situation where unnecessary sql UPDATE statements were issued fo...

[iBiryukov]

...r unchanged DateTime objects

computeChangeSet method in unitOfWork compares original and new data using 'identical' (===) operator. This works well, except for DateTime objects.

Two different DateTime objects that contain the same data, should not trigger an unnecessary UPDATE query.

I made changes to account for this case.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2725

We use Jira to track the state of pull requests and the versions they got
included in.

[Ocramius]

sorry for closing eagerly. Again, doctrine won't support hardcoded custom checks. see http://stackoverflow.com/questions/15486402/doctrine2-orm-does-not-save-changes-to-a-datetime-field/15488230#15488230

You will need for support of value objects to see this happening.

[iBiryukov]

Well, Doctrine clearly shows wrong behaviour. It's executing an update statement when no update statement is required.
If you don't like my solution, how about we open a bug and wait until somebody else comes up with something better? Or have a conversation about possible solutions here.

Thanks for the link. It describes yet another issue, related to the same line of code, but is not exactly the same, as what I was talking about.

[Ocramius]

The idea is to have comparators (just an idea so far) do the work. So far we didn't introduce them because they only solve edge cases and cause great performance overhead. I was working on something in my ChangeSet project, but it's currently on hold.

[AlexeyKosov]

So far we didn't introduce them because they only solve edge cases and cause great performance overhead.

Great performance overhead is generating redundant UPDATE queries which, obviously, take some time to execute.

[Ocramius]

Yes, but we can't fix it without a massive amount of overhead for the 99% use-case scenario.

[AlexeyKosov]

Ok, of course, it's up to you to make strategic decisions, but it could be solved without any massive overhead. 1 if condition for checking if the comparable items are both objects and have the same type and another 1 to check if there's a comparator (or however you call it) registered for that class. In other words, it could be implemented as an optional feature, i.e. if one wanted to enable it for, say, DateTime or Money objects, they could inject a comparator for the specific class which would be responsible for comparing objects of this type. All other comparisons could still be done using ===.
IMHO, a couple of simple checks are much less evil than executing tons of unnecessary update queries.

[Ocramius]

Yes, but that's just fixing a symptom that comes from the mutability/immutability dichotomy in column types. We don't have a performance-valid solution for that, and adding exceptions and edge case handling just clutters the code and introduces more bugs. If you want to improve this then I suggest looking into a way of unifying hydration and changeset computation with DBAL type awareness baked in. Hacks for edge cases are not allowed in any of the doctrine projects, unless it's about security issues.

…

On 3 Aug 2017 11:33 PM, "AlexeyKosov" ***@***.***> wrote: Ok, of course, it's up to you to make strategic decisions, but it could be solved without any massive overhead. 1 if condition for checking if the comparable items are both objects and have the same type and another 1 to check if there's a comparator (or however you call it) registered for that class. In other words, it could be implemented as an optional feature, i.e. if one wanted to enable it for, say, DateTime or Money objects, they could inject a comparator for the specific class which would be responsible for comparing objects of this type. All other comparisons could still be done using ===. IMHO, a couple of simple checks are much less evil than executing tons of unnecessary update queries. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#814 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakDIMolXZhiUUSLB2IAiLErrToy1Rks5sUjy6gaJpZM4BEaIw> .

[AlexeyKosov]

Your point makes sense and, indeed, the ideal solution would be comparing the values in the form they go to the database, rather than objects.
I was just concerned not about the mutability/immutability dichotomy, but about the case when a different object instance, however, having the same value, is set to an entity.

$entity->setCreatedAt(new DateTime('2017-08-04 00:00:00'));
$em->flush();
$entity->setCreatedAt(new DateTime('2017-08-04 00:00:00'));
$em->flush(); // unneccesary query

Currently, I have to wrap all setters with something like this:

public function setCreatedAt(DateTime $createdAt)
{
    if ($createdAt != $this->createdAt) {
        $this->createdAt = $createdAt;
    }
}

Obviously, looks like a dirty hack. It's great that you managed to avoid such hacks in Doctrine itself, and hopefully, at some point you'll manage to allow Doctrine's users get rid of those in application business-logic.

[alcaeus]

@AlexeyKosov this issue can't be solved - just because a !== b && a == b is true doesn't mean Doctrine should just not update it. This is part of your application logic: someone sets a new object instance but your application wants to treat it the same. That belongs exactly where you currently have it: in your model.