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

Add resultset transforming #1066

Merged
merged 1 commit into from
May 12, 2016
Merged

Conversation

merk
Copy link
Collaborator

@merk merk commented Mar 24, 2016

The first commit is contained in #1065 and should be merged first.

This PR adds the capability to "transform" results, which for FOSElasticaBundle allows us to insert the original object that generated the result into a new property on the Result object.

It could be that someone would want to somehow transform the $hit returned by ElasticSearch into something else and this mechanism allows that to occur. The primary magic occurs in 'TransformingBuilder' which decorates a real Builder and calls any registered Transformer for each ResultSet.

@merk merk force-pushed the resultset-transforming branch 3 times, most recently from 59542b4 to 2867782 Compare March 26, 2016 05:13
@ruflin
Copy link
Owner

ruflin commented Mar 27, 2016

Thanks for splitting this into 2 different PR's, makes it easier to review. Lets get #1065 in and then review this one. As far as I can see this one will be quite small with 5-6 files to change. Also don't forget to update the CHANGELOG.md with the changes in the second commit.

@merk
Copy link
Collaborator Author

merk commented Mar 29, 2016

Is Transformation the right name for the code provided in this PR? It might make more sense to make this more generalised - calling it a Result Processor?

I'm not sure what else one might do with the Result hits that come back from ES in this manner, but the primary use case for me is to inject original objects so they're available alongside the hit.

@merk
Copy link
Collaborator Author

merk commented Apr 28, 2016

Rebased to keep up with master and renamed the concept of a Transformer to a ResultSet Processor.

@merk
Copy link
Collaborator Author

merk commented Apr 28, 2016

FWIW, I'm not sure what Codecov means with the comment? It seems to be talking about old commits?

@ruflin
Copy link
Owner

ruflin commented Apr 28, 2016

@merk Ignore @codecov-io , I just disabled it again. It seems they have a few issues in the recent days.

@ruflin
Copy link
Owner

ruflin commented Apr 28, 2016

@merk I worry that we need to rebase again as soon as I merge one of the two PR. It seems Github started to ignore .gitattributes automatic merge for the changelog. Are you around the next hour to potentially rebase again?

@merk
Copy link
Collaborator Author

merk commented Apr 28, 2016

This one might need more work once the others are merged.

I'm still not 100% the approach is correct, and will want to implement some real processors before submitting this for merging.

@merk
Copy link
Collaborator Author

merk commented Apr 28, 2016

FWIW, the other merge conflicts weren't CHANGELOG - they were in Client.php

@ruflin
Copy link
Owner

ruflin commented Apr 28, 2016

@merk Ok, other one is merged. Let me know when you feel comfortable to move forward here. I plan to do a release today or tomorrow with the changes we have in so far. Any objections?

@merk
Copy link
Collaborator Author

merk commented Apr 28, 2016

This wont be ready today, so go ahead with a release, we'll get this one in the next release.

@merk merk closed this Apr 28, 2016
@merk merk reopened this Apr 28, 2016
@@ -4,11 +4,14 @@ All notable changes to this project will be documented in this file based on the
## [Unreleased](https://github.com/ruflin/Elastica/compare/3.1.1...HEAD)

### Backward Compatibility Breaks
- Method \Elastica\ResultSet::create and property \Elastica\ResultSet::$class were removed. To change the ResultSet class, implement your own ResultSet Builder. #1065
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That needs probably some adjustment now and be moved up as we did a release in the meantime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notes here except for the processing line were part of the previous PR, github obviously didnt update the diffs when it was merged.

@ruflin
Copy link
Owner

ruflin commented May 4, 2016

@merk In general looks good to me. I added some more notes. Two more questions to make sure I understand it to 100%:

  • Most of the users doing search and using ResultSet will keep working the same as the "default" builder is used and is an optional param.
  • All implementation which access _totalHits, _maxScore, _took, _timedOut will break and will have to use the getters.

@merk
Copy link
Collaborator Author

merk commented May 4, 2016

Yes, the default builder is going to be used in most cases, there are very few reasons to reimplement it unless you need custom behaviour. The default builder is still used even if you're going to use the ProcessingBuilder.

I think we should undo those property removals and deprecate them as well, no point to cause BC breaks unless we have to.

@merk
Copy link
Collaborator Author

merk commented May 5, 2016

Regarding your second point, you've already released 3.2.0 with these BC breaks. Do you want to release 3.2.1 reversing this (this was also part of the previous PR for the ResultSet builder, not this one for the Processor)?

@ruflin
Copy link
Owner

ruflin commented May 10, 2016

@merk Release 3.2.1 with reverting the BC breaks sounds like a great idea. Wasn't aware we could do it without :-)

@ruflin
Copy link
Owner

ruflin commented May 10, 2016

Can you rebase on top of master?

@merk
Copy link
Collaborator Author

merk commented May 11, 2016

Sent a PR with the BC break reversed

@merk
Copy link
Collaborator Author

merk commented May 11, 2016

Rebased

@ruflin ruflin merged commit 0844a92 into ruflin:master May 12, 2016
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.

2 participants