Option to listen to postRemove event #96

codedge · 2021-06-21T19:55:17Z

This PR adds the option to listen to Doctrines postRemove. This is a result of the discussion in #62, a first step.

You can know specify in meili_search.yml:

meili_search:
    # One, or multiple of postPersist, postUpdate, preRemove, postRemove
    doctrineSubscribedEvents: ['postPersist', 'postUpdate', 'preRemove', 'postRemove']

@curquiza When merging this, we need to update the wiki - Indexing automaticlly via ...

norkunas · 2021-06-22T03:21:56Z

Now I remember: the thing is that doctrine before dispatching postRemove event unsets the primary key so there is an error:

In SearchableEntity.php line 85:
                                                             
  [Symfony\Component\Config\Definition\Exception\Exception]  
  Entity has no primary key                                  
                                                             

Exception trace:
  at /var/www/html/vendor/meilisearch/search-bundle/src/SearchableEntity.php:85
 MeiliSearch\Bundle\SearchableEntity->setId() at /var/www/html/vendor/meilisearch/search-bundle/src/SearchableEntity.php:52
 MeiliSearch\Bundle\SearchableEntity->__construct() at /var/www/html/vendor/meilisearch/search-bundle/src/Services/MeiliSearchService.php:337
 MeiliSearch\Bundle\Services\MeiliSearchService->makeSearchServiceResponseFrom() at /var/www/html/vendor/meilisearch/search-bundle/src/Services/MeiliSearchService.php:131
 MeiliSearch\Bundle\Services\MeiliSearchService->remove() at /var/www/html/vendor/meilisearch/search-bundle/src/EventListener/MeiliSearchIndexerSubscriber.php:47
 MeiliSearch\Bundle\EventListener\MeiliSearchIndexerSubscriber->postRemove() at /var/www/html/vendor/symfony/doctrine-bridge/ContainerAwareEventManager.php:68
 Symfony\Bridge\Doctrine\ContainerAwareEventManager->dispatchEvent() at /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/Event/ListenersInvoker.php:115
 Doctrine\ORM\Event\ListenersInvoker->invoke() at /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:1251
 Doctrine\ORM\UnitOfWork->executeDeletions() at /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:446
 Doctrine\ORM\UnitOfWork->commit() at /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:378
 Doctrine\ORM\EntityManager->flush() at /var/www/html/var/cache/dev/Container52kWrqR/EntityManager_9a5be93.php:129
 Container52kWrqR\EntityManager_9a5be93->flush() at /var/www/html/src/Command/TestasCommand.php:70
 App\Command\TestasCommand->execute() at /var/www/html/vendor/symfony/console/Command/Command.php:299
 Symfony\Component\Console\Command\Command->run() at /var/www/html/vendor/symfony/console/Application.php:996
 Symfony\Component\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/framework-bundle/Console/Application.php:96
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/console/Application.php:295
 Symfony\Component\Console\Application->doRun() at /var/www/html/vendor/symfony/framework-bundle/Console/Application.php:82
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /var/www/html/vendor/symfony/console/Application.php:167
 Symfony\Component\Console\Application->run() at /var/www/html/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php:56
 Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() at /var/www/html/vendor/autoload_runtime.php:35
 require_once() at /var/www/html/bin/console:15

norkunas · 2021-06-22T03:22:32Z

Also I've forgot about rollbacked transactions :) but that probably should follow in another PR.

codedge · 2021-06-22T07:25:23Z

There are currently no tests for listening and then acting on the events coming from Doctrine. I'll add some to catch these kind of errors.

curquiza · 2021-06-22T13:36:48Z

Thanks for your PR @codedge, request me as reviewer when you think this is finished! 🙂

codedge · 2021-06-23T11:27:54Z

After some digging this is the result:

`postRemove` has no entity id

The postRemove event is obviously not the best choice when you need the entity id for further processing.
There is a big discussion about this here doctrine/orm#2326 with no outcome. It is just by design.

Yes, we can tackle this with some hacks, f. ex. saving the entity in some new field in the preRemove event so that we still have it in the postRemove event but that is... hacky. I am not a big fan of that.

The hack would mean that we always would need to listen to the preRemove event when the postRemove event is configured.

So using the postRemove event for removing a document from MS is likely not going to be successful. What dou you think?

Transactions

If you @norkunas can outline what your concerns are regarding? 😄

I would think if your transaction fails, meaning the removal of the entity in your db is not successful, but the preRemove event already fired and removed the document from MS - then why not adding it again?

It seems there are also new Transaction events added in Doctrine so you could catch those errors and call MS again to re-index the document.

Let me know, where I am wrong here 😉

norkunas · 2021-06-23T12:29:20Z

My primary concern was to have at least integration with messenger by adding but the problem is that SearchService does not have a method to remove document by identifier which I could handle by myself when needed with preRemove setting the ID and postRemove dispatching the message :)

codedge · 2021-06-23T12:39:40Z

I see :D Well, then we all learned a bit about Doctrine :D :D

@curquiza
Then I would still push some test for this one here and remove the postRemove config for now. Then we should move on with the messenger.

codedge · 2021-06-26T14:04:07Z

Changes in this PR are only pointing towards testing, code style (strict typings, format).

PHPstan

I added https://github.com/phpstan/phpstan for checking the coding regarding type juggling.
There is a composer script composer phpstan that start and runs phpstan.

The current level is 5, 8 are available and the more the better. For now, I think this is quite a good result.

I already added this to be a check for every PR. See my changes to the test.yml.

Separating tests

I felt like some tests are more like unit testing, most are integration tests. That's why I split them into a Unit and an Integration directory and created two test suites. That's nothing anyone will notice but it seems it gives some more structure to the testing files.

Further the unit tests can rely on the default KernelTestCase that is shipped with Symfony. I feel like sticking to the (Symfony) defaults is like a good thing.

Code style

I added the new PHP CS Fixer rule PHP80MigrationRisky which works a bit more towards stricter code/types by adding return types to methods.

Doctrine events tests & coverage

There are now doctrine event subscriber tests. The overall test coverage is about 85%.
There is additionally a composer command composer test:unit:coverage which will create the code coverage (if you have either xdebug or pcov installed).

Obosolete files

I found two files that are obsolete and used nowhere.

All the changes should be non-breaking. If you checked @curquiza feel free to merge.

… checks

norkunas · 2021-06-28T06:36:49Z

src/Engine.php

@@ -80,7 +80,7 @@ public function remove($searchableEntities): array
        /** @var SearchableEntity $entity */
        foreach ($searchableEntities as $entity) {
            $searchableArray = $entity->getSearchableArray();
-            if (null === $searchableArray || 0 === \count($searchableArray)) {
+            if (null == $searchableArray || 0 == \count($searchableArray)) {


Why this change? :)

This change came from PHPstan stating that this comparison is always null. I prepared an example in the PHPstan playground: https://phpstan.org/r/07c0aa63-1a31-4a24-b6af-c14dffa92dc5

In the end I adjusted the code now:

$entity->getSearchableArray() always returns an array

The check of null is removed now

From time to time I am opting for PHPstan level 6, so there might be changes added that focus towards level 6.

curquiza

Thank you a lot everyone!!
And sorry for the delay!

bors merge

🚀

@curquiza

96: Option to listen to postRemove event r=curquiza a=codedge This PR adds the option to listen to Doctrines `postRemove`. This is a result of the discussion in #62, a first step. You can know specify in `meili_search.yml`: ``` meili_search: # One, or multiple of postPersist, postUpdate, preRemove, postRemove doctrineSubscribedEvents: ['postPersist', 'postUpdate', 'preRemove', 'postRemove'] ``` @curquiza When merging this, we need to update the [wiki - Indexing automaticlly via ...](https://github.com/meilisearch/meilisearch-symfony/wiki/index-data-into-meilisearch#indexing-automatically-via-doctrine-events) Co-authored-by: Holger Lösken <[email protected]>

bors · 2021-07-01T17:47:15Z

Timed out.

codedge · 2021-07-01T18:05:10Z

bors has its bad days 😃

@curquiza Can you create a release out of the current master branch after this is merged? My last changes regarding the installation, #94, is thing I need for the Symfony recipe. The Symfony recipe checks fails, because of the error fixed in #94 .

curquiza · 2021-07-01T18:27:58Z

I merge without bors then, because I'm on holiday in few minutes during a month and I don't want to wait 😇

codedge changed the title ~~Option to listen to postRemove event~~ WIP: Option to listen to postRemove event Jun 22, 2021

Option to listen to postRemove event

7cc5a35

codedge force-pushed the doctrine-post-remove branch from d88ea37 to 7cc5a35 Compare June 22, 2021 18:27

Holger Lösken added 4 commits June 26, 2021 13:29

Add more testing

10f6261

Fix cs

fcea1db

Update code style

30ae63f

Add phpstan

074d1db

Holger Lösken added 6 commits June 26, 2021 16:15

Remove deprecated composer option

827b14b

Dependabot config for updating github action versions, add phpstan to…

b2fc4d9

… checks

Rename linter

14f59c9

Test if deleting is async and test needs to wait

8ea92a7

2 seconds wait might be enough

9a87048

Fix cs

31fda9d

codedge changed the title ~~WIP: Option to listen to postRemove event~~ Option to listen to postRemove event Jun 26, 2021

norkunas reviewed Jun 28, 2021

View reviewed changes

Holger Lösken added 2 commits June 28, 2021 23:20

Fix comparison

ee08110

Fix cs

81945cc

curquiza mentioned this pull request Jul 1, 2021

Document "Listen to postRemove event" #97

Closed

curquiza approved these changes Jul 1, 2021

View reviewed changes

curquiza merged commit 2fd7b74 into main Jul 1, 2021

curquiza deleted the doctrine-post-remove branch July 1, 2021 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Option to listen to postRemove event #96

Option to listen to postRemove event #96

Uh oh!

codedge commented Jun 21, 2021

Uh oh!

norkunas commented Jun 22, 2021 •

edited

Loading

Uh oh!

norkunas commented Jun 22, 2021

Uh oh!

codedge commented Jun 22, 2021

Uh oh!

curquiza commented Jun 22, 2021

Uh oh!

codedge commented Jun 23, 2021

Uh oh!

norkunas commented Jun 23, 2021

Uh oh!

codedge commented Jun 23, 2021

Uh oh!

codedge commented Jun 26, 2021 •

edited

Loading

Uh oh!

norkunas Jun 28, 2021

Uh oh!

codedge Jun 28, 2021 •

edited

Loading

Uh oh!

curquiza left a comment

Uh oh!

bors bot commented Jul 1, 2021

Uh oh!

codedge commented Jul 1, 2021

Uh oh!

curquiza commented Jul 1, 2021

Uh oh!

Uh oh!

Option to listen to postRemove event #96

Option to listen to postRemove event #96

Uh oh!

Conversation

codedge commented Jun 21, 2021

Uh oh!

norkunas commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

norkunas commented Jun 22, 2021

Uh oh!

codedge commented Jun 22, 2021

Uh oh!

curquiza commented Jun 22, 2021

Uh oh!

codedge commented Jun 23, 2021

postRemove has no entity id

Transactions

Uh oh!

norkunas commented Jun 23, 2021

Uh oh!

codedge commented Jun 23, 2021

Uh oh!

codedge commented Jun 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PHPstan

Separating tests

Code style

Doctrine events tests & coverage

Obosolete files

Uh oh!

norkunas Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

codedge Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curquiza left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jul 1, 2021

Uh oh!

codedge commented Jul 1, 2021

Uh oh!

curquiza commented Jul 1, 2021

Uh oh!

Uh oh!

norkunas commented Jun 22, 2021 •

edited

Loading

`postRemove` has no entity id

codedge commented Jun 26, 2021 •

edited

Loading

codedge Jun 28, 2021 •

edited

Loading