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

Make ObjectIterator compatible with Doctrine Collections #775

Closed
wants to merge 1 commit into from

Conversation

mcrumm
Copy link
Contributor

@mcrumm mcrumm commented Apr 3, 2017

PR to deprecate ONGR\ElasticsearchBundle\Collection\Collection in favor of using doctrine/collections.

doctrine/collections ~1.4

v1.4 of doctrine/collections introduced the ArrayCollection::createFrom() method, which is used internally for immutable actions when new collections are created. This ensures that common collection projection functions (like map, filter, etc.) can be used when subclassing ArrayCollection to add additional constructor arguments.

Converter::isCollection

Converter::isCollection() will check for an instance of the Doctrine\Common\Collections\Collection interface, which makes the converter much more flexible in being able to support any object that can act as a collection. Collection is the combination of the interfaces Countable, IteratorAggregate, ArrayAccess, so even a custom variation would work with the converter.

Introduces ObjectCallbackIterator( Internal )

Doctrine's Collection implements IteratorAggregate instead of Iterator, so some logic needed to be moved into a custom Iterator class that ObjectIterator can use for foreach().

Since ObjectIterator converts arrays to objects on-demand, the ObjectCallbackIterator returned by ObjectIterator::getIterator() gets instantiated with a callback that can do the document conversion and keep the $rawObjects list in ObjectIterator up-to-date. This same logic gets applied when ObjectIterator::offsetGet() is called, which is why offsetGet() needs to be overridden in ObjectIterator as well.

ObjectCallbackIterator could be marked INTERNAL to make it clear it's not meant to be used on its own.

Room for Improvement

If it's not strictly necessary for ObjectIterator to hydrate documents one-at-a-time, then this implementation could possibly be replaced by extending AbstractLazyCollection instead of ArrayCollection, and converting the documents when the collection is initialized, which is (sort of) what Doctrine's PersistentCollection does in the ORM.

Given that document hydration is tricky at best, I thought it better to leave the implementation working as-is for now. Type hinting for Collection instead of ArrayCollection, as I have done in Converter::isCollection(), would allow ObjectIterator to change transparently later on.

Closes #774

@saimaz
Copy link
Member

saimaz commented Apr 4, 2017

Very nice @mcrumm, thank you for the PR.

@mcrumm
Copy link
Contributor Author

mcrumm commented Apr 4, 2017

@saimaz I would hold off on merging this. I found a few idiosyncrasies with some of the additional methods on ArrayCollection. I can add some tests for them, which should help the code coverage, as well.

Ensure Collection::get() hydrates a document

Handle conversion in ObjectIterator::first() and ObjectIterator::last()

Handle conversion for ObjectIterator methods that return values

Add tests for ObjectCallbackIterator

CS fixes for ObjectIterator
@mcrumm
Copy link
Contributor Author

mcrumm commented Apr 4, 2017

@saimaz I think I've done all I can for this implementation. Please take a look at #777, as I think it makes for a much cleaner implementation, as well as a much safer one, in terms of side effects.

@saimaz
Copy link
Member

saimaz commented Apr 13, 2017

I'm closing this because of #777 is a much better way. I will release it in 5.1.0 version. For the 5.1 release, the only thing left is the inner hit iterator.

Thank you for the great PR.

@saimaz saimaz closed this Apr 13, 2017
@mcrumm
Copy link
Contributor Author

mcrumm commented Apr 13, 2017

I definitely like the other implementation better. Thanks for taking a look at these, @saimaz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants