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

Add test coverage for passing optimistic lock version as string #8533

Merged
merged 1 commit into from Mar 11, 2021

Conversation

acoulton
Copy link
Contributor

As suggested by @greg0ire I have updated the tests to cover the current behaviour of passing a string version argument for optimistic locking.

Since #8527 was only-sort-of-a-bug and this is just a simple variation of how ->lock is already tested, I thought it made sense to modify the existing tests to cover this case rather than the overhead of a whole separate test case. Particularly since I guess this will probably change again in a future release, and the goal is just to avoid that happening accidentally in the meantime.

If you'd prefer to have a standalone GH8527 ticket test instead I can happily do that, just let me know.

As discussed in doctrine#8527, when using optimistic locking with integer
version columns, Doctrine has always supported passing the lock
version as a string. For example when passing in a version
received in POST / GET.

Technically speaking this does not comply with the docs and phdoc
(which show the app explicitly casting to int before passing).

Nonetheless the maintainers decided it should continue to be valid
for now and reinstated the old soft-equals logic with doctrine#8531.

This modified test just avoids accidental changes in future.
@greg0ire greg0ire added this to the 2.8.3 milestone Mar 11, 2021
@greg0ire greg0ire merged commit 93f31d2 into doctrine:2.8.x Mar 11, 2021
@greg0ire
Copy link
Member

Thanks @acoulton !

@acoulton acoulton deleted the test-string-lock-version branch March 11, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants