-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Discriminator value could be an integer #11425
Discriminator value could be an integer #11425
Conversation
The change on line 399 was introduced in a merge up: 5a40b99 🤔 |
@prohalexey Does this PR fix #11341? Does it make sense to add the unit tests which are suggested in this issue? |
Yes, please do 🙏 |
@prohalexey can you please incorporate the unit tests suggested in #11341 ? |
Tests have been added and the code has been changed to a more accurate version. @greg0ire pls, look |
@greg0ire @SenseException @derrabus could someone check this pr ? |
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?
Also, once you have a single commit, please improve your commit message according to the contributing guide. "Discriminator value could be an integer (TypeError is raised)" is not enough IMO. |
This fixes a bug that occurred when configuring integers as discriminator values. Doctrine throws a type error whenever the application generates queries.
381e605
to
e83d8a8
Compare
@greg0ire I think I'm done. |
Thanks @prohalexey ! |
Hello @greg0ire , if it is supposed to be possible to use an integer as a descriminator, in the SqlWalker.php file in line 2255 shouldn't the discriminator be cast to string so that quote() accepts it? |
I think so yeah… my IDE prints a warning, but I'm not sure why static analysis does not report it. @prohalexey what do you think? |
I ran tests(GH11341Test, GH11199Test) and did not stop at this line (I set breakpoint at 2255). Trying to understand what conditions we must have to call this function |
Yep, I confirm that there is a bug if you make DQL query with INSTANCEOF function. I will create a new PR. @greg0ire |
New PR #11456 |
The same as
Without casting a TypeError is raised.