Adding support to add objects directly that then are serialized and adde... #295

Merged
merged 8 commits into from Jan 25, 2013

Projects

None yet

5 participants

@leahaense
Contributor

...d to the index

@lsmith77 lsmith77 commented on an outdated diff Jan 10, 2013
lib/Elastica/Document.php
* @return Elastica_Document
*/
- public function setData(array $data)
@lsmith77
lsmith77 Jan 10, 2013 Collaborator

we changed this since in the request class it checks if its an array or not already, so we can support json encoded data here too ..

@lsmith77 lsmith77 and 2 others commented on an outdated diff Jan 10, 2013
lib/Elastica/Type.php
@@ -329,4 +355,17 @@ public function request($path, $method, $data = array(), array $query = array())
return $this->getIndex()->request($path, $method, $data, $query);
}
+
+ /**
+ * Sets the serializer used in addObject
+ * @see Elastica_Type::addObject
+ *
+ * @param \JMS\Serializer\Serializer $serializer
+ * @param array $groups
+ */
+ public function setSerializer($serializer, array $groups = array())
@lsmith77
lsmith77 Jan 10, 2013 Collaborator

we didnt want to add a type hint here, since we were not sure how PHP 5.2 would react to a type hint with namespaced code. the "benefit" is that its sort of "duck typeing" .. ie. it would also work with other serializers (but only if they also have a setGroups() method, which f.e the Symfony2 Serializer does not have).

@ruflin
ruflin Jan 13, 2013 Owner

Somehow my comment here was not stored. I also prefer the "duck typing" way here as I don't think it is a good idea to have external class dependencies in the library. Probably it would make sense to add a link to an example serializer

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

another approach is to not even do it in the library or not inside this class. ie. we could use inheritance to add this feature into a subclass that can either be shipped with Elastica or with FOQElasticaBundle if you think this is a too Symfony2 eco-system specific feature.

@ruflin
ruflin Jan 13, 2013 Owner

The question is probably, if the serializer is only need in combination with Symfony2 or if it could also be used in general. If only Symfony2, it would probably make sense to add it to the FOQElasticaBundle.

@jmikola What do you think?

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

there is already a ZF2 module for https://github.com/schmittjoh/serializer and the Symfony2 serializer is also available as a standalone component (however it does not support groups)

@ruflin
ruflin Jan 13, 2013 Owner

I really like the idea of addObject and the serializer. But I'm undecided how to handle it. I would argue, that the setGroup part should happen before the serializer is added to the Type. So the Type does not have to care about the group. Or is it necessary that it is set every time an object is added?

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

this is not really feasible, since i will likely need different serialization groups per type and i also do not want to have to create a different serializer instance per type. furthermore i dont really want to have to pass in the groups in the addObject() call either, since this is something i would want to configure along with my mappings:
liip-forks/FOQElasticaBundle@master...serializer_support

@ruflin
ruflin Jan 13, 2013 Owner

Ok, so it also does not make sense to move

$this->_serializer->setGroups($this->_serializerGroups);

to setSerializer as otherwise it could be affected by external changes.

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

correct ..
the only thing we could do as i noted is to add a duck typing check into addObject() or calling a "configureSerializer()" or something that would be implemented via inheritance, but imho that would also not be great as it would limit other possibilities for extending. so another possibility would be to instead of passing in a list of groups, one would inject a callable that return a properly configured serializer instance whenever its needed.

@ruflin
ruflin Jan 13, 2013 Owner

Let's keep it the way it is. But some documentation / links should be added to the serializer and serializer groups

@ruflin
ruflin Jan 13, 2013 Owner

@leahaense @lsmith77 Can you add the documentation link and perhaps also create a small unit test for it? I would recommend to create a test serializer object instead of adding the external library.

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

actually i am starting to like the idea of a callable more and more ..

  1. it ensures that we do not need to do another check to see if groups are even supported, ie. we only need to require that there is a serialize() method
  2. it enables even more at runtime configuration that some serializers might support (f.e. the JMS serializer also supports other exclusion strategies such as version etc)
@ruflin
ruflin Jan 13, 2013 Owner

same here ...

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

actually there isnt even any reason to call serialize() at all .. just call the callable and it should take care of serialization and then we dont even have any duck typing. i will check with @leahaense next week.

@jmikola
jmikola Jan 14, 2013 Contributor

Just saw this thread. I was going to suggest having an adapter class or something (type-hinting against a specific implementation would certainly be undesirable), but the callback sounds like a great idea. You can simply document it's "interface" here and let the calling code prepare it with the appropriate serializer implementation.

@ruflin ruflin and 2 others commented on an outdated diff Jan 10, 2013
lib/Elastica/Type.php
@@ -27,6 +27,16 @@ class Elastica_Type implements Elastica_Searchable
protected $_name = '';
/**
+ * @var \JMS\Serializer\Serializer
+ */
+ protected $_serializer;
+
+ /**
+ * @var array
+ */
+ protected $_serializerGroups;
+
@ruflin
ruflin Jan 10, 2013 Owner

when, where and how is the _serializersGroup set? What is it for?

@leahaense
leahaense Jan 10, 2013 Contributor

the property is set when setting the _serializer. The symfony2 serializer supports serializer groups that define which properties of the object to be serialized are serialized and which ones are ignored.

@lsmith77
lsmith77 Jan 10, 2013 Collaborator

correction .. the Symfony2 serializer does not support serialization groups, the JMS Serializer does.
in case you have not yet used the JMS Serializer. basically it allows you to add mapping definitions for your models. these mapping definitions specify if properties should be included when serialized and with what data type etc. every mapping can optionally have a list of groups associated. then when you serialize you can specify from which groups do you want to include mappings.

@ruflin
ruflin Jan 13, 2013 Owner

The part I still don't understand is, how the variable can be set (except something extends Elastica_Type) as the variable is protected and there is no setter for serializerGroup. Is the idea that Elastica_Type is going to be extended?

@lsmith77
lsmith77 Jan 13, 2013 Collaborator

its set with setSerializer()

@ruflin
ruflin Jan 13, 2013 Owner

Sorry guys, some I was blind ...

@ruflin
Owner
ruflin commented Jan 20, 2013

@lsmith77 @leahaense Any updates here?

@lsmith77
Collaborator

we are looking to get @leahaense some more budget to work on this and another issue in the FOQElasticaBundle itself.

@leahaense
Contributor

i refactored the code to accept/expect a callable for serializing the object and also added some tests

@ruflin
Owner
ruflin commented Jan 24, 2013

Looks good. Can you merge the changes from master into your changes? We moved all the code to namespaces in the recent week.

leahaense added some commits Jan 24, 2013
@leahaense leahaense Merge remote-tracking branch 'realorigin/master' into serializer_support
Conflicts:
	lib/Elastica/Document.php
	lib/Elastica/Type.php
	test/lib/Elastica/Test/TypeTest.php
9459a9a
@leahaense leahaense fixing test 0033576
@leahaense
Contributor

done

@ruflin ruflin commented on an outdated diff Jan 24, 2013
lib/Elastica/Exception/Runtime.php
@@ -0,0 +1,7 @@
+<?php
+/**
+ * Runtime Exception
+ */
+class Elastica_Exception_Runtime extends RuntimeException
@ruflin
ruflin Jan 24, 2013 Owner

This should also be in a namespace, right?

@ruflin
Owner
ruflin commented Jan 24, 2013

Thx, but I think you missed on file. Can you also update the changes.txt file with your changes?

@leahaense
Contributor

Oh, I meant to delete this exception class because there now already is a runtime exception. It's removed now and changes.txt is adapted.

@lsmith77 lsmith77 commented on an outdated diff Jan 24, 2013
lib/Elastica/Type.php
@@ -357,4 +378,15 @@ public function request($path, $method, $data = array(), array $query = array())
return $this->getIndex()->request($path, $method, $data, $query);
}
+
+ /**
+ * Sets the serializer callable used in addObject
+ * @see Elastica_Type::addObject
@lsmith77
lsmith77 Jan 24, 2013 Collaborator

comment also needs to be updated

@lsmith77 lsmith77 commented on an outdated diff Jan 24, 2013
lib/Elastica/Type.php
@@ -357,4 +378,15 @@ public function request($path, $method, $data = array(), array $query = array())
return $this->getIndex()->request($path, $method, $data, $query);
}
+
+ /**
+ * Sets the serializer callable used in addObject
+ * @see Elastica_Type::addObject
+ *
+ * @param array|string $serializer @see Elastica_Type::_serializer
@lsmith77
lsmith77 Jan 24, 2013 Collaborator

comment also needs to be updated

@leahaense
Contributor

ok, comments fixed. i hope i caught everything now :)

@ruflin ruflin merged commit 8566a0c into ruflin:master Jan 25, 2013
@ruflin
Owner
ruflin commented Jan 25, 2013

Looks good, thx.

@munkie
Contributor
munkie commented Jan 25, 2013

I'm wondering what is the point in removing array hinting in setData method?
What kind of data serializer should return?
Is there any docs how to use this kind of functionality?

@lsmith77
Collaborator

the point is that the serializer can already take care of serialization to json.

@leahaense
Contributor

To know about how to use this have a look at the added tests.

@ruflin
Owner
ruflin commented Jan 25, 2013

As always, an addition to Elastica.io would be helpful. But first we should also update all the documentation in Elastica.io to PHP 5.3

@munkie
Contributor
munkie commented Jan 25, 2013

So it up to serializer to generate valid json string ? I was wondering because in tests mock serializer returns array not json string.

@leahaense
Contributor

It can be either array or json. Before sending the request to elasticsearch elastica checks if data is already json and if not it creates json from what's in data.

@munkie
Contributor
munkie commented Jan 25, 2013

I'm asking because i have pull request #312 and it introduces get/set/has/remove methods in Document, allowing to manipulate document data. And it relies on $_data property is array. And i'm thinking how it should handle situation when $_data is json string.

@ruflin
Owner
ruflin commented Jan 25, 2013

Good point. I completely forgot about this one. Probably if a Document is already an "object", it should throw an exception if someone tries to modify it.

@leahaense leahaense referenced this pull request in FriendsOfSymfony/FOSElasticaBundle Jan 28, 2013
Closed

Serializer support #222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment