Remove useless serialize on remove

[johndodev]

Pull Request

Related issue

This is a first step to fix #379

What does this PR do?

This PR remove the serializing of "about to be removed from index" entities.
Indeed, when an entity should be removed because of a change (an update of the entity, not a remove) which make it not indexable anymore, the entity was serialized. This is problem because

• the serialized result is not used
• the entity may not be serializable anymore, because of the change

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.37%. Comparing base (ff4348a) to head (164af5a).
Report is 5 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #385      +/-   ##
============================================
- Coverage     88.41%   88.37%   -0.04%     
  Complexity        1        1              
============================================
  Files            20       20              
  Lines           889      886       -3     
============================================
- Hits            786      783       -3     
  Misses          103      103              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[johndodev]

About the test which was not passing :

$result = $this->engine->remove($searchableImage);
$this->assertEmpty($result);

I think the test was wrong, the engine should not assume an entity should not be removed, as it don't know anything about previous actions on this entity.

[norkunas]

What is the real output of $result after remove?

[johndodev]

something like that :

array:1 [
  "sf_phpunit__image" => array:1 [
    0 => array:5 [
      "taskUid" => 1812
      "indexUid" => "sf_phpunit__image"
      "status" => "enqueued"
      "type" => "documentDeletion"
      "enqueuedAt" => "2025-06-06T09:25:04.919874074Z"
    ]
  ]
]

[norkunas]

Yes, the test then was totally wrong. Also I think current implementation is not optimal because it schedules to delete documents one by one.. There is a batch endpoint for this, but i'll open another issue about this.

Btw if you'd remove this code above:

$result = $this->engine->index($searchableImage);
$this->assertEmpty($result);

Does the test still pass? I think this is not needed, because there is doctrine listener which does the indexing, and the test flushes the entity to database, so not sure why it's done like this

[johndodev]

Without the previous indexing test yes, still passing.

About optimality, yes, there is also there where there is a foreach with a db query, instead of using an array in only one query, that's why we currently use a rawSearch insead of search, but as you say it's another subject.
We do something like

$raw = $searchService->rawSearch(SomeEntity::class, $q, $params);
$ids = array_map(fn(array $hit) => $hit['objectID'], $raw['hits']);
$entities = $repo->findBy['id' => $ids];

but maybe it's less generic.

[norkunas]

About that part you gave - it's more complex problem, and i am in long long progress of refactoring that.

So then remove that ->index(..) call if test still passes and it will be good to merge I think

[johndodev]

About that part you gave - it's more complex problem

Yes that what I thought about "multiple fields ids", etc.

[norkunas]

Thank you @johndodev