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.

Sign up for GitHub

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

Jump to bottom

Introduce EntityManagerInterface#wrapInTransaction() #8419

Merged
merged 1 commit into from
Aug 8, 2021
Merged

Introduce EntityManagerInterface#wrapInTransaction() #8419

merged 1 commit into from
Aug 8, 2021

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 9, 2021 •
edited
Loading

This introduces Doctrine\ORM\EntityManagerInterface#wrapInTransaction() so we can return any value from Transaction.

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:19
@simPod simPod changed the base branch from old-prototype-3.x to 3.0.x July 17, 2021 12:55
@simPod simPod force-pushed the more-tests branch 3 times, most recently from d0bcde1 to 08226ca Compare July 17, 2021 13:14
@simPod simPod changed the title Add more values to dataToBeReturnedByTransactional Fix incorrect value conversion in transactional() Jul 17, 2021
@greg0ire
Copy link
Member

I believe I've seen this bug fixed on v3 like half a year ago. Now I've found this PR stale so I just wanted to update the test and I've found the fix is not there anymore. I can't tell what happened here, branches are shuffled etc.

The v3 you know has been renamed to old-prototype-3.x, and we are working on backporting some of the PRs here: https://github.com/doctrine/orm/projects/4

The difference this time is we are going to regularly merge up 2.x into 3.x .

We may backport more, feel free to find the commits in old-prototype-3.x and backport them 👍

@greg0ire
Copy link
Member

Is there a BC-break in this PR? If not, please target 2.10.x instead.

@greg0ire
Copy link
Member

Or even 2.9.x since it is a bugfix

@simPod
Copy link
Contributor Author

simPod commented Jul 17, 2021

Thanks for explaining, I'll revert this and target theold-prototype. This was initially only about tests for the bug.

@simPod simPod changed the base branch from 3.0.x to old-prototype-3.x July 17, 2021 13:22
@simPod simPod changed the title Fix incorrect value conversion in transactional() Add more types to TransactionalAcceptsReturn test Jul 17, 2021
@simPod
Copy link
Contributor Author

simPod commented Jul 17, 2021

@greg0ire I suppose CI is broken on old-prototype, right?

@greg0ire
Copy link
Member

Yes, it's no longer maintained, it's just a source of commits to port to maintained branches. Why are you targeting it? Does the bug only happen there?

@simPod
Copy link
Contributor Author

simPod commented Jul 17, 2021

@greg0ire the bug is fixed only there

aa9f11e#diff-09261ffd6eb75f4fda9de6c2fbf156761380cf13687e57a199878630890402c4R232

the previous version (e.g. the whole 2.x orm) returns true from transactional() when inner closure returns anything falsy ([], 0, false) instead of the falsy value.

@greg0ire
Copy link
Member

Could you please work on porting the new behavior to 3.x instead? Otherwise it's just more porting work for us in the future

@simPod
Copy link
Contributor Author

simPod commented Jul 17, 2021

ah ok, I have just reverted that few h ago. Np. Gimme sec.

@simPod simPod changed the base branch from old-prototype-3.x to 3.0.x July 17, 2021 15:51
@simPod simPod changed the title Add more types to TransactionalAcceptsReturn test Fix incorrect value conversion in transactional() Jul 17, 2021
@simPod simPod changed the title Fix incorrect value conversion in transactional() Fix incorrect value conversion in transactional() Jul 17, 2021
@simPod
Copy link
Contributor Author

simPod commented Jul 17, 2021

It's done, test failures are irrelevant https://github.com/simPod/orm/runs/3093712747.

@greg0ire
Copy link
Member

I think they relate to doctrine/cache#381

@greg0ire
Copy link
Member

Blocked by #8847 and 2 merges up

@greg0ire
Copy link
Member

Regarding the PR itself: why did non-empty value deserve a special treatment, and why does null deserve one?

@simPod simPod force-pushed the more-tests branch 2 times, most recently from 3f5a1af to 8c7e00b Compare August 1, 2021 09:57
@simPod simPod changed the title Introduce Doctrine\ORM\EntityManagerInterface#wrapInTransaction() Introduce EntityManagerInterface#wrapInTransaction() Aug 1, 2021
@simPod simPod marked this pull request as ready for review August 1, 2021 09:58
@simPod simPod requested a review from greg0ire August 1, 2021 09:59
@greg0ire
Copy link
Member

greg0ire commented Aug 4, 2021

Please fix the conflict 🙏

Note: this PR kind of backports #6147

@simPod
Copy link
Contributor Author

simPod commented Aug 4, 2021 •
edited
Loading

done

Note: this PR kind of backports #6147

I believe I've seen it multiple times here so initially I only wanted to add test cases for it then I§ve found out it's not implemented 🤷🏾

@greg0ire
Copy link
Member

greg0ire commented Aug 4, 2021

Hence the branch name!

greg0ire
greg0ire previously approved these changes Aug 4, 2021
View reviewed changes
@SenseException
Copy link
Member

The extended interface is a BC break. @greg0ire Is this considered as a "smaller" one?

@greg0ire greg0ire dismissed their stale review August 4, 2021 22:15

No, you are right, I didn't spot it. Not sure how to make it BC…

@simPod simPod requested a review from greg0ire August 8, 2021 14:25
@greg0ire greg0ire requested a review from SenseException August 8, 2021 15:35
@greg0ire greg0ire merged commit b600c01 into doctrine:2.10.x Aug 8, 2021
@greg0ire greg0ire added this to the 2.10.0 milestone Aug 8, 2021
@greg0ire
Copy link
Member

greg0ire commented Aug 8, 2021

Thanks @simPod !

@simPod simPod deleted the more-tests branch August 9, 2021 05:43
@github-actions github-actions bot mentioned this pull request Oct 3, 2021
Merged
@derrabus derrabus mentioned this pull request Dec 21, 2021
Merged
@greg0ire greg0ire mentioned this pull request Apr 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zoltanka zoltanka zoltanka left review comments

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@simPod @greg0ire @beberlei @SenseException @zoltanka