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

PersistentCollection doesn't have the matching method #929

Open
dorongutman opened this issue Aug 24, 2014 · 11 comments
Open

PersistentCollection doesn't have the matching method #929

dorongutman opened this issue Aug 24, 2014 · 11 comments
Labels
Milestone

Comments

@dorongutman
Copy link

Both https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/PersistentCollection.php#L861 and https://github.com/doctrine/collections/blob/master/lib/Doctrine/Common/Collections/ArrayCollection.php#L358 (which is the type one needs to use on a ReferenceMany field) have a "matching" method for searching (via a Criteria) a set of items in the collections.

However - https://github.com/doctrine/mongodb-odm/blob/master/lib/Doctrine/ODM/MongoDB/PersistentCollection.php (which is what returned on a ReferenceMany field) doesn't have that method.

How can I search in the ReferenceMany field ?

@jmikola jmikola added this to the 1.0.0 milestone Aug 26, 2014
@jmikola
Copy link
Member

jmikola commented Aug 26, 2014

This feature isn't yet implemented. Quoting the 1.0 changelog:

The base DocumentRepository class now implements the Selectable interface from the Criteria API in Doctrine Collections 1.1. This brings some consistency with ORM; however, ODM's PersistentCollection class does not yet support Selectable. This functionality was introduced in #699.

@malarzm malarzm modified the milestones: 1.0.0, 1.x Jun 11, 2015
@youuri
Copy link

youuri commented Dec 6, 2016

Hi guys, any update on this issue?

@alcaeus
Copy link
Member

alcaeus commented Dec 7, 2016

None besides what's written above. It's in the 1.x milestone, meaning we are willing to add it to one of the next 1.x releases, but nothing more than that. You could always help get things started by creating a pull request!

@redthor
Copy link

redthor commented Dec 7, 2016 via email

@svscorp
Copy link

svscorp commented Jan 3, 2017

+1 for having this feature

I checked the ArrayCollection::matching() implementation, it can be implemented in a similar way in PersistentCollection I believe. Ideally using PersistentCollection's methods instead of array_*. But then few more methods needs to be implemented, i.e. PersistentCollection::reverse.

What do you think?

@alcaeus

@alcaeus
Copy link
Member

alcaeus commented Jan 4, 2017

@svscorp I haven't taken a look in a while. Since you have an idea about how to implement it, could you create a pull request?

@etshy
Copy link
Contributor

etshy commented Apr 10, 2019

I took a bit of a look at this yesterday night (about how it was implemented in ORM too).
I'm a bit lost as I never really looked into the code before, but here what I saw for now, let me know if I'm completely wrong.

In ORM's matching method (in PersistentCollection) it call a persister->loadCriteria(Criteria) to query the referenced Entities.
The thing is, in ORM, there is a specific persisters for Relations (I remember a persister for Many relationship at least) to implement the loadCriteria differently while in ODM we only have a DocumentPersister.

I didn't take a proper look at how the persister is choosen yet.

I guess the best to do would the same and query the referenced Document (for ReferenceOne and ReferenceMany at least, as Embed Documents will be available without querying them when the "main Document" is fetched from DB), rather than query them all and then filter them like in an array ?

About the multiple persisters in ORM, Is there a need to add multiple persisters in ODM to differentiate the relations ?
If yes, how to determine which persister to instantiate ?

Do we have to change the mapping array (in persistentCollection) into a Class like in ORM ? (In ORM the mapping equivalent is called association)

Do we have to add new Collection class (like the LazyCriteriaCollection and others in ORM that doesn't seem to have equivalent in ODM) ?

I still need to look a LOT of things; how (and where) the query to referenced Documents is made, to be able to add the Criteria to it ans surely other things.

ps : Not related but there is a lot of methods with missing docBlock (return statement, param etc.), is that intentional ?

ps2 : I wrote that quickly during a break, I maybe forgot some things or it may be a bit confusing.

@alcaeus
Copy link
Member

alcaeus commented Apr 11, 2019

The implementation differs a lot from ORM. For one, we have a lot more flavours of associations than ORM. For example, for any owning-side collection, matching wouldn't really be a performance boost (be it references or embedded relationships): since the collection only keeps a list of documents (either raw embedded document data or references) that should be in it, we can't apply a custom query to the collection. So those will always have to be walked like any other generic ArrayCollection.

However, if we're dealing with a inverse reference-many that isn't loaded by a repositoryMethod (which also doesn't exist in ORM), we can return a new PersistentCollection instance that modifies the criteria property from the mapping, adding whatever criteria was received in the call to matching().

I've previously hacked this together but haven't found the time to polish it for inclusion in 2.0 - I can gladly hack it back in and push a WIP branch for others to pick up on.

As for your question regarding a new Collection class, in our case we duplicated it, but that was because we couldn't change the original implementation. For ODM, we'd have to change PersistentCollectionInterface to also extend Selectable, which is a BC break and won't happen in 2.0. Alternatively, this can be moved to PersistentCollection entirely. This is one of the reasons why I've shied away from implementing this in 2.0: it adds a lot of complexity that we're trying to avoid this late in 2.0 development (I'd like to get it done rather soon). I believe it's best to sit on this until 2.1 and 2.2 and then add this in a backward compatible fashion.

@etshy
Copy link
Contributor

etshy commented May 3, 2019

Hey.
I was in break for the past 2 weeks, and I couldn't really continue working on this.

If it's for 2.1 (or more) I guess it's not really urgent, then.

I think I'll take a better look of the sources, to know how it works, for now.

Once I know better about the sources, how it works etc, I could really help.

(I'll have to take a really good look at the 2.x sources, as I only worked with 1.x until now)

@alcaeus
Copy link
Member

alcaeus commented May 3, 2019

2.x is not very different except for coding standards and the low-level logic. Collections are very similar. As this feature won’t make it into 2.0 either way, there’s no need to hurry.

@alcaeus alcaeus removed this from the 2.x milestone Aug 5, 2021
@stale
Copy link

stale bot commented Sep 6, 2021

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues that have not seen any activity in 30 days label Sep 6, 2021
@stale stale bot closed this as completed Apr 18, 2022
@malarzm malarzm removed the Stale Issues that have not seen any activity in 30 days label Apr 18, 2022
@malarzm malarzm added this to the 2.x milestone Apr 18, 2022
@malarzm malarzm reopened this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants