Document->setData() is not supposed to accept DTO.

[VincentLanglet]

Hi @damienalexandre, @lyrixx

In the doc, there is the following example

$dto = new Beer();
$dto->bar = 'American Pale Ale';
$dto->foo = 'Hops from Alsace, France';

// Add a document to the queue
$indexer->scheduleIndex('beers', new Document('123', $dto));
$indexer->flush();

but this is throwing static analysis issues because the second parameter $data is supposed to be a string|array https://github.com/ruflin/Elastica/blob/8.x/src/Document.php#L43

This also means that one day, when typehint will be added to elastica, the solution with DTO won't work anymore.

I was surprised it currently works, but discovered it was because of the following code https://github.com/jolicode/elastically/blob/master/src/Indexer.php#L45-L48, but this won't work the day native typehint will be added to setData/getData methods.

I'm not familiar with the library and the strategy behind but a proper way to change this could be:

• Exposing a method prepareData which do

  $context = $this->contextBuilder->buildContext(\get_class($object));
  return $this->serializer->serialize($object, 'json', $context);

• Updating the doc to do

  $indexer->scheduleIndex('beers', new Document('123', $indexer->prepareData($dto)));

• Deprecating the \is_object check in scheduleIndex.

Another way could be to change the scheduleIndex signature from

scheduleIndex($index, Document $document)

to

public function scheduleIndex(string|Index $index, ?string $id, string|data|object $data))
    {
        if (\is_object($data)) {
            $context = $this->contextBuilder->buildContext(\get_class($data));
            $data = $this->serializer->serialize($data, 'json', $context);
        }

        $document = new Document($id, $data, $index);
        $this->getCurrentBulk()->addDocument($document, Bulk\Action::OP_TYPE_INDEX);

        $this->flushIfNeeded();
    }

WDYT ? Do you see another way solution to

• Avoid static analysis issues
• Avoid this library to loose the "data as DTO" feature when elastica add array|string as native type.

[jdreesen]

See also #70 and #105

[lyrixx]

Hello

Thanks for your issue.

We have already discussed that in this issue https://github.com/jolicode/elastically/issues/70 and we decided to do nothing

[VincentLanglet]

We have already discussed that in this issue #70 and we decided to do nothing

Sorry, I missed the issue when creating mine. However, I'm a little bit sad with this answer.

I agree that I don't have a good feeling with the solution proposed in https://github.com/jolicode/elastically/pull/105 which doesn't feel natural. In this PR, you said

Maybe we can sent an PR upstream to fix the typehint

That was my first thought, so I fork the elastica project and look at the code and how it's used. The document data won't work if I pass an object and don't use elastically. So the fact we can use a DTO is specifically a feature added by elastically. Therefore the elastica phpdoc is correct, they only supports array and string.

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

• With the growing popularity of static analysis tool like phpstan and psalm (as a maintainer of Symfony you should have seen all the PR adding phpdoc for those), I would say that ignoring them is the wrong direction.
• The PHP doc is not wrong, the issue is with the way it's used currently
• Yes the code works as expected so far, but the error reported is explaining that the day typehint will be added to elastica, it won't work. I would promote doing something now rather than waiting for the crash.

I can dedicate time to work on this and solve the issue. I would really appreciate if you consider and accept one solution.

Maybe you would be ok with the first solution (exposing a prepareData method in the indexer or in another service) ? This way:

• People who doesn't care bout static analysis can use this library exactly like proposed by the doc
• People who care bout static analysis and potential futur error can write

  $indexer->scheduleIndex('beers', new Document('123', $service->prepareData($dto)));

[lyrixx]

Thanks for time! this is really cool to see people wanting to fix issues.

I agree with you, it would be better so solve this issue once for all.

I did not try, but could it work with a custom Elastically\Document class? Of course, this class will extends the Elastica one. But it'll add another method, like getDto()?

edit: let's try: https://github.com/jolicode/elastically/pull/149

[VincentLanglet]

I did not try, but could it work with a custom Elastically\Document class? Of course, this class will extends the Elastica one. But it'll add another method, like getDto()?

edit: let's try: #149

This is an awesome idea indeed !

[VincentLanglet]

Solved by https://github.com/jolicode/elastically/pull/150