Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cbleek opened this issue Sep 15, 2017 · 10 comments
Projects
Milestone

Comments

@cbleek
Copy link
Member

cbleek commented Sep 15, 2017

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 cbleek added this to the ZF3 milestone Sep 15, 2017
@cbleek cbleek added the bug label Sep 15, 2017
@cbleek
Copy link
Member Author

cbleek commented Sep 15, 2017

@kilip can you take a look at this?

@kilip
Copy link
Collaborator

kilip commented Sep 18, 2017

@cbleek I am working on this issue now

@kilip
Copy link
Collaborator

kilip commented Sep 19, 2017

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
Copy link
Member Author

cbleek commented Sep 19, 2017

OK. @TiSiE can you help?

cross-bot pushed a commit that referenced this issue Sep 19, 2017
Add 'DeleteImageSetListener' back to 'Core/File/Events' event manager.
@cbleek
Copy link
Member Author

cbleek commented Sep 19, 2017

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

@TiSiE
Copy link
Member

TiSiE commented Sep 19, 2017

@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 2a516cb, effectively caused this issue.

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

Anyway, it's working again 😄

@TiSiE TiSiE added the fixed label Sep 19, 2017
@TiSiE
Copy link
Member

TiSiE commented Sep 19, 2017

@cbleek probably....

@kilip
Copy link
Collaborator

kilip commented Feb 14, 2018

@cbleek , @TiSiE

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

@TiSiE
Copy link
Member

TiSiE commented Feb 14, 2018

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
Copy link
Collaborator

kilip commented Feb 15, 2018

@TiSiE

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

TiSiE added a commit that referenced this issue Feb 15, 2018
* pr-462:
  [Organizations] added FileControllerFactory, ref #412 * fixed error when removing images because of the wrong EventManager injected * added behat scenario to test add and remove logo from organization
  [Organizations] changed filter for ListJobQuery from Organization to Organization::getId()
  [Organizations] improved Organizations Profile Page * fixed errors when Organization Image is null * added list organization profile filter for User::getRole() is not a User::ROLE_USER * added Behat scenario to tests profile listing for Guest and Recruiter
  [Organizations] improved Organization Profile pages * allowed guests to access organization profile page * fixed job list pagination in organization profile page also fix ajax loading * displayed image in organization profile detail
  [Organizations] added factory to create InviteController
  [Organizations] added organizations profile page ref #458 * profile lists available in route lang/organizations/profile * profile detail available in route lang/organizations/profile/:id * added feature and scenario to test profile
  [PHPUnit] ignored deprecated classes and function from code coverage via @deprecated comment
  [Behat] improved behat tests: * clean all data everytime test performed
  [Organizations] fixed label for Organization Name
  improved behat tests, ref #458
  [Organizations] added job paginator for Organization Profile page ref #458
  [Behat] improved creating job
  [Core] added behat tests for organization profile
  [Organizations] added profile controller ref #458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
ZF3
Done
Development

No branches or pull requests

3 participants