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

Proposed fix for #8499 #8508

Merged
merged 1 commit into from Feb 27, 2021
Merged

Proposed fix for #8499 #8508

merged 1 commit into from Feb 27, 2021

Conversation

diego-ninja
Copy link
Contributor

@diego-ninja diego-ninja commented Feb 23, 2021

Changed lock function to compare timestamps instead of DateTimeInterface objects directly.

When using optimistic lock with DateTimeInterface based version field a bug appears due to the use of the === operator for comparing the lock version and the entity version. This comparison always resolves to false because the === operator when comparing objects is only true when both sides are the exact same instance of the object.

To fix the issue I have decided to compare timestamps instead the DateTimeInterface based objects directly, calling getTimestamp() method and doing a strict comparison.

Modified OptimisticLockException to use DateTimeInterface instead of DateTime class.

Added test suite to cover case.

Fixes #8499

@greg0ire
Copy link
Member

Please improve your commit message according to the contributing guide.

@diego-ninja
Copy link
Contributor Author

Please improve your commit message according to the contributing guide.

Done.

@greg0ire
Copy link
Member

I meant the commit message, so you can squash your current commits together, and reuse your PR message as a commit message. BTW, when you create a PR with just one commit, Github will reuse its message as a PR message.

@diego-ninja
Copy link
Contributor Author

I meant the commit message, so you can squash your current commits together, and reuse your PR message as a commit message. BTW, when you create a PR with just one commit, Github will reuse its message as a PR message.

I hope it's ok now.

@greg0ire
Copy link
Member

I will do it for you.

@greg0ire greg0ire added the Bug label Feb 24, 2021
@greg0ire
Copy link
Member

DDC- is a prefix that was used when we were using JIRA a long time ago, I will replace it with GH. Sorry for breaking this BTW, that was due to a CS-fixing commit of mine 🙇

@greg0ire greg0ire changed the title Proposed fix for DDC-8499 Proposed fix for #8499 Feb 24, 2021
@greg0ire
Copy link
Member

greg0ire commented Feb 24, 2021

🤔 is this line wrong:

* @param int|null $lockVersion
? It does not mention \DateTimeInterface although the docs do mention using a datetime field as a revision: https://www.doctrine-project.org/projects/doctrine-orm/en/latest/reference/transactions-and-concurrency.html#locking-support

This line might be wrong too:

* @param int $lockVersion

…ace objects directly.

When using optimistic lock with DateTimeInterface based version field a bug appears due to the use of the === operator for comparing the lock version and the entity version. This comparison always resolves to false because the === operator when comparing objects is only true when both sides are the exact same instance of the object.

To fix the issue I have decided to compare timestamps instead the DateTimeInterface based objects directly, calling getTimestamp() method and doing a strict comparison.

Modified OptimisticLockException to use DateTimeInterface instead of DateTime class.

Added test suite to cover case.

Fixes #8499
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

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

Successfully merging this pull request may close these issues.

Optimistic lock not working when used with DateTime objects.
3 participants