Elastica/Query params converted to array on setters

[ewgRa]

I have case, when Query must be created by two classes, for example there is BaseFooClass that set some basic logic like CompanyId match or Enabled match. And next one FooClass that add to this query additional logic, like Category match for example.

Problem is that when BaseFooClass create query and set Filter to Query, this filter cast and stored as array and next FooClass can't just get "getPostFilter" and add there some additional logic.

    public function setPostFilter($filter)
    {
        if ($filter instanceof AbstractFilter) {
            $filter = $filter->toArray();
        } else {
            trigger_error('Deprecated: Elastica\Query::setPostFilter() passing filter as array is deprecated. Pass instance of AbstractFilter instead.', E_USER_DEPRECATED);
        }

In this case I set object, but get array back and can't build query step by step in different places.

Can someone describe me a way to achieve desired in my case with current Elastica code? For now I create something like custom decorator that finally build Query with calling method "toQuery" for example, but I want have this functionality in Elastica.

I understand that this is a BC changes, but maybe there is possible to have decorator, or QueryBuilder (not DSL, DSL not working in this case).

I can create pull request for this, but first need feedback to understand is it is a good idea.

[ewgRa]

Or the best way implement ArrayAccess interface to Param and another classes? And in this case we haven't BC but can work with queries parts as objects.

[ruflin]

@ewgRa I understand the issue. In case we change this, we would have to change this across the hole library I think. Because this "issue" exists for almost any object. Most of the time the objects are directly transformed to arrays to not have to deal with it at a later stage. I like the idea with ArrayAccess, but perhaps it could make even more sense to use ArrayObject as a foundation?

We should discuss the pros and cons of the different options we have in detail as this would mean quite a radical change to the library.

[ewgRa]

@ruflin ArrayAccess and ArrayObject will be also BC, or than it must be cloned, because can be such code:
$filter = new FIlter()
$query->setPostFilter($filter);
$filter->addSomething...
in current version in $query will be empty filter, but when we remove toArray - there will be object and in query will be not empty object.
I will write test for this later and make a pull request for this case to fix this in tests.

On my mind there is two options:

'1. Create new class that will be decorator of Query, and on final call make DecoratorQuery->toQuery, on "search" methods add "if $query instanceof DecoratorQuery then call toQuery".

Props:
- no BC
- Query can be marked as deprecated and removed carefully in next major releases and this DecoratorQuery will be marked as deprecated.

Cons:
- need to be maintained across to original Query class until original Query will be removed (good tests make this cons not so critical)
- need at least two major releases, first is mark Query as deprecated, than copy DecoratorQuery to Query and mark DecoratorQuery as deprecated and unmark Query as deprecated.

'2. Make Params as ArrayObject and all toArray replace to clone, to avoid BC.
Props: -
Cons:
- magic of clone, developers will be confused and always need remember this trick.
- still have BC in some cases

'3. Remove toArray everywere and call toArray on final build stage.
Props: true way
Cons: BC

This is how I see it, you as Elastica author can have different view and maybe have better ideas, at least how make 1. option much better.
I prefer 1. and 3. way, and which one is depends on BC conventions at Elastica lib.

First option is longer way, but definitely have less surprises :)

[ewgRa]

@ruflin I wrote tests that fix current state for Query class. See #802

There is many another classes that call toArray, did you think that this classes also must be covered with similar tests? It is about 65 places:

grep toArray . -R | grep function -v | grep parent -v | grep @see -v | wc -l

If there will be BC, tests will be broken and we will see this BC.

Please merge it and say your opinion of which one way we can go. I prefer third one, but question in BC. Also as must be another toArray places covered by tests or not.

[ruflin]

@ewgRa Sorry for the huge delay in answering. I think it is now time to pick this up again together with #878 I'm also in favor of method number 3. The main problem I see here is that no all toArray functions are the same and some need to raise exceptions etc. So the question is where and how this would be handled in the future?

[ewgRa]

Really big question. I need more time to research code, also as to understand QueryBuilder work.

As quick research:

that no all toArray functions are the same

I grep and see that they all have signature like "public function toArray()", I think they all can implement kind of Arrayable interface. What I miss or you mean something else?

some need to raise exceptions

As I understand #878 changes give us objects that always in valid state and in this case we will haven't exceptions in toArray and this problem is disappear.

If not, I think there is only one option to handle it - only at toArray at final state, because we can say something about valid this object or not only on final state when object translated to elasticsearch query. This is also will be BC and I think must be covered by tests to understand this BC.
I think this BC it is not a big deal, just harder to debug where is problem is, because now you get exception earlier, close to object creation, and after changes exception will be later, only on query execution.

[im-denisenko]

As I understand #878 changes give us objects that always in valid state

Nope, it's not about it. It's about consistency of querybuilder methods and constructors.

Remove toArray everywere and call toArray on final build stage.
Props: true way
Cons: BC

If this is already BC, we could also instead of Arrayable implement JsonSerializable and lets php handle recursive query serialization (it's >=5.4.0 though).

[ruflin]

@ewgRa What I was meaning by the different toArray functions is that some check if all the params are set and are in the right type. The function name and parameters are always the same.

@im-denisenko So if \Elastica\Param would implement JsonSerialisable all objects would support it and it would replace the toArray function. The part I don't like about this, is that the library then is built on top of the solution that it always must end in JSON. I know that Thrift etc. are deprecated, but I still don't think we should link it to a certain format before the data is transferred.

[ewgRa]

@ruflin If we can't guarantee that object is valid until it will be converted to string - than we can handle exceptions only at final convertation to string - in our case - right before query will be executed.
I don't think that people catch such Exceptions and have specific logic for this. In most cases this Exceptions will be fixed at development process and there is no try-catch at all.
I think it is make development process a little bit harder, because harder will be find code, where invalid object created.
Is this BC acceptable?

We can add "public function validate()" method, if someone want to check is object valid or not earlier and use it in toArray

If this is already BC, we could also instead of Arrayable implement JsonSerializable and lets php handle recursive query serialization

Can't say anything, even haven't experience with recursive json serialization. Props and cons? Can we implement it both that jsonSerialize will just call toArray?

[im-denisenko]

Props and cons?

@ewgRa Not so much, indeed. First difference is that JsonSerializable it's native php interface and we don't reinvent wheel with Arrayable which is almost same thing.

Second is that toArray must serialize all inner objects when called, while jsonSerialize just returns them. JsonSerialize-way quite simpler, because you don't need to handle all these convertions by hand.

public function toArray()
{
    return ['foo' => $this->filter->toArray()];
}

public function jsonSerialize()
{
    return ['foo' => $this->filter];
}

In example above $this->filter is just an instance of AbstractFilter, but in case if there will be complex nested structure, we should iterate over it and call toArray from every structure's member.

[im-denisenko]

The part I don't like about this, is that the library then is built on top of the solution that it always must end in JSON

@ruflin Agree, haven't thought about it.

[im-denisenko]

Just a thought: we could provide BC layer using this trick:

class Elastica\Param implements JsonSerializable
{
    /**
     * Hackish way to support convertion to array but use jsonSerialize internally.
     */
    final public function toArray()
    {
        return json_decode(json_encode($this), true);
    }
}

[ewgRa]

Actually I like idea with JsonSerializable and I think we can implement it both.

How about this https://gist.github.com/ewgRa/45e632b15bc523de03c4 ?

[ewgRa]

json_decode(json_encode($this), true);

Overhead and kind of tricky

[im-denisenko]

@ewgRa If we implement this array_walk_recursive stuff on our own, there are no real reason to drop php 5.3 and use JsonSerializable for now. I would suggest to fully imitate JsonSerializable behavior using our own interface:

• JsonSerializable => Elastica\ArraySerializable
• json_encode => Elastica\Util::arrayEncode (recursive serialization goes here)
• jsonSerialize => arraySerialize (doesn't care about inner objects, just return them as is)

[im-denisenko]

This way, in future, when php 5.3 would finally die, we could add support for JsonSerializable (just for convenience) with few lines of code:

class Elastica\Param implements ArraySerializable, JsonSerializable
{
    final public function jsonSerialize()
    {
        return $this->toArray();
    }
}

[ewgRa]

@im-denisenko toArray() for now must return only arrays, without objects. If we later will just call toArray at jsonSerialize - we lost advantage of this native recursion calls by JsonSerializable interface.

Maybe extract this question to another issue? Our goal is fast working json serialization, but also have toArray. We can add to interface three methods:
toArrayWithObjects - main method that must implement objects
toArray -> call toArrayWithObjects and recursive convert objects to arrays
jsonSerialize - just call toArrayWithObjects

How to not drop 5.3 - we can have some extra logic and make it work like symfony work with IntlDateFormatter for example. Have custom replacement for JsonSerializable, implement \JsonSerializable, then check in code if it is native JsonSerializable interface, call json_encode(Object), if not - json_encode(Object->toArray) or something like this.

Anyway, if we will have toArray that call toArrayWithObjects, later we can add jsonSerialize without break BC.

On this week I go to vacation, and return at 12 July and will have time for make PR for this issue.

[ruflin]

I'm not sure if we mix here 3 problems:

1. Working always with objects instead of converting to arrays
2. Simplifying the conversion to an array
3. Conversion to JSON

For the last one I don't really understand what the problem is having it in the transport layer. Conversion to JSON (or any non PHP format) should happen as late as possible, which is in the transport layer.

The first 2 are the ones we should focus on. My assumption in the beginning was that every object knows best how it is converted to an array, that is why the toArray function exists in every object.

I really have to take a closer look again and think about it in more depth by going through all the comments above. Keep up the good discussion.

[ewgRa]

Ok, lets focus on first two and then discuss third.

I will prepare PR after 12 July.

[ewgRa]

Mostly finished https://github.com/ewgRa/Elastica/tree/no-to-array-at-setters

There is some places left, where toArray used, will be check it soon

[ruflin]

Looks promising. I like that most of the ->toArray() disappear. Make sure to use the $this->_convertArrayable() as it is a protected function. It's quite some time I have seen a recursive function 👍

During the brief review I got the felling that the following appeared multiple times (or similar functions):

      if (isset($array[$this->_getBaseName()]['query'])) {
           $array[$this->_getBaseName()]['query'] = $array[$this->_getBaseName()]['query']['query'];
      }

Perhaps this could also be packaged in a function in case it is "really" the same. Haven't checked in detail ;-)

[ewgRa]

PR - #916

following appeared multiple times

If you have better idea how to make it more beautiful - let me know. I think about this code and haven't better idea how to rewrite it. Problem is that before there is no store objects in params, in many places there was $this->setParams($object->toArray()).

Now we need store objects in params and at toArray this adds one extra key, that must be collapsed (in some cases with additional logic).
Some solutions like decorator for objects, that will remove this extra key, or store objects in objectParams - on my mind have more cons, that props.

[ruflin]

I would suggest we merge it first and then see if we can find a better solution later.

[ewgRa]

PR for this ticket merged.

[ruflin]

Thanks