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

Manual merge of 3.0.x into master #4026

Merged
merged 46 commits into from
May 31, 2020
Merged

Manual merge of 3.0.x into master #4026

merged 46 commits into from
May 31, 2020

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire mentioned this pull request May 24, 2020
16 tasks
@greg0ire greg0ire requested a review from morozov May 25, 2020 07:23
@morozov
Copy link
Member

morozov commented May 25, 2020

Why do we need to merge two changes at once?

@greg0ire
Copy link
Member Author

greg0ire commented May 25, 2020

What do you mean by "2 changes"? I mean to merge 4 PRs at once. If you are referring to the 4 PRs, I feel safer pushing something and knowing for a fact there will be no up merge problems, which is guaranteed by the green builds and the --atomic flag.

@greg0ire
Copy link
Member Author

@morozov only the 7 last commits are new and need to be reviewed

morozov and others added 24 commits May 26, 2020 22:07
Ignore all violations of the LowercasePHPFunctions sniff in SQLSrvStatement
The latest version is easier to understand for static analyzers.
It is more simple and more accurate
We are testing what happens when providing the wrong type.
The method signature does not allow null.
Not all RDBMS have a concept of database, apparently (see SQLAnywhere).
Those who do not should pass in a string to stay compatible with the
signature.
That alone fixes 6 issues found by Psalm
This is what is expected with FETCH_COLUMN
@greg0ire
Copy link
Member Author

@morozov it's complicated to do the merge up with your changes, I'm not super sure. Can you please do the merge up of 74eca6b ? That way I only have my part to take care about…

@morozov
Copy link
Member

morozov commented May 28, 2020

@greg0ire not sure I understand the ask. 74eca6b is currently part of 3.0.x. Are you asking me to merge 3.0.x into master? Looks like this is what you're doing in this PR.

As for the change itself, it looks trivial: the method is moved from one interface to another and is implemented in the class which now needs to implement it. The same change already exists in master where it was backported from.

Please let me know how exactly I can help you.

@greg0ire
Copy link
Member Author

@greg0ire not sure I understand the ask. 74eca6b is currently part of 3.0.x. Are you asking me to merge 3.0.x into master? Looks like this is what you're doing in this PR.

No I'm just asking you to merge 74eca6b into master. You don't have to handle the merge for my Psalm related changes, but only for your changes.

As for the change itself, it looks trivial: the method is moved from one interface to another and is implemented in the class which now needs to implement it. The same change already exists in master where it was backported from.

Please let me know how exactly I can help you.

I'll give it another try since it does look trivial, but 74eca6b might have ancestors that are not merged either, not sure if the conflict was about them or 74eca6b

@greg0ire
Copy link
Member Author

greg0ire commented May 29, 2020

Gave it another try, and I'm still uncomfortable with this. Please try the following on master:

git merge 74eca6ba7fdd6a48af0c450a486d1f77f950a041
git push fork
* review*
git push origin

EDIT: here is a comparison: master...74eca6b
As you see, it's far from being just that one trivial commit 😉

@morozov
Copy link
Member

morozov commented May 29, 2020

@greg0ire please see #4038.

As you see, it's far from being just that one trivial commit

Yeah, its the fetch mode rework and the corresponding deprecation backports.

@greg0ire
Copy link
Member Author

Thank you!!!

@greg0ire greg0ire changed the title Manual merge of 3.0.x + #3977 into master Manual merge of 3.0.x into master May 30, 2020
@greg0ire
Copy link
Member Author

greg0ire commented May 30, 2020

@morozov ready for review (the last 5 commits are only on this branch)

@greg0ire greg0ire mentioned this pull request May 30, 2020
@greg0ire
Copy link
Member Author

@morozov can we merge as is? I handled your review in #4040, since it was already wrong on 3.0.x

morozov
morozov previously approved these changes May 30, 2020
@greg0ire
Copy link
Member Author

greg0ire commented May 31, 2020

@morozov sorry, please approve again, the last push contained the fixes that were intended for #4040 and missed the last commit without which I don't understand how the build could pass…

@greg0ire greg0ire merged commit 66e9dc3 into doctrine:master May 31, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants