Fix Indexer to works with Models

[laryjulien]

This PR fixes #70.

Far from perfect but it works. An other way of fixing this issue is to create an Elastically\Document that extends Elastica\Document and adding a property for the "model". I am not sure this will be a better solution.

[lyrixx]

I don't understand the move. With this PR, Document::getData() still returns an object, but the PHPDoc keeps saying it return an array or a string

What did I miss?

[laryjulien]

I have created a specific key to store the object in Elastica\Document $data array. It must be use like this: new Document($id, ['__model' => $object]). Then I have changed Elastically\Indexer to look for model property in Document. If all conditions are ok, it normalizes the object in model and finally replaces the whole $data with normalized object. If the __model property is missing, then the normalize/replace will not be triggered.

[lyrixx]

I got that part. But this does not solve the initial issue,isn't ?

[laryjulien]

In a way, it does.

$indexer->scheduleIndex('beers', new Document('123', $dto)); is replaced by $indexer->scheduleIndex('beers', new Document('123', ['_model' => $dto]));

So static analyzers are not yelling about the second parameter of Document being an object.

[lyrixx]

OK, this solves only a part of the initial issue.

I'm sorry but I don't like your proposition. It really feels a hack, and it's not really natural.

I'll wait a bit for others feedback, but for now i'm voting -1 on this one.

[laryjulien]

Does the solution with an Elastically\Document extending Elastica\Document by adding a property to store the object seems better to you?

[lyrixx]

I guess, but this needs to be tested

[laryjulien]

And to complete, my first solution is clearly a hack. Because you can not add an object to a Elastica\Document, $data is meant to be an array with Document properties and values. I am only seeing two solutions:

• even we provide extra feature to Document by implementing a new one (my previous comment)

• or we add methods to Indexer that will take an object, normalize it, create a Document from it then doing the schedule part.

  Others solutions are welcome!

[lyrixx]

Maybe we can sent an PR upstream to fix the typehint

[damienalexandre]

Does the solution with an Elastically\Document extending Elastica\Document by adding a property to store the object seems better to you?

That mean we need to extend all the places where a Document is created by Elastica - we aim to override the least code possible.

add methods to Indexer that will take an object, normalize it, create a Document from it then doing the schedule part

That could work for indexing, but that does not change the type mismatch in search results https://github.com/jolicode/elastically/blob/master/src/Result.php#L34.

From my POV it's quite acceptable to have an object in the "_data" property of Document even if it's supposed to be a string|array (the property is typed as array, the constructor as string + array by the way...): https://github.com/ruflin/Elastica/blob/master/src/Document.php#L20

[laryjulien]

I can change type-hint on Elastica side but Elastica library will be more inconsistent due to $_data being an object. Example: https://github.com/ruflin/Elastica/blob/master/src/Document.php#L263 And by traversing a lot of objects from Document, Index, Client, Bulk, AbstractDocument, IndexDocument to Action. This could be a problem here: https://github.com/ruflin/Elastica/blob/master/src/Bulk/Action.php#L45.

If you have a use-case that justifies the type changing because this seems hard to argue about the benefits from Elastica POV

[laryjulien]

https://github.com/ruflin/Elastica/issues/811 https://github.com/ruflin/Elastica/issues/1067

[damienalexandre]

If you have a use-case that justifies the type changing because this seems hard to argue about the benefits from Elastica POV

There is no advantage adding object for Elastica you are right.

But our use-case here is valid, Elastically first goal is to bring objects / DTO into Elastica. Sometimes it means ignoring the phpdoc. That may break in the future, for example if PHP Type hint are added in version 8 (discussion here https://github.com/ruflin/Elastica/discussions/1961#discussioncomment-1779120) but in the meantime it's very convenient to just create a new Document, pass the DTO and that's it. I would like to keep this DX as much as possible.

[laryjulien]

What do you think about removing type-hint on $document in Indexer methods? If $document is a Document, then it is added to the Bulk else we try to serialize it.

[lyrixx]

IMHO this does not make any sense. We must not modify code for the sake of a tool. For now the code works as expected and because some PHP doc are wrong, some tools return false positives.

I think we are going in the wrong direction

[lyrixx]

Hello, This has been solved in #150 🎉