cross-solution / YAWIK

YAWIK is a web application. It can be used as an ATS applicant tracking system or as a jobboard.
https://yawik.org
MIT License
125 stars 67 forks source link

[ZF3] when deleting an organization logo, reference from the organization entity is not deleted. #412

Closed cbleek closed 7 years ago

cbleek commented 7 years ago

If the organization logo is deleted, you'll get an error like

Doctrine\ODM\MongoDB\DocumentNotFoundException
File:
/var/www/zf3.yawik.org/vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/DocumentNotFoundException.php:32
Message:
The "DoctrineMongoODMModule\Proxy\__CG__\Organizations\Entity\OrganizationImage" document with identifier "59bab4980acec31623d66c9e" could not be found.

it only happens on the ZF3 branch

auswahl_999 525

cbleek commented 7 years ago

@kilip can you take a look at this?

kilip commented 7 years ago

@cbleek I am working on this issue now

kilip commented 7 years ago

Sorry @cbleek looks like I can't fix this issue, I don't have a good understanding with doctrine mongodb relation. Looks like this problem caused by doctrine reference issue. Organizations\Entity\OrganizationImage entity always have null value on $organization property, that's why the OrganizationImage::preRemove code never executed:

    /**
     * Tells Doctrine to delete the image reference, if the image is deleted.
     *
     * @ODM\PreRemove()
     */
    public function preRemove()
    {
        if ($org = $this->getOrganization()) {
            $this->getOrganization()->setImage(null);
        }
    }
cbleek commented 7 years ago

OK. @TiSiE can you help?

cbleek commented 7 years ago

Shouldn't the listener be mentioned in our documentation? http://yawik.readthedocs.io/en/latest/modules/core/index.html#events

TiSiE commented 7 years ago

@cbleek @kilip Yes, I can!

Organizations do not use a single image anymore, but ImageSets instead. (That was implemented to be able to store images in different sizes like thumbnail and original).

The code quoted above is for backwards compatibility and is not used anymore for new organization entities.

The removing of references for ImageSets is done by DeleteImageSetListener, which is registered in the event manager with the key 'Core/File/Events'. (Core/module.config.php#502).

However, that listener was removed in 2a516cb350ed8b0561324cfcdb23add04ac0a127, effectively caused this issue.

@kilip is to blame here for removing the lines :stuck_out_tongue_winking_eye: , as well as myself for approving the changes :angry: . We (and by we, I mean mostly me) need to improve on pull request reviews.. :confused:

Anyway, it's working again :smile:

TiSiE commented 7 years ago

@cbleek probably....

kilip commented 6 years ago

@cbleek , @TiSiE

Looks like this issue still not fixed. If possible please reopen the issue and assign me to fix this issue

TiSiE commented 6 years ago

It WAS fixed.

However, it's broken again.

Because of the LazyControllerFactory. When creating the FileController, the wrong EventManager gets injected! Must be the 'Core/File/Events', but instead is the 'Core/Ajax/Events'.

So, I say using LazyControllerFactory MAY NOT be a good idea, especially in this case! We should always create dedicated factories. (which is the recommend way in ZF anyway.)


Still, there will be a new issue for that!

kilip commented 6 years ago

@tisie

To prevent this happening again I will create a behat feature to test this add and remove logo functionality