Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixing identifier field not being set in TransformerCollection #167

Merged
merged 1 commit into from Nov 28, 2012

Conversation

Projects
None yet
2 participants
Contributor

richardmiller commented Sep 24, 2012

As per #165

@jmikola can you just give this a once over when you get a chance. Should fix the issue - as it stood the identifier option was set to be set for the whole collection when it needs to be per transformer. Not sure that having to ad the method to get it from the transformers is ideal but seemed better than having to build a config mapping of class to identifier field for the collection transformer.

Owner

jmikola commented Oct 2, 2012

Identifiers are already provided in the options, so we're just hooking into it here instead of passing the options array to ElasticaToModelTransformerCollection service, correct?

@jmikola jmikola commented on the diff Oct 2, 2012

Transformer/ElasticaToModelTransformerCollection.php
$transformed = array();
foreach ($sorted AS $type => $objects) {
$transformedObjects = $this->transformers[$type]->transform($objects);
- $transformed[$type] = array_combine(array_map(function($o) use ($identifierGetter) {return $o->$identifierGetter();},$transformedObjects),$transformedObjects);
+ $identifierGetter = 'get' . ucfirst($this->transformers[$type]->getIdentifierField());
@jmikola

jmikola Oct 2, 2012

Owner

Can we use the PropertyPath here, as we do in AbstractElasticaToModelTransformer? I think that's only available in Symfony 2.1+, so we'd probably want this solution for your PR and then patch it in the master branch shortly after merging.

@richardmiller

richardmiller Oct 3, 2012

Contributor

I think it is 2.1 only - we are using this way in the 2.0 version of AbstractElasticaToModelTransformer would be good to use PropertyPath in master though

Contributor

richardmiller commented Oct 3, 2012

The identifier option is per mapped entity so it could not have been passed in as a single option as the class was set up to accept (and which was not being passed in at all when it was created as a service). To pass it in as an options argument we would have had create a mapping of entities to their identifier field and then looked them up in this. Since the individual transformers already know what the identifier field it seemed sensible to just ask them.

Although the actual identifier is not really needed here as it is just used to maintain the order of the documents in an array so anything that uniquely identifies them within the type would do.

@richardmiller richardmiller added a commit that referenced this pull request Nov 28, 2012

@richardmiller richardmiller Merge pull request #167 from Exercise/fixing_collection_identifier
Fixing identifier field not being set in TransformerCollection
51f03eb

@richardmiller richardmiller merged commit 51f03eb into 2.0 Nov 28, 2012

@tumbochka tumbochka pushed a commit to formapro-forks/FOSElasticaBundle that referenced this pull request Jul 17, 2014

@jeremymarc jeremymarc Merge pull request #167 from Remixjobs/fmessages-fix
[js flash-messages] Add core.js to template
431a677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment