From 4af9f442fd940953c5211e509d3ecaca13f6c832 Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Thu, 12 Mar 2015 22:17:57 +1100 Subject: [PATCH 1/3] Move common sorting code to base Transformer class --- .../AbstractElasticaToModelTransformer.php | 26 ++-------- Propel/ElasticaToModelTransformer.php | 26 ++-------- .../AbstractElasticaToModelTransformer.php | 51 +++++++++++++++++++ 3 files changed, 57 insertions(+), 46 deletions(-) create mode 100644 Transformer/AbstractElasticaToModelTransformer.php diff --git a/Doctrine/AbstractElasticaToModelTransformer.php b/Doctrine/AbstractElasticaToModelTransformer.php index a9afb22..603eafa 100755 --- a/Doctrine/AbstractElasticaToModelTransformer.php +++ b/Doctrine/AbstractElasticaToModelTransformer.php @@ -3,7 +3,7 @@ namespace FOS\ElasticaBundle\Doctrine; use FOS\ElasticaBundle\HybridResult; -use FOS\ElasticaBundle\Transformer\ElasticaToModelTransformerInterface; +use FOS\ElasticaBundle\Transformer\AbstractElasticaToModelTransformer as BaseTransformer; use FOS\ElasticaBundle\Transformer\HighlightableModelInterface; use Symfony\Component\PropertyAccess\PropertyAccessorInterface; @@ -12,7 +12,7 @@ use Symfony\Component\PropertyAccess\PropertyAccessorInterface; * This mapper assumes an exact match between * elastica documents ids and doctrine object ids. */ -abstract class AbstractElasticaToModelTransformer implements ElasticaToModelTransformerInterface +abstract class AbstractElasticaToModelTransformer extends BaseTransformer { /** * Manager registry. @@ -38,13 +38,6 @@ abstract class AbstractElasticaToModelTransformer implements ElasticaToModelTran 'query_builder_method' => 'createQueryBuilder', ); - /** - * PropertyAccessor instance. - * - * @var PropertyAccessorInterface - */ - protected $propertyAccessor; - /** * Instantiates a new Mapper. * @@ -69,16 +62,6 @@ abstract class AbstractElasticaToModelTransformer implements ElasticaToModelTran return $this->objectClass; } - /** - * Set the PropertyAccessor. - * - * @param PropertyAccessorInterface $propertyAccessor - */ - public function setPropertyAccessor(PropertyAccessorInterface $propertyAccessor) - { - $this->propertyAccessor = $propertyAccessor; - } - /** * Transforms an array of elastica objects into an array of * model objects fetched from the doctrine repository. @@ -111,10 +94,7 @@ abstract class AbstractElasticaToModelTransformer implements ElasticaToModelTran // sort objects in the order of ids $idPos = array_flip($ids); $identifier = $this->options['identifier']; - $propertyAccessor = $this->propertyAccessor; - usort($objects, function ($a, $b) use ($idPos, $identifier, $propertyAccessor) { - return $idPos[$propertyAccessor->getValue($a, $identifier)] > $idPos[$propertyAccessor->getValue($b, $identifier)]; - }); + usort($objects, $this->getSortingClosure($idPos, $identifier)); return $objects; } diff --git a/Propel/ElasticaToModelTransformer.php b/Propel/ElasticaToModelTransformer.php index 87c4047..c80daa2 100644 --- a/Propel/ElasticaToModelTransformer.php +++ b/Propel/ElasticaToModelTransformer.php @@ -3,6 +3,7 @@ namespace FOS\ElasticaBundle\Propel; use FOS\ElasticaBundle\HybridResult; +use FOS\ElasticaBundle\Transformer\AbstractElasticaToModelTransformer; use FOS\ElasticaBundle\Transformer\ElasticaToModelTransformerInterface; use Symfony\Component\PropertyAccess\PropertyAccessorInterface; @@ -14,7 +15,7 @@ use Symfony\Component\PropertyAccess\PropertyAccessorInterface; * * @author William Durand */ -class ElasticaToModelTransformer implements ElasticaToModelTransformerInterface +class ElasticaToModelTransformer extends AbstractElasticaToModelTransformer { /** * Propel model class to map to Elastica documents. @@ -33,13 +34,6 @@ class ElasticaToModelTransformer implements ElasticaToModelTransformerInterface 'identifier' => 'id', ); - /** - * PropertyAccessor instance. - * - * @var PropertyAccessorInterface - */ - protected $propertyAccessor; - /** * Constructor. * @@ -52,16 +46,6 @@ class ElasticaToModelTransformer implements ElasticaToModelTransformerInterface $this->options = array_merge($this->options, $options); } - /** - * Set the PropertyAccessor instance. - * - * @param PropertyAccessorInterface $propertyAccessor - */ - public function setPropertyAccessor(PropertyAccessorInterface $propertyAccessor) - { - $this->propertyAccessor = $propertyAccessor; - } - /** * Transforms an array of Elastica document into an array of Propel entities * fetched from the database. @@ -82,11 +66,7 @@ class ElasticaToModelTransformer implements ElasticaToModelTransformerInterface // Sort objects in the order of their IDs $idPos = array_flip($ids); $identifier = $this->options['identifier']; - $propertyAccessor = $this->propertyAccessor; - - $sortCallback = function ($a, $b) use ($idPos, $identifier, $propertyAccessor) { - return $idPos[$propertyAccessor->getValue($a, $identifier)] > $idPos[$propertyAccessor->getValue($b, $identifier)]; - }; + $sortCallback = $this->getSortingClosure($idPos, $identifier); if (is_object($objects)) { $objects->uasort($sortCallback); diff --git a/Transformer/AbstractElasticaToModelTransformer.php b/Transformer/AbstractElasticaToModelTransformer.php new file mode 100644 index 0000000..2b1de5c --- /dev/null +++ b/Transformer/AbstractElasticaToModelTransformer.php @@ -0,0 +1,51 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\ElasticaBundle\Transformer; + +use Symfony\Component\PropertyAccess\PropertyAccessorInterface; + +abstract class AbstractElasticaToModelTransformer implements ElasticaToModelTransformerInterface +{ + /** + * PropertyAccessor instance. + * + * @var PropertyAccessorInterface + */ + protected $propertyAccessor; + + /** + * Set the PropertyAccessor instance. + * + * @param PropertyAccessorInterface $propertyAccessor + */ + public function setPropertyAccessor(PropertyAccessorInterface $propertyAccessor) + { + $this->propertyAccessor = $propertyAccessor; + } + + /** + * Returns a sorting closure to be used with usort() to put retrieved objects + * back in the order that they were returned by ElasticSearch. + * + * @param array $idPos + * @param string $identifierPath + * @return callable + */ + protected function getSortingClosure(array $idPos, $identifierPath) + { + $propertyAccessor = $this->propertyAccessor; + + return function ($a, $b) use ($idPos, $identifierPath, $propertyAccessor) { + return $idPos[$propertyAccessor->getValue($a, $identifierPath)] > $idPos[$propertyAccessor->getValue($b, $identifierPath)]; + }; + } +} From d5a9b7b235bdbdbc757ed7c28b7608fe40f1312b Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Thu, 12 Mar 2015 22:21:53 +1100 Subject: [PATCH 2/3] Add missing method to HighlightableModelInterface. This is not a BC break - the method has always been required and lacking the method would cause a fatal error. --- Transformer/HighlightableModelInterface.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Transformer/HighlightableModelInterface.php b/Transformer/HighlightableModelInterface.php index 70b8ed3..96c6c7c 100644 --- a/Transformer/HighlightableModelInterface.php +++ b/Transformer/HighlightableModelInterface.php @@ -3,10 +3,17 @@ namespace FOS\ElasticaBundle\Transformer; /** - * Maps Elastica documents with model objects. + * Indicates that the model should have elastica highlights injected. */ interface HighlightableModelInterface { + /** + * Returns a unique identifier for the model. + * + * @return mixed + */ + public function getId(); + /** * Set ElasticSearch highlight data. * From 84e5831a813f53f236a4b3027ee49dd71bc99895 Mon Sep 17 00:00:00 2001 From: Tim Nagel Date: Thu, 12 Mar 2015 23:08:32 +1100 Subject: [PATCH 3/3] Fixing scrutinizer issues --- DependencyInjection/FOSElasticaExtension.php | 3 +-- .../AbstractElasticaToModelTransformer.php | 10 ++++++---- Doctrine/AbstractProvider.php | 2 +- Doctrine/Listener.php | 8 ++++---- Index/IndexManager.php | 5 +++++ Index/MappingBuilder.php | 6 +++--- Persister/ObjectPersisterInterface.php | 18 +++++++++--------- Persister/ObjectSerializerPersister.php | 6 +++++- Propel/ElasticaToModelTransformer.php | 2 +- Serializer/Callback.php | 12 ++++-------- Tests/Doctrine/AbstractProviderTest.php | 1 - Tests/Functional/app/ORM/IndexableService.php | 4 ++-- ...lasticaToModelTransformerCollectionTest.php | 2 +- .../ElasticaToModelTransformerCollection.php | 2 +- 14 files changed, 43 insertions(+), 38 deletions(-) diff --git a/DependencyInjection/FOSElasticaExtension.php b/DependencyInjection/FOSElasticaExtension.php index b26e50c..ec45e25 100644 --- a/DependencyInjection/FOSElasticaExtension.php +++ b/DependencyInjection/FOSElasticaExtension.php @@ -502,7 +502,7 @@ class FOSElasticaExtension extends Extension break; } - if ($tagName) { + if (null !== $tagName) { foreach ($this->getDoctrineEvents($typeConfig) as $event) { $listenerDef->addTag($tagName, array('event' => $event)); } @@ -527,7 +527,6 @@ class FOSElasticaExtension extends Extension break; default: throw new InvalidArgumentException(sprintf('Cannot determine events for driver "%s"', $typeConfig['driver'])); - break; } $events = array(); diff --git a/Doctrine/AbstractElasticaToModelTransformer.php b/Doctrine/AbstractElasticaToModelTransformer.php index 603eafa..0263f42 100755 --- a/Doctrine/AbstractElasticaToModelTransformer.php +++ b/Doctrine/AbstractElasticaToModelTransformer.php @@ -2,10 +2,10 @@ namespace FOS\ElasticaBundle\Doctrine; +use Doctrine\Common\Persistence\ManagerRegistry; use FOS\ElasticaBundle\HybridResult; use FOS\ElasticaBundle\Transformer\AbstractElasticaToModelTransformer as BaseTransformer; use FOS\ElasticaBundle\Transformer\HighlightableModelInterface; -use Symfony\Component\PropertyAccess\PropertyAccessorInterface; /** * Maps Elastica documents with Doctrine objects @@ -16,6 +16,8 @@ abstract class AbstractElasticaToModelTransformer extends BaseTransformer { /** * Manager registry. + * + * @var ManagerRegistry */ protected $registry = null; @@ -41,11 +43,11 @@ abstract class AbstractElasticaToModelTransformer extends BaseTransformer /** * Instantiates a new Mapper. * - * @param object $registry + * @param ManagerRegistry $registry * @param string $objectClass * @param array $options */ - public function __construct($registry, $objectClass, array $options = array()) + public function __construct(ManagerRegistry $registry, $objectClass, array $options = array()) { $this->registry = $registry; $this->objectClass = $objectClass; @@ -118,7 +120,7 @@ abstract class AbstractElasticaToModelTransformer extends BaseTransformer } /** - * {@inheritdoc} + * {@inheritDoc} */ public function getIdentifierField() { diff --git a/Doctrine/AbstractProvider.php b/Doctrine/AbstractProvider.php index e250e56..9845edc 100644 --- a/Doctrine/AbstractProvider.php +++ b/Doctrine/AbstractProvider.php @@ -71,7 +71,7 @@ abstract class AbstractProvider extends BaseAbstractProvider $objects = $this->getSlice($queryBuilder, $batchSize, $offset, $objects); $objects = array_filter($objects, array($this, 'isObjectIndexable')); - if ($objects) { + if (!empty($objects)) { if (!$ignoreErrors) { $this->objectPersister->insertMany($objects); } else { diff --git a/Doctrine/Listener.php b/Doctrine/Listener.php index c35b947..a1d3585 100644 --- a/Doctrine/Listener.php +++ b/Doctrine/Listener.php @@ -3,8 +3,8 @@ namespace FOS\ElasticaBundle\Doctrine; use Doctrine\Common\Persistence\Event\LifecycleEventArgs; -use FOS\ElasticaBundle\Persister\ObjectPersisterInterface; use FOS\ElasticaBundle\Persister\ObjectPersister; +use FOS\ElasticaBundle\Persister\ObjectPersisterInterface; use FOS\ElasticaBundle\Provider\IndexableInterface; use Psr\Log\LoggerInterface; use Symfony\Component\PropertyAccess\PropertyAccess; @@ -19,14 +19,14 @@ class Listener /** * Object persister. * - * @var ObjectPersister + * @var ObjectPersisterInterface */ protected $objectPersister; /** * Configuration for the listener. * - * @var string + * @var array */ private $config; @@ -84,7 +84,7 @@ class Listener $this->objectPersister = $objectPersister; $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); - if ($logger) { + if ($logger && $this->objectPersister instanceof ObjectPersister) { $this->objectPersister->setLogger($logger); } } diff --git a/Index/IndexManager.php b/Index/IndexManager.php index 13aecad..98ce870 100644 --- a/Index/IndexManager.php +++ b/Index/IndexManager.php @@ -6,6 +6,11 @@ use FOS\ElasticaBundle\Elastica\Index; class IndexManager { + /** + * @var Index + */ + private $defaultIndex; + /** * @var array */ diff --git a/Index/MappingBuilder.php b/Index/MappingBuilder.php index 93a3fcc..e03bf54 100644 --- a/Index/MappingBuilder.php +++ b/Index/MappingBuilder.php @@ -38,13 +38,13 @@ class MappingBuilder } $mapping = array(); - if ($typeMappings) { + if (!empty($typeMappings)) { $mapping['mappings'] = $typeMappings; } // 'warmers' => $indexConfig->getWarmers(), $settings = $indexConfig->getSettings(); - if ($settings) { + if (!empty($settings)) { $mapping['settings'] = $settings; } @@ -95,7 +95,7 @@ class MappingBuilder $mapping['_meta']['model'] = $typeConfig->getModel(); } - if (!$mapping) { + if (empty($mapping)) { // Empty mapping, we want it encoded as a {} instead of a [] $mapping = new \stdClass(); } diff --git a/Persister/ObjectPersisterInterface.php b/Persister/ObjectPersisterInterface.php index 9163320..f624971 100644 --- a/Persister/ObjectPersisterInterface.php +++ b/Persister/ObjectPersisterInterface.php @@ -10,6 +10,15 @@ namespace FOS\ElasticaBundle\Persister; */ interface ObjectPersisterInterface { + /** + * Checks if this persister can handle the given object or not. + * + * @param mixed $object + * + * @return boolean + */ + public function handlesObject($object); + /** * Insert one object into the type * The object will be transformed to an elastica document. @@ -66,13 +75,4 @@ interface ObjectPersisterInterface * @param array $identifiers array of domain model object identifiers */ public function deleteManyByIdentifiers(array $identifiers); - - /** - * If the object persister handles the given object. - * - * @param object $object - * - * @return bool - */ - public function handlesObject($object); } diff --git a/Persister/ObjectSerializerPersister.php b/Persister/ObjectSerializerPersister.php index 018f851..792aa9a 100644 --- a/Persister/ObjectSerializerPersister.php +++ b/Persister/ObjectSerializerPersister.php @@ -18,12 +18,16 @@ class ObjectSerializerPersister extends ObjectPersister protected $serializer; /** + * @param Type $type + * @param ModelToElasticaTransformerInterface $transformer * @param string $objectClass + * @param callable $serializer */ public function __construct(Type $type, ModelToElasticaTransformerInterface $transformer, $objectClass, $serializer) { parent::__construct($type, $transformer, $objectClass, array()); - $this->serializer = $serializer; + + $this->serializer = $serializer; } /** diff --git a/Propel/ElasticaToModelTransformer.php b/Propel/ElasticaToModelTransformer.php index c80daa2..d143478 100644 --- a/Propel/ElasticaToModelTransformer.php +++ b/Propel/ElasticaToModelTransformer.php @@ -85,7 +85,7 @@ class ElasticaToModelTransformer extends AbstractElasticaToModelTransformer $objects = $this->transform($elasticaObjects); $result = array(); - for ($i = 0; $i < count($elasticaObjects); $i++) { + for ($i = 0, $j = count($elasticaObjects); $i < $j; $i++) { $result[] = new HybridResult($elasticaObjects[$i], $objects[$i]); } diff --git a/Serializer/Callback.php b/Serializer/Callback.php index 38d93dc..004d133 100644 --- a/Serializer/Callback.php +++ b/Serializer/Callback.php @@ -23,10 +23,8 @@ class Callback { $this->groups = $groups; - if ($this->groups) { - if (!$this->serializer instanceof SerializerInterface) { - throw new \RuntimeException('Setting serialization groups requires using "JMS\Serializer\Serializer".'); - } + if (!empty($this->groups) && !$this->serializer instanceof SerializerInterface) { + throw new \RuntimeException('Setting serialization groups requires using "JMS\Serializer\Serializer".'); } } @@ -34,10 +32,8 @@ class Callback { $this->version = $version; - if ($this->version) { - if (!$this->serializer instanceof SerializerInterface) { - throw new \RuntimeException('Setting serialization version requires using "JMS\Serializer\Serializer".'); - } + if ($this->version && !$this->serializer instanceof SerializerInterface) { + throw new \RuntimeException('Setting serialization version requires using "JMS\Serializer\Serializer".'); } } diff --git a/Tests/Doctrine/AbstractProviderTest.php b/Tests/Doctrine/AbstractProviderTest.php index 24e3d15..7b41837 100644 --- a/Tests/Doctrine/AbstractProviderTest.php +++ b/Tests/Doctrine/AbstractProviderTest.php @@ -58,7 +58,6 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->with('index', 'type', $this->anything()) ->will($this->returnValue(true)); - $providerInvocationOffset = 2; $previousSlice = array(); foreach ($objectsByIteration as $i => $objects) { diff --git a/Tests/Functional/app/ORM/IndexableService.php b/Tests/Functional/app/ORM/IndexableService.php index 018451e..8f17bd0 100644 --- a/Tests/Functional/app/ORM/IndexableService.php +++ b/Tests/Functional/app/ORM/IndexableService.php @@ -13,12 +13,12 @@ namespace FOS\ElasticaBundle\Tests\Functional\app\ORM; class IndexableService { - public function isIndexable($object) + public function isIndexable() { return true; } - public static function isntIndexable($object) + public static function isntIndexable() { return false; } diff --git a/Tests/Transformer/ElasticaToModelTransformerCollectionTest.php b/Tests/Transformer/ElasticaToModelTransformerCollectionTest.php index d83f596..56a7200 100644 --- a/Tests/Transformer/ElasticaToModelTransformerCollectionTest.php +++ b/Tests/Transformer/ElasticaToModelTransformerCollectionTest.php @@ -37,7 +37,7 @@ class ElasticaToModelTransformerCollectionTest extends \PHPUnit_Framework_TestCa $this->collection = new ElasticaToModelTransformerCollection($this->transformers = array( 'type1' => $transformer1, 'type2' => $transformer2, - ), array()); + )); } public function testGetObjectClass() diff --git a/Transformer/ElasticaToModelTransformerCollection.php b/Transformer/ElasticaToModelTransformerCollection.php index 041e361..9920f43 100644 --- a/Transformer/ElasticaToModelTransformerCollection.php +++ b/Transformer/ElasticaToModelTransformerCollection.php @@ -81,7 +81,7 @@ class ElasticaToModelTransformerCollection implements ElasticaToModelTransformer $objects = $this->transform($elasticaObjects); $result = array(); - for ($i = 0; $i < count($elasticaObjects); $i++) { + for ($i = 0, $j = count($elasticaObjects); $i < $j; $i++) { $result[] = new HybridResult($elasticaObjects[$i], $objects[$i]); }