FWM-Team / FWM-Backlog-Issues

This is a backlog of issues to deal with
0 stars 0 forks source link

Object serialization #11

Open FWAJL opened 9 years ago

FWAJL commented 9 years ago

@damirius

Your original comment:

As you know, I've implemented IDocument interface for Entities which contain files. So I had to add two new members, webPath and absolutePath to each IDocument class. I had to set them to private since our BaseManager is using public members to create queries. So when we do json_encode on those objects it will not encode everything that we need. I could add it to the IDocument interface and implement it in classes that use the mentioned interface, but I think this should be implemented somewhere in Entity abstract class since we could use it on other type of entity objects as well. We just need some small method which we can use to specify which members to return. For example GetSerializable() which will contain all members that we need to return as json later. Or we can even pass it some variable to see if we need to return, for example, just id and name or something more than that. After that we can call that method for each object and send it like that. We can also use http://php.net/manual/en/jsonserializable.jsonserialize.php.

EDIT: You can check out JsonSerializable interface implementation in Document DAO on feature_damir branch. Great thing about it is that we can just pass the object to the json_decode and jsonSerialize will be automatically called to provide the output. It works of course even with nested objects, you can check out FileLoadController in Library\Controllers lines 47-53, is the old implementation where I had to manually assign the values to the output array and on line 44 $list is just the array of Document objects which is passed to the fileResults. Problematic thing is that if we implement it in Entity class we have to implement jsonSerialize method there too with some default return value. We can use get_object_var php function, but it will return all members so we maybe don't want to do that. We can also use ReflecionObject class to select only public ones since currently we are always displaying all public properties, then we can override the method of Entity class in specific classes that extend it and that have private properties.

Thanks!

FWAJL commented 9 years ago

@damirius

First, I am thankful for the initiative.

Now, I think that the best solution is to have a default implementation in the class Entity so that it is done for all the classes. As you describe it correctly, we need a way to get all the properties to serialize and it seems to me at first glance the ReflectionObject is a good candidate to get the public properties only. It is true we don't want the private to be exposed. Can you implement this or you need help to do it?

Keep up the good work.

damirius commented 9 years ago

@FWAJL

Thanks!

I can implement this, no problem. Just let me know if you want to open new issue for this task or do you want me to implement it as the part of some other issue?

FWAJL commented 9 years ago

@damirius @bsaiken

Let's have Brian decide.

Tell us in which case you would need serialization and how long it would take to code the solution above.

If it is quicker to implement for just the Document objects, let's just do that first (isn't already done btw?) and think about a generic usage later.

Thank you.

damirius commented 9 years ago

@FWAJL

Yes it's already done for Document objects. Implementation is just adding new method and implementation. Entity should implement \JsonSerializable interface and this method should be added:

public function jsonSerialize() {
  $serializableArray = array();
  $reflectionPropertiesArray = (new \ReflectionObject($this))->getProperties(\ReflectionProperty::IS_PUBLIC);
  foreach($reflectionPropertiesArray as $property) {
    $propertyName= $property->name;
    $serializableArray[$propertyName] = $this->$propertyName;
  }
  return $serializableArray;
}

That will return the same thing as it returns now when passing object to json_encode, but overriding method in extended classes can be then implemented to return custom values with private properties or anything else we wish.

FWAJL commented 9 years ago

@damirius

Commit this code commented for now. We will need to test it throughout the site which is time consumming. I think we need to get some more urgent stuff done.

Let's see if we really need this for other purposes later. It is very likely but we probably need to tackle it then.

Anyhow, very good job suggesting this improvement! :+1:

What do you say @bsaiken ?

damirius commented 9 years ago

@FWAJL

Cool. Method should be included in entity class and commented out in new commit in my feature_damir branch. You are right that it could take time to test it out. In theory using reflector for reading public properties and generating return array from that should provide same thing as passing non serializable object directly to json_encode. But as we all know something will always go wrong.

Thanks!

bsaiken commented 9 years ago

@FWAJL @damirius

Jeremie and I will need to discuss this at our meeting next Wednesday. So, we'll have an answer next week.