QueryBuilder methods should have same signatures as corresponding constructors

[im-denisenko]

I would like to force all methods of QueryBuilder DSL classes have exact same signatures as constructors of classes they create. It'll prevent from something like #872, #796, #798 in future.

It could be done in two different ways:

• Reflection magic in QueryBuilder tests (check here)
• Replace current methods implementation with combination of call_user_func + func_get_args.

@webdevsHub Is there some reason why querybuilder was not implemented by second way?

[im-denisenko]

PR is not ready yet.

[webdevsHub]

I will have a look into your tests changes later this week, but I am not happy with the tests as they are right now too (especially that "argument check array" which is hard to understand I guess).

There is a reason why I decided to write all that methods and not use __call with @method annotations:

I wanted the QueryBuilder to act like you are writing real Elasticsearch queries (this is the reason why I used snake_case and not camelCase). Many constructors actually do not behave like real Elasticsearch queries (e.g. most of the constructor arguments are optional, but required in real ES queries like using bool_and filter without at least 2 other filters). In addition Elastica and ES naming is sometimes different (like geo_shape or bool filters).

I found so much things that prevented a direct one-one Elastica <=> ES mapping that I decided to write a method for each single DSL part, because it is the most flexible implementation (you can add custom "pre-constructor" code on a very basic level or it can be used as a compatibility layer if ES dicided to change something in existing parts in a new version).

As always, the most flexible implementation has a high risk of bugs and of diverging from the original intention.

I am not convinced that call_user_func + func_get_args can work without serious decrease of usebility (usebility in my view here is: write queries like in actual Elasticsearch JSON).

[im-denisenko]

most of the constructor arguments are optional, but required in real ES

With optional arguments, you're free to write queries like this:

$and = $qb->filter()->bool_and();

if (/* one */) {
    $and->addFilter(/* filter one */);
}

if (/* two */) {
    $and->addFilter(/* filter two */);
}

If these filters required in constructor, you forced to create array first:

$andFilters = []; // why I need this variable?

if (/* one */) {
    $andFilters[] = /* filter one */;
}

if (/* two */) {
    $andFilters[] = /* filter two */;
}

$and = $qb->filter()->bool_and($andFilters);

I'm think, it'd be better to check (if check at all) ES requirements on serialization stage, when toArray is called.

[im-denisenko]

I am not convinced that call_user_func + func_get_args can work without serious...

I mean following:

class Query implements DSL
{
    /**
     * @param AbstractQuery  $query
     * @param AbstractFilter $filter
     */
    public function filtered()
    {
        $constructor = 'Elastica\Query\Filtered::__construct';
        return call_user_func_array($constructor, func_get_args());
    }
}

Without explicit arguments all methods will work as pure proxy to constructors, and we still can have docblock with explained params. Even if some parameter will exist in constructor but will missing in docblock, it less worse than now, because it will be just a documentation bug.

Update:
I see now, that it's not possible to call constructor via call_user_func, but it still can be done with ReflectionClass::newInstanceArgs

[webdevsHub]

I like that reflection proxy idea and it is definitely better then my implementation 👍

About serialization stage: I quickly scanned though toArray code and found exceptions, invalid array structures and often no required checks at all. But just like constructor args, QueryBuilder should not try to fix that and work as pure proxy.

[ruflin]

I also like that query builder is only a proxy. Standardising the toArray is a good idea. Also check out the suggestions from @ewgRa here about the toArray functions: #878

If can make some changes here that make it even more "standard", that would be nice. Also working with objects most of the time could be help. For example every time I pass $type to an object, I have to check if it actually expects the string or the object.

[webdevsHub]

If this is implemented at some point in time $name would be required again (like in all aggregations) and produce a BC break.

[webdevsHub]

tests are very good implemented and easy to understand! Do you want it to be merged or do you want to include that proxy approach in this pull request?

[im-denisenko]

@webdevsHub Yep, I want to add proxy and remove _assertParametersEquals as it's not required in that case. I'll add these changes later today.

[ruflin]

One thing that is very nice right now is the autocomplete in the IDE. I assume this would not work anymore if we would work with Reflection magic.

[im-denisenko]

@ruflin They probably should perform autocompletion using docblock but not parameters itself. I'll check it later.

[im-denisenko]

Sadly, but autocomplete not working with reflection magic, so I left out this idea.

PR is ready to be merged.

[ruflin]

Merged. Thx.