Add support for ManyToMany Criteria #885

bakura10 · 2013-12-22T15:28:11Z

This replaces #773

It adds support for ManyToMany but also do an efficient filtering instead of loading the whole collection. I think it can also benefits from #882 when this one get merged.

doctrinebot · 2013-12-22T15:28:16Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2868

We use Jira to track the state of pull requests and the versions they got
included in.

guilhermeblanco · 2013-12-22T15:45:33Z

@bakura10 tests! =)

bakura10 · 2013-12-22T15:46:08Z

Yep yep. I'm just not sure of the approach yet, and I've found a bug :(.

bakura10 · 2013-12-22T16:14:55Z

I've fixed the logic, however I have no idea about how to test this. Actually, there is no test for most persisters :(.

stof · 2013-12-23T19:27:31Z

lib/Doctrine/ORM/PersistentCollection.php

-            throw new \RuntimeException("Matching Criteria on PersistentCollection only works on OneToMany associations at the moment.");
+        if ($this->association['type'] !== ClassMetadata::ONE_TO_MANY
+            && $this->association['type'] !== ClassMetadata::MANY_TO_MANY) {
+            throw new \RuntimeException("Matching Criteria on PersistentCollection only works on OneToMany and ManyToMany associations at the moment.");


this means it works for all PersistentCollection, as there are the only 2 types of toMany relations (and a toOne relation is not a collection). You should simply remove the exception

beberlei · 2014-01-02T21:23:40Z

Please don't do multiple things at once. The lazy thing sounds very complicated, support for ManyToMany to start with would be awesome all alone.

bakura10 · 2014-01-02T21:33:30Z

I think this PR does only one thing no? (Actually the other PR is MUCH more needed than this one :))

bakura10 · 2014-01-03T08:34:55Z

Tests no longer should break!

beberlei · 2014-01-03T21:20:47Z

Still breaks, plesae run the tests locally, just call phpunit

bakura10 · 2014-01-04T18:00:15Z

I'm not sure to understand, all tests pass without any problem locally :/.

bakura10 · 2014-01-04T18:01:46Z

Furthermore the failing tests on Travis have nothing to do with my changes :/.

bakura10 · 2014-01-05T11:11:25Z

Some errors I had locally then were because of an older DBAL. I leo updated DBAL and now all tests pass locally, so I don't really understand.

EDIT : this is actually passing, it seems there was an error with one stack on Travis.

bakura10 · 2014-01-19T14:20:00Z

I really really need this. What do you want me to do in order to merge? :)

Ocramius · 2014-01-20T05:27:52Z

lib/Doctrine/ORM/PersistentCollection.php

-            throw new \RuntimeException("Matching Criteria on PersistentCollection only works on OneToMany associations at the moment.");
+        if ($this->association['type'] === ClassMetadata::MANY_TO_MANY) {
+            $persister = $this->em->getUnitOfWork()->getCollectionPersister($this->association);
+            return new ArrayCollection($persister->loadCriteria($this, $criteria));


Add an empty newline before this line

Ocramius · 2014-01-20T05:31:22Z

@bakura10 a rebase is needed, plus I'm not sure about what kind of coverage you get with the current tests...

bakura10 · 2014-01-20T09:08:22Z

Hi,

I've fixed the CS and added a functional test. However, it fails! In fact, the SQL query that is executed is perfectly right, however the hydration does not work. I'm not confident enough about how ResultSetMapping and hydrators work, so a bit of help would be appreciated. I think this is the problem: https://github.com/doctrine/doctrine2/pull/885/files#diff-982b7374bbe9d5f4b6b71f4869a446eaR512

aeneasr · 2014-01-29T19:49:24Z

👍 on this, super useful

bakura10 · 2014-02-02T17:16:37Z

Anyone can help me on the issue I'm having? Sorry for being a bit persistent on this one but this is really useful !

Ocramius · 2014-02-02T17:22:49Z

@FabioBatSilva do you know if this clashes with SLC?

bakura10 · 2014-02-09T10:39:17Z

Hi,

I've been doing more debugging. I can confirm that the SQL queries is generated correctly, and manually calling "fetchAll" from the prepared statement correctly returns the wanted rows.

Where it fails is indeed in the hydrator. The method "gatherRowData" from ObjectHydrator returns an empty array. I'm not sure why, but when I compared with other results, the query returns result like this:

array(4) {
  'id' =>
  string(1) "1"
  'name' =>
  string(12) "Developers_0"
  'user_id' =>
  string(1) "1"
  'group_id' =>
  string(1) "1"
}

This contains both the columns from the table and from the join table, but none of the columns are suffixed by an number (like "id0", "name1"...) and I suppose the hydrator must reuse this info to map those things to objects using the ResultSetMapping.

Any idea @beberlei ?

bakura10 · 2014-02-09T10:58:03Z

I've finally made it by using the ResultSetMappingBuilder!!!

Could someone have a look at the approach (performance wise)? Only one test is failing locally but it seems it has nothing to do with my PR.

bakura10 · 2014-02-13T22:21:26Z

@Ocramius could you give this one a quick review please? To me it's ready to be merged :).

deeky666 · 2014-02-13T22:30:27Z

lib/Doctrine/ORM/Persisters/CollectionPersister.php

+     *
+     * @return array
+     */
+    public function loadCriteria(PersistentCollection $collection, Criteria $criteria);


@bakura10 Shouldn't the contract here be to throw an exception if the operation is unsupported by the implementing collection persister? IMO that should be documented here as you already do throw an exception in AbstractCollectionPersister. What do you think?

I mimic the other methods. All the other methods (slice, containsKey...) throw an exception in the AbstractCollectionPersister but it's not in the interface. I just followed the same pattern, if I add it here I should also add it for all other methods then :).

@bakura10 Okay I didn't know that. Then it's perfectly fine to me, no need to go a different way here then :)

bakura10 · 2014-02-18T09:27:02Z

Rebased @beberlei

bakura10 · 2014-02-20T11:45:15Z

Tests are all passing, only one just did not execute. Can someone relaunch tests so that Travis goes green? :)

bakura10 · 2014-03-16T10:52:16Z

Hi,

It seems someone relaunched the tests, and everything is passing now. What is preventing this to being merged, so that I can eventually work on it quickly?

aeneasr · 2014-03-16T11:01:42Z

👍

Add support for ManyToMany Criteria

bakura10 · 2014-03-16T16:38:21Z

Awesome, thanks!

stof reviewed Dec 23, 2013
View reviewed changes

bakura10 mentioned this pull request Dec 30, 2013

Cookbook Doctrine ORM: Criteria on ManyToMany not supported yet ZF-Commons/zfc-rbac#154

Closed

beberlei mentioned this pull request Jan 2, 2014

Support matching Criteria on PersistentCollection on ManyToMany associat... #773

Closed

This was referenced Jan 6, 2014

DoctrineModule\Form\Element\ObjectSelect criteria for a collection field doctrine/DoctrineModule#340

Closed

Owner-related permissions ZF-Commons/zfc-rbac#157

Closed

Ocramius reviewed Jan 20, 2014
View reviewed changes

deeky666 reviewed Feb 13, 2014
View reviewed changes

bakura10 added 15 commits February 18, 2014 10:26

Rebase to master

160b07d

Revert tab changes

07654dd

Revert docblock

738cc25

Fix wrong logic

9385a60

Add method to interface

be39afa

Fix docblock

bb1f71f

Remove useless exception

c69b756

Fix tests

fbbe922

fallback to persister count

e5ba286

Fix test

a55f2c4

Fix CS

0d5de64

Functional test

f03f991

Add test

6cbd643

Fix bug with hydrators

a45ecb5

CS fix

b550d44

Majkl578 mentioned this pull request Mar 4, 2014

[Doc] added note about Criteria limits on PersistentCollection #969

Merged

guilhermeblanco added a commit that referenced this pull request Mar 16, 2014

Merge pull request #885 from bakura10/criteria-many-to-many

44c1dae

Add support for ManyToMany Criteria

guilhermeblanco merged commit 44c1dae into doctrine:master Mar 16, 2014

Ocramius added a commit that referenced this pull request Mar 16, 2014

Removing note on m2m association filtering as of #885

9b4ae6d

doctrinebot mentioned this pull request Dec 6, 2015

DDC-2868: [GH-885] Add support for ManyToMany Criteria #3624

Closed

enekochan mentioned this pull request May 16, 2016

Criteria in Many-To-Many relations broken if not manually accessing data before or EAGER used #5827

Closed

foaly-nr1 mentioned this pull request Aug 24, 2016

Comparison support for many-to-many associations #5990

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ManyToMany Criteria #885

Add support for ManyToMany Criteria #885

bakura10 commented Dec 22, 2013

doctrinebot commented Dec 22, 2013

guilhermeblanco commented Dec 22, 2013

bakura10 commented Dec 22, 2013

bakura10 commented Dec 22, 2013

stof Dec 23, 2013

beberlei commented Jan 2, 2014

bakura10 commented Jan 2, 2014

bakura10 commented Jan 3, 2014

beberlei commented Jan 3, 2014

bakura10 commented Jan 4, 2014

bakura10 commented Jan 4, 2014

bakura10 commented Jan 5, 2014

bakura10 commented Jan 19, 2014

Ocramius Jan 20, 2014

Ocramius commented Jan 20, 2014

bakura10 commented Jan 20, 2014

aeneasr commented Jan 29, 2014

bakura10 commented Feb 2, 2014

Ocramius commented Feb 2, 2014

bakura10 commented Feb 9, 2014

bakura10 commented Feb 9, 2014

bakura10 commented Feb 13, 2014

deeky666 Feb 13, 2014

bakura10 Feb 13, 2014

deeky666 Feb 13, 2014

bakura10 commented Feb 18, 2014

bakura10 commented Feb 20, 2014

bakura10 commented Mar 16, 2014

aeneasr commented Mar 16, 2014

bakura10 commented Mar 16, 2014

Add support for ManyToMany Criteria #885

Add support for ManyToMany Criteria #885

Conversation

bakura10 commented Dec 22, 2013

doctrinebot commented Dec 22, 2013

guilhermeblanco commented Dec 22, 2013

bakura10 commented Dec 22, 2013

bakura10 commented Dec 22, 2013

stof Dec 23, 2013

Choose a reason for hiding this comment

beberlei commented Jan 2, 2014

bakura10 commented Jan 2, 2014

bakura10 commented Jan 3, 2014

beberlei commented Jan 3, 2014

bakura10 commented Jan 4, 2014

bakura10 commented Jan 4, 2014

bakura10 commented Jan 5, 2014

bakura10 commented Jan 19, 2014

Ocramius Jan 20, 2014

Choose a reason for hiding this comment

Ocramius commented Jan 20, 2014

bakura10 commented Jan 20, 2014

aeneasr commented Jan 29, 2014

bakura10 commented Feb 2, 2014

Ocramius commented Feb 2, 2014

bakura10 commented Feb 9, 2014

bakura10 commented Feb 9, 2014

bakura10 commented Feb 13, 2014

deeky666 Feb 13, 2014

Choose a reason for hiding this comment

bakura10 Feb 13, 2014

Choose a reason for hiding this comment

deeky666 Feb 13, 2014

Choose a reason for hiding this comment

bakura10 commented Feb 18, 2014

bakura10 commented Feb 20, 2014

bakura10 commented Mar 16, 2014

aeneasr commented Mar 16, 2014

bakura10 commented Mar 16, 2014