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

Fix variadic args in Expr #8319

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Fix variadic args in Expr #8319

merged 1 commit into from
Jul 23, 2021

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Oct 26, 2020 •
edited by greg0ire
Loading

Closes #8537

@simPod simPod changed the base branch from 2.7 to 2.8.x October 26, 2020 07:04
@simPod simPod changed the title Fix args Fix variadic args in Expr Oct 26, 2020
@simPod simPod changed the base branch from 2.8.x to master October 26, 2020 21:10
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 20, 2021 07:13
@simPod simPod requested a review from greg0ire July 20, 2021 07:17
greg0ire
greg0ire reviewed Jul 20, 2021
View reviewed changes
lib/Doctrine/ORM/Query/Expr.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Query/Expr.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

Please document the BC-break in UPGRADE.md

greg0ire
greg0ire reviewed Jul 20, 2021
View reviewed changes
UPGRADE.md Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

This looks related to #8537 , there are a few more methods that could benefit from this change in lib/Doctrine/ORM/QueryBuilder.php, would you like to also take care of those in this PR?

@simPod
Copy link
Contributor Author

simPod commented Jul 20, 2021

Aight

@simPod simPod marked this pull request as draft July 20, 2021 19:40
@simPod simPod force-pushed the fix-args branch 2 times, most recently from 0dc0cd4 to c883de4 Compare July 21, 2021 06:52
@simPod
Copy link
Contributor Author

simPod commented Jul 21, 2021

There are also select(), addSelect(), where() and having() using func_get_args() in one path. Do you want them rewritten as well?

@simPod simPod marked this pull request as ready for review July 21, 2021 06:55
@greg0ire
Copy link
Member

Yes please! From what I see, that would be a really nice simplification, right?

@simPod simPod requested a review from greg0ire July 21, 2021 09:32
greg0ire
greg0ire previously approved these changes Jul 21, 2021
View reviewed changes
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.

The code looks good to me, let's rework the UPGRADE and squash?

UPGRADE.md Outdated Show resolved Hide resolved
@greg0ire greg0ire dismissed their stale review July 21, 2021 09:57

Didn't mean to approve

@simPod
Copy link
Contributor Author

simPod commented Jul 21, 2021

Done, thank you

greg0ire
greg0ire previously approved these changes Jul 21, 2021
View reviewed changes
@simPod simPod force-pushed the fix-args branch 2 times, most recently from 048afc0 to 7dec17e Compare July 23, 2021 08:34
@greg0ire greg0ire merged commit 520bfa5 into doctrine:3.0.x Jul 23, 2021
@greg0ire
Copy link
Member

Thanks @simPod !

@greg0ire greg0ire added this to the 3.0.0 milestone Jul 23, 2021
@simPod simPod deleted the fix-args branch August 1, 2021 09:23
@greg0ire greg0ire mentioned this pull request Jul 18, 2022
Closed
@greg0ire greg0ire mentioned this pull request Sep 7, 2023
Closed
@simPod simPod mentioned this pull request Feb 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes left review comments

@SenseException SenseException SenseException approved these changes

@greg0ire greg0ire greg0ire approved these changes

@beberlei beberlei Awaiting requested review from beberlei

Assignees
No one assigned
Labels
BC Break
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@simPod @greg0ire @ondrejmirtes @SenseException