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

Check hint value before considering instance read-only #11768

Merged

Conversation

pbreteche
Copy link
Contributor

Documentation suggests that HINT_READ_ONLY accept a boolean value.
https://www.doctrine-project.org/projects/doctrine-orm/en/3.3/reference/improving-performance.html#read-only-entities

But UnitOfWork only check if hint is set to register new Entity instance as read-only.

Using the value parameter will code more consistent to documentation.

@greg0ire
Copy link
Member

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@greg0ire greg0ire added the Bug label Dec 16, 2024
@greg0ire
Copy link
Member

Does this affect 2.x? If yes, please retarget to 2.20.x, and we will merge it up. Also, it would be nice to have a test covering this, inside tests/Tests/ORM/Functional/ReadOnlyTest.php

@pbreteche pbreteche changed the base branch from 3.3.x to 2.21.x December 17, 2024 06:41
@pbreteche pbreteche force-pushed the HINT_READ_ONLY-use-its-boolean-value branch from bf316ed to 811f7d0 Compare December 17, 2024 07:14
@pbreteche pbreteche changed the base branch from 2.21.x to 2.20.x December 17, 2024 07:15
@pbreteche pbreteche force-pushed the HINT_READ_ONLY-use-its-boolean-value branch from 811f7d0 to 62a66ef Compare December 17, 2024 07:16
@pbreteche
Copy link
Contributor Author

@greg0ire thanks for reviewing. Retargeting is done with one unit test.

@greg0ire
Copy link
Member

Please improve your commit message according to the contributing guide.

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/2.20.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already pushed. Don't forget the --force option otherwise git will try to merge both things together.

This fixes a bug that occurs when calling setHint(Query::HINT_READ_ONLY, false) from a query object.
UnitOfWork checks if this hint exists without considering the value passed as second argument.
Handling the second parameter improves consistency with documentation.
https://www.doctrine-project.org/projects/doctrine-orm/en/2.20/reference/improving-performance.html#read-only-entities
@pbreteche pbreteche force-pushed the HINT_READ_ONLY-use-its-boolean-value branch from ca1f6da to 4a9101f Compare December 18, 2024 13:55
@pbreteche
Copy link
Contributor Author

Thanks for your patience. I'm not used to contributing to open-source projects .
Hoping this commit message is more meaningful.

@greg0ire greg0ire modified the milestone: 2.20.1 Dec 18, 2024
@greg0ire
Copy link
Member

And thanks for bearing with me, the new commit message is 👌

@greg0ire greg0ire added this to the 2.20.1 milestone Dec 19, 2024
@greg0ire greg0ire merged commit e3cabad into doctrine:2.20.x Dec 19, 2024
74 checks passed
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.

4 participants