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

Infer type from field instead of column #9153

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Conversation

armenio
Copy link
Contributor

@armenio armenio commented Oct 28, 2021

getTypeOfColumn() relies on getTypeOfField(), and does not suffer from
mismatching issues caused by quoting, because you cannot quote a field.
Since a field can be composite, that method returns an array, hence why we
need to select the first element.

Fixes #9109 caused by #9010

@armenio
Copy link
Contributor Author

armenio commented Oct 31, 2021

need labels: "Bug" and "Failing Test"

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2021

In attempt to resolve #9109

Please improve your commit message according to the contributing guide.

Specifically, the commit message should explain what did not work before exactly, and why it works now. Here is the commit message I would reverse-engineer after reading your PR:

Infer type from field instead of column

getTypeOfColumn() relies on getTypeOfField(), and does not suffer from
mismatching issues caused by quoting, because you cannot quote a field.
Since a field can be composite, that method returns an array, hence why we
need to select the first element.

Fixes #9109

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2021

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.10.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. For the first line, use r, so that you get a chance to re-use the commit message in Infer type from field instead of column #9153 (comment)
  4. Close your editor, git should do its magic, and you should end up with one commit
  5. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@armenio armenio changed the title GH-9109 - ArrayCollection->matching() not working anymore with Criteria Infer type from field instead of column Nov 5, 2021
@armenio
Copy link
Contributor Author

armenio commented Nov 5, 2021

Commits are squashed

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2021

Please use git commit --amend to drop the In attempt to resolve #9109 part of your commit message.

@armenio
Copy link
Contributor Author

armenio commented Nov 5, 2021

done.

greg0ire
greg0ire previously approved these changes Nov 5, 2021
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.

As a related but separate issue from the issue at hand, I think we should either document that the $columnName argument is supposed to be quoted if that's indeed the intended usage, or improve that method to make it work regardless of whether it is quoted or not.

@armenio
Copy link
Contributor Author

armenio commented Nov 5, 2021

Thank you very much for your effort @greg0ire

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2021

@armenio you're welcome! I notice there is an issue with PostgreSQL though, can you please look into it?

getTypeOfColumn() relies on getTypeOfField(), and does not suffer from
mismatching issues caused by quoting, because you cannot quote a field.
Since a field can be composite, that method returns an array, hence why we
need to select the first element.
@armenio
Copy link
Contributor Author

armenio commented Nov 5, 2021

Fixed PgSQL error with changing strategy to "AUTO" in @GeneratedValue of PK's

@beberlei
Copy link
Member

beberlei commented Nov 7, 2021

@greg0ire This should maybe even target a new 2.9.7 or? It was merged to 2.9.6 as far as i see breaking a core function.

@beberlei beberlei added this to the 2.10.3 milestone Nov 7, 2021
@greg0ire
Copy link
Member

greg0ire commented Nov 7, 2021

@beberlei 2.9.x is no longer supported, and upgrading to 2.10.x does not seem like a huge deal so I would backport it iff someone complains they cannot upgrade. Otherwise, it's a good incentive to get people to upgrade IMO 🙂

@greg0ire greg0ire merged commit 8336420 into doctrine:2.10.x Nov 8, 2021
@greg0ire
Copy link
Member

greg0ire commented Nov 8, 2021

Thanks @armenio !

@beberlei
Copy link
Member

beberlei commented Nov 8, 2021

@greg0ire I guess another suggestion could be to downgrade to 2.9.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArrayCollection->matching() not working anymore with Criteria->eq('id', $number)
4 participants