orm icon indicating copy to clipboard operation
orm copied to clipboard

Discriminator value could be an integer

Open prohalexey opened this issue 1 year ago • 3 comments

The same as

399:    $values[] = $conn->quote((string) $subclassMetadata->discriminatorValue);

Without casting a TypeError is raised.

prohalexey avatar Apr 25 '24 15:04 prohalexey

The change on line 399 was introduced in a merge up: 5a40b99e11a0de532eb866f062dccad20da19add :thinking:

greg0ire avatar Apr 28 '24 15:04 greg0ire

@prohalexey Does this PR fix https://github.com/doctrine/orm/issues/11341?

Does it make sense to add the unit tests which are suggested in this issue?

W0rma avatar Apr 29 '24 06:04 W0rma

Does it make sense to add the unit tests which are suggested in this issue?

Yes, please do :pray:

greg0ire avatar Apr 29 '24 07:04 greg0ire

Any news here @greg0ire @W0rma ?

prohalexey avatar May 07 '24 08:05 prohalexey

@prohalexey can you please incorporate the unit tests suggested in #11341 ?

greg0ire avatar May 07 '24 08:05 greg0ire

Tests have been added and the code has been changed to a more accurate version. @greg0ire pls, look

prohalexey avatar May 07 '24 13:05 prohalexey

@greg0ire @SenseException @derrabus could someone check this pr ?

prohalexey avatar May 10 '24 13:05 prohalexey

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/3.1.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. Close your editor, git should do its magic, and you should end up with one commit
  4. 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.

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.

greg0ire avatar May 13 '24 20:05 greg0ire

@greg0ire I think I'm done.

prohalexey avatar May 15 '24 07:05 prohalexey

Thanks @prohalexey !

greg0ire avatar May 15 '24 09:05 greg0ire

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?

yakijavier avatar May 15 '24 12:05 yakijavier

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?

greg0ire avatar May 15 '24 13:05 greg0ire

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 getChildDiscriminatorsFromClassMetadata

prohalexey avatar May 15 '24 13:05 prohalexey

Yep, I confirm that there is a bug if you make DQL query with INSTANCEOF function. I will create a new PR. @greg0ire

prohalexey avatar May 15 '24 14:05 prohalexey

New PR https://github.com/doctrine/orm/pull/11456

prohalexey avatar May 15 '24 14:05 prohalexey