Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Refactor internal transformer methods to be more strict #256

Closed
wants to merge 1 commit into from

3 participants

@jmikola
Owner

Be strict about incoming values to transformNested(). 82ec81c introduced support a non-iterable $objects parameter, and proxied to transform(). This method should never be called in that case, so throwing an exception should allow us to catch bad logic or mappings.

Additionally, do not accept ArrayAccess objects when we expect an iterable value. Support for ArrayAccess in normalizeValue() dates back to 0db0490 (likely an oversight).

Lastly, use an equality check instead of calling is_null(), as it is more efficient.

@jmikola jmikola Refactor internal transformer methods to be more strict
Be strict about incoming values to transformNested(). 82ec81c introduced support a non-iterable $objects parameter, and proxied to transform(). This method should never be called in that case, so throwing an exception should allow us to catch bad logic or mappings.

Additionally, do not accept ArrayAccess objects when we expect an iterable value. Support for ArrayAccess in normalizeValue() dates back to 0db0490 (likely an oversight).

Lastly, use an equality check instead of calling is_null(), as it is more efficient.
a485229
@jmikola
Owner

@daFish: Since this reverts some purposeful changes you made in #216, feel free to chime in.

@jmikola
Owner

See follow-up comments in #216 (comment)

@daFish

@jmikola Unfortunately I'm out of ideas here right now for a better solution but as long as we'd still be able to index nested objects, even if they're null, then I'm fine with every solution.

@jmikola
Owner

@daFish: Based on the ES documentation for object and nested types, it looks like these are comparable to @Embed and @EmbedMany in Doctrine ODM, respectively. In that case, I think it's be more clear to handle them separately.

Fields mapped as object should be transformed, but no iteration should take place. Those mapped as nested should be iterated over, with each element being transformed. I suppose each should have an allowance for null, which should be transformed to the empty representation of both. But in terms of indexing, if a field is null, shouldn't it be omitted from the stored ES document altogether?

@daFish

@jmikola I see. Yeah, I wondered before why object was treated the same way than nested. I see if I can come up with a solution based on your suggestion. Thanks, Jeremy.

@merk
Owner

This PR is too stale to be merged, and transformers are scheduled to be refactored into a new approach in 3.2

@merk merk closed this
@merk merk deleted the transform-refactoring branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 27, 2013
  1. @jmikola

    Refactor internal transformer methods to be more strict

    jmikola authored
    Be strict about incoming values to transformNested(). 82ec81c introduced support a non-iterable $objects parameter, and proxied to transform(). This method should never be called in that case, so throwing an exception should allow us to catch bad logic or mappings.
    
    Additionally, do not accept ArrayAccess objects when we expect an iterable value. Support for ArrayAccess in normalizeValue() dates back to 0db0490 (likely an oversight).
    
    Lastly, use an equality check instead of calling is_null(), as it is more efficient.
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 22 deletions.
  1. +18 −22 Transformer/ModelToElasticaAutoTransformer.php
View
40 Transformer/ModelToElasticaAutoTransformer.php
@@ -117,30 +117,28 @@ protected function getPropertyValue($object, $property)
}
/**
- * transform a nested document or an object property into an array of ElasticaDocument
+ * Transform a nested document or an object property into an array of
+ * Elastica_Document objects.
*
- * @param array|\Traversable|\ArrayAccess $objects the object to convert
+ * @param array|\Traversable $objects the objects to convert
* @param array $fields the keys we want to have in the returned array
- *
* @return array
+ * @throws \InvalidArgumentException if $objects is not an array or Traversable
*/
protected function transformNested($objects, array $fields)
{
- if (is_array($objects) || $objects instanceof \Traversable || $objects instanceof \ArrayAccess) {
- $documents = array();
- foreach ($objects as $object) {
- $document = $this->transform($object, $fields);
- $documents[] = $document->getData();
- }
+ if (!(is_array($objects) || $objects instanceof \Traversable)) {
+ throw new \InvalidArgumentException('$objects parameter must be an array or Traversable.');
+ }
- return $documents;
- } elseif (null !== $objects) {
- $document = $this->transform($objects, $fields);
+ $documents = array();
- return $document->getData();
+ foreach ($objects as $object) {
+ $document = $this->transform($object, $fields);
+ $documents[] = $document->getData();
}
- return array();
+ return $documents;
}
/**
@@ -152,23 +150,21 @@ protected function transformNested($objects, array $fields)
*/
protected function normalizeValue($value)
{
- $normalizeValue = function(&$v)
- {
+ $normalize = function(&$v) {
if ($v instanceof \DateTime) {
$v = $v->format('c');
- } elseif (!is_scalar($v) && !is_null($v)) {
- $v = (string)$v;
+ } elseif ($v !== null && !is_scalar($v)) {
+ $v = (string) $v;
}
};
- if (is_array($value) || $value instanceof \Traversable || $value instanceof \ArrayAccess) {
+ if (is_array($value) || $value instanceof \Traversable) {
$value = is_array($value) ? $value : iterator_to_array($value);
- array_walk_recursive($value, $normalizeValue);
+ array_walk_recursive($value, $normalize);
} else {
- $normalizeValue($value);
+ $normalize($value);
}
return $value;
}
-
}
Something went wrong with that request. Please try again.