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

Tagirijus · 2021-10-07T17:10:04Z

Bug Report

Q	A
BC Break	yes
Version	2.9.6

Summary

It seems that something changed with the ->matching() method of an ArrayCollection. It persist in v2.10.1.

Current behavior

When I have an ArrayCollection I want to filter with matching and a Criteria, which has an expression like ->eq('id', $number) my script tells me something like:

"Could not resolve type of column "`id`" of class ..." and so on.

How to reproduce

Let's say you have an entity "work" with an ArrayCollection called "connectedalbums", which stores other entities like "album". I now want to access the album with the id "3" and would do it like this:

// $work is already fetched form the DB.

$criteria = new Criteria();
$criteria->where(Criteria::expr()->eq('id', 3));
$album = $work->getConnectedAlbums()->matching($criteria)->toArray();

if (!empty($album)) {
    $album = $album[0];
}

With this, PHP throws an Uncaught RuntimeException like mentioned above.

Expected behavior

I expect an ArrayCollection with ->matching() and the Criteria with the expression ->eq('id', $number) to give me the correct item from the ArrayCollection, whichs column id has the value, which is stored in $number. It worked like this in v2.9.5!

The text was updated successfully, but these errors were encountered:

Tagirijus · 2021-10-10T07:10:10Z

Someone commented here and removed the comment again (probably because it did not work for them neither), but I still tried the idea. The idea was to remove the vendor folder and re-install with composer again: it did not work here. Thanks for the idea, though!

Nek- · 2021-10-16T22:37:04Z

@greg0ire @Tagirijus isn't this fixed?

Tagirijus · 2021-10-18T09:58:45Z

As I wrote: "it persist in v2.10.1". (-;

Nek- · 2021-10-22T15:42:14Z

Hello, I tried a reproducer (in a symfony app, but still): https://gist.github.com/Nek-/b799f16acc1ff73833dd2f2a1d634848

It's working fine with Doctrine ORM 2.10.2.

greg0ire · 2021-10-22T17:17:49Z

@Nek- Hi mate! Is your gist missing a docker-compose.yml?

EDIT: ah no, it's just missing a cd doctrine-matching-test

And then the patch cannot be applied, because of how fast the ecosystem evolves: patching composer.lock won't work I'm afraid :p

Nek- · 2021-10-24T22:01:05Z

Fixed, nvm it just exposes that it's working fine and this issue should be closed to me 😉 .

Tagirijus · 2021-10-25T11:22:21Z

Well, sorry but it does still persist in v2.10.2 here. I also removed the vendor folder and did composer install again... did not help. )=

Edit: when I find some time, maybe I can dig deeper into this and try to understand on how to make some tests, in case my code example in my intitial post does not help. Sorry for not being able to give more information right now.

One information I found out, while I am not sure, if it is important: when downgrading again to work in v2.9.5 (since it works here), composer also shows that it is downgrading doctrine/dbal v3.1.3 to v2.13.4 and removes symfony/polyfill-php72 v1.23.0.

Oh an another info: I do not work with symfony at all. I use Siler as a framework underneath ( https://github.com/leocavalcante/siler ).

Nek- · 2021-10-25T12:45:03Z

@Tagirijus please provide a reproducer. I took some personal time to try to reproduce and fix your issue but instead I ended up showing that it's working fine.

armenio · 2021-10-25T12:48:35Z

I am using DoctrineOrmModule, running composer update doesn’t solve the problem

armenio · 2021-10-28T00:30:08Z

@Nek- @greg0ire @Tagirijus
I really couldn't reproduce this in a test case.
but I found the possible bug.

It should be here: https://github.com/doctrine/orm/blob/2.10.x/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php#L252

to contextualize, this happens before the bug:

[$name, $value, $operator] = $parameter;

$field = $this->quoteStrategy->getColumnName($name, $targetClass, $this->platform);

so here is the problem:

$paramTypes[] = PersisterHelper::getTypeOfColumn($field, $targetClass, $this->em);

$field needs to be replaced with $name

$paramTypes[] = PersisterHelper::getTypeOfColumn($name, $targetClass, $this->em);

Test results after that change:

Time: 01:15.521, Memory: 442.50 MB

OK, but incomplete, skipped, or risky tests!
Tests: 3600, Assertions: 13052, Skipped: 109, Incomplete: 2.

pay attention to merge with "weak" tests involving this: #9010

Nek- · 2021-10-28T07:28:16Z

@armenio without making a testcase to reproduce, can't you reproduce your issue in a standard app? (no matter the framework, just use yours to be the closest possible to your case).

…h Criteria->eq('id', $number)

armenio · 2021-10-28T11:06:24Z

@Nek- See: #9153

armenio · 2021-11-04T01:08:22Z

can you reopen?

Tagirijus · 2021-11-04T07:32:25Z

It would be okay to me, if this one only gets re-opened, when I finally come up with some clear reproducible tests, hehe. I do not find time for this at the moment, though. Sorry. ))=

armenio · 2021-11-04T13:01:33Z

It would be okay to me, if this one only gets re-opened, when I finally come up with some clear reproducible tests, hehe. I do not find time for this at the moment, though. Sorry. ))=

I already created the test case forcing the error, see: #9153
@greg0ire @Nek- what else do i need to do?

Tagirijus · 2021-11-04T13:36:32Z

Could not test this either. But still thanks for your effort! (=

armenio · 2021-11-04T14:49:51Z

Could not test this either. But still thanks for your effort! (=

what can i do to help even more?

Tagirijus · 2021-11-04T14:51:40Z

Get me some more free time so that I finally could test your posted test case on my own, haha. :D (-;

greg0ire · 2021-11-04T16:15:41Z

@armenio assuming this used to work in the past, you could use git bisect to find the commit that breaks your test.

armenio · 2021-11-04T20:02:44Z

@armenio assuming this used to work in the past, you could use git bisect to find the commit that breaks your test.

@greg0ire I found!
What is the next step? I don't know the flow. please let me know what i need to know
here is the result of git bisect:

armenio@MacBook-Pro doctrine-orm % git bisect bad                                                                                                    
128ebe630b14e9db45efdede6b79847ba5920bdf is the first bad commit
commit 128ebe630b14e9db45efdede6b79847ba5920bdf
Author: Laszlo_Csupity <laszlo_csupity@epam.com>
Date:   Mon Sep 13 13:55:41 2021 +0200

    Use types in collection persister

 lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Screenshot:

greg0ire · 2021-11-04T20:24:41Z

Next steps:

find that commit on Github;
find the pull request (appears on the commit page);
let the author know about this issue.

armenio · 2021-11-04T20:38:45Z

Next steps:

find that commit on Github;

find the pull request (appears on the commit page);

let the author know about this issue.

Thanks @greg0ire! Just reported at #9010

greg0ire · 2021-11-04T21:39:16Z

pay attention to merge with "weak" tests involving this: #9010

Oh wow hold on a minute… do you mean to say you would have spotted the issue had you reviewed that PR?

armenio · 2021-11-04T22:44:40Z

pay attention to merge with "weak" tests involving this: #9010

Oh wow hold on a minute… do you mean to say you would have spotted the issue had you reviewed that PR?

Maybe, why not? I did this to get your attention that there really is something wrong there.
I have the bug "apparently" fixed here locally.
Can I push it in my PR #9153 for you to review?
Thanks!

greg0ire · 2021-11-04T22:57:03Z

Can I push it in my PR #9153 for you to review?

What would be the point? You've demonstrated that I'm not good enough to review a PR.
Jokes aside, you can push the fix, but I personally won't be participating in this unless a sincere apology comes first.

armenio · 2021-11-04T23:11:34Z

What would be the point? You've demonstrated that I'm not good enough to review a PR. Jokes aside, you can push the fix, but I personally won't be participating in this unless a sincere apology comes first.

Please don't get me wrong. maybe it's because of my bad English. See the quotes in the word.
As I said, I tried to get your attention for the situation. I didn't necessarily mean you, of course.
Anyway, I sincerely apologize. Mainly for seeming rude.
I will push the attempt to fix. If you can, please review it when you can.
I thank you again

greg0ire · 2021-11-05T06:41:45Z

Not super convincing, but in doubt, I'll accept the apology and give this PR a review.

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 doctrine#9109

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 doctrine#9109

armenio mentioned this issue Oct 14, 2021

Fix ignoring custom types for PersistentCollection matching() #9010

Merged

Ocramius added Bug Missing Tests labels Oct 19, 2021

greg0ire closed this as completed Oct 25, 2021

armenio mentioned this issue Oct 28, 2021

Infer type from field instead of column #9153

Merged

greg0ire reopened this Nov 4, 2021

greg0ire closed this as completed in #9153 Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Tagirijus commented Oct 7, 2021

Tagirijus commented Oct 10, 2021

Nek- commented Oct 16, 2021

Tagirijus commented Oct 18, 2021

Nek- commented Oct 22, 2021 •

edited

Loading

greg0ire commented Oct 22, 2021 •

edited

Loading

Nek- commented Oct 24, 2021 •

edited

Loading

Tagirijus commented Oct 25, 2021 •

edited

Loading

Nek- commented Oct 25, 2021 •

edited

Loading

armenio commented Oct 25, 2021

armenio commented Oct 28, 2021

Nek- commented Oct 28, 2021 •

edited

Loading

armenio commented Oct 28, 2021

armenio commented Nov 4, 2021

Tagirijus commented Nov 4, 2021

armenio commented Nov 4, 2021 •

edited by greg0ire

Loading

Tagirijus commented Nov 4, 2021

armenio commented Nov 4, 2021

Tagirijus commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 5, 2021

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

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

Comments

Tagirijus commented Oct 7, 2021

Bug Report

Summary

Current behavior

How to reproduce

Expected behavior

Tagirijus commented Oct 10, 2021

Nek- commented Oct 16, 2021

Tagirijus commented Oct 18, 2021

Nek- commented Oct 22, 2021 • edited Loading

greg0ire commented Oct 22, 2021 • edited Loading

Nek- commented Oct 24, 2021 • edited Loading

Tagirijus commented Oct 25, 2021 • edited Loading

Nek- commented Oct 25, 2021 • edited Loading

armenio commented Oct 25, 2021

armenio commented Oct 28, 2021

Nek- commented Oct 28, 2021 • edited Loading

armenio commented Oct 28, 2021

armenio commented Nov 4, 2021

Tagirijus commented Nov 4, 2021

armenio commented Nov 4, 2021 • edited by greg0ire Loading

Tagirijus commented Nov 4, 2021

armenio commented Nov 4, 2021

Tagirijus commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 4, 2021

armenio commented Nov 4, 2021

greg0ire commented Nov 5, 2021

Nek- commented Oct 22, 2021 •

edited

Loading

greg0ire commented Oct 22, 2021 •

edited

Loading

Nek- commented Oct 24, 2021 •

edited

Loading

Tagirijus commented Oct 25, 2021 •

edited

Loading

Nek- commented Oct 25, 2021 •

edited

Loading

Nek- commented Oct 28, 2021 •

edited

Loading

armenio commented Nov 4, 2021 •

edited by greg0ire

Loading