doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

PersistentCollection doesn't have the matching method #929

Open dorongutman opened 10 years ago

dorongutman commented 10 years ago

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 commented 10 years ago

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.

youuri commented 7 years ago

Hi guys, any update on this issue?

alcaeus commented 7 years ago

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 commented 7 years ago

I thought the custom collections feature made this less important?

On 7 Dec 2016 5:23 pm, "Andreas" notifications@github.com wrote:

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!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/doctrine/mongodb-odm/issues/929#issuecomment-265366258, or mute the thread https://github.com/notifications/unsubscribe-auth/AAu3mPs1jYpa0mXYL-04fi0AZZzgHvd_ks5rFlDRgaJpZM4Cahuy .

svscorp commented 7 years ago

+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 commented 7 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

stale[bot] commented 3 years ago

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.