TagRepository::deleteTaggingByParams() Dangerous behaviour !

[sadortun]

Summary
Hi !

I was very lucky to detect this issue before some major side effects. And i am not sure if this is the intended behaviour.

Calling TagManager::deleteTagging([ 1 , 2 , 3] , 'My\Entity') is working perfectly fine and as intended, but if you specify an empty array [] as a list of tags to delete, the method delete everything, instead of not deleting any tags.

This behaviour would be similar to calling $myCollection->removeElement() and having the entire collection wiped out. instead of not deleting anything.

    public function deleteTaggingByParams(array $tags, $entityClassName, $entityId, User $owner = null)
    {
        $builder = $this
            ->getDeleteTaggingQueryBuilder($entityClassName)
            ->andWhere('t.recordId = :entityId')
            ->setParameter('entityId', $entityId);

        if (!empty($tags)) {
            $builder->andWhere($builder->expr()->in('t.tag', ':tags'))->setParameter('tags', $tags);
        }
// .....

This method seems to be only used once in the TagListener, and uses the [] to delete tagging before deleting a Taggable entity.

IMO, you really should make the deleteTagging() method safe, and add another method deleteAllTagging() that is used when you want to delete everything.

Steps to reproduce
TagRepository::deleteTaggingByParams([ ] , 'My\Entity')

Actual Result No changes in tags

Expected Result
All tags associated to the entity are deleted.

Details about your environment

• OroPlatform: 4.1.9
• PHP version: 7.4.5
• Database (MySQL, PostgreSQL) version

[anyt]

Hi Samuel, It's a valid improvement. Feel free to create a PR with the fix.

[sadortun]

@anyt Sure, will do. Ill get back to you in about 2 weeks.