Dizkus breaks if user is deleted.

[damon18]

Sorry, I broke Dizkus.

Thinking about issue zikula/core#1322 I wanted to find out what currently happens when a user is deleted (by admin). So I created a user and made some posts in Dizkus, then deleted the user and visited Dizkus.

Entity was not found. 
500 Internal Server Error - EntityNotFoundException 

Stack Trace (Plain Text)   
[1] Doctrine\ORM\EntityNotFoundException: Entity was not found.
    at n/a
        in C:\xampp\htdocs\core\src\vendor\doctrine\orm\lib\Doctrine\ORM\Proxy\ProxyFactory.php line 177

    at Doctrine\ORM\Proxy\ProxyFactory->Doctrine\ORM\Proxy\{closure}(object(UserEntity), 'offsetGet', array('uid'))
        in C:\xampp\htdocs\core\src\app\cache\dev\doctrine\orm\DoctrineProxy\__CG__ZikulaModuleUsersModuleEntityUserEntity.php line 582

    at Closure->__invoke(object(UserEntity), 'offsetGet', array('uid'))
        in C:\xampp\htdocs\core\src\app\cache\dev\doctrine\orm\DoctrineProxy\__CG__ZikulaModuleUsersModuleEntityUserEntity.php line 582

    at DoctrineProxy\__CG__\Zikula\Module\UsersModule\Entity\UserEntity->offsetGet('uid')
        in C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\lastPostBy--t_Zikula\Theme\Andreas08Theme-l_en.php line 7

    at include('C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\lastPostBy--t_Zikula\Theme\Andreas08Theme-l_en.php')
        in C:\xampp\htdocs\core\src\vendor\drak\smarty\Smarty\Smarty.class.php line 1870

    at Smarty->_smarty_include(array('smarty_include_tpl_file' => 'user/lastPostBy.tpl', 'smarty_include_vars' => array('last_post' => object(PostEntity))))
        in C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\forum\singleforumtable--t_Zikula\Theme\Andreas08Theme-l_en.php line 72

    at include('C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\forum\singleforumtable--t_Zikula\Theme\Andreas08Theme-l_en.php')
        in C:\xampp\htdocs\core\src\vendor\drak\smarty\Smarty\Smarty.class.php line 1870

    at Smarty->_smarty_include(array('smarty_include_tpl_file' => 'user/forum/singleforumtable.tpl', 'smarty_include_vars' => array()))
        in C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\main--t_Zikula\Theme\Andreas08Theme-l_en.php line 28

    at include('C:\xampp\htdocs\core\src\app\cache\dev\ztemp\view_compiled\zikula-dizkus\Zikula\Module\DizkusModule\user\main--t_Zikula\Theme\Andreas08Theme-l_en.php')
        in C:\xampp\htdocs\core\src\lib\legacy\Zikula\View.php line 2938

    at Zikula_View->_fetch('user/main.tpl', '', '', false)
        in C:\xampp\htdocs\core\src\lib\legacy\Zikula\View.php line 756

    at Zikula_View->fetch('user/main.tpl')
        in C:\xampp\htdocs\core\src\modules\zikula-dizkus\Zikula\Module\DizkusModule\Controller\UserController.php line 99

    at Zikula\Module\DizkusModule\Controller\UserController->indexAction(array())
        in  line 

    at call_user_func(array(object(UserController), 'indexAction'), array())
        in C:\xampp\htdocs\core\src\lib\util\ModUtil.php line 1153

    at ModUtil::exec('ZikulaDizkusModule', 'user', 'index', array(), false, null)
        in C:\xampp\htdocs\core\src\lib\util\ModUtil.php line 1203

    at ModUtil::func('ZikulaDizkusModule', 'user', 'index', array())
        in C:\xampp\htdocs\core\src\lib\Zikula\Bundle\CoreBundle\EventListener\LegacyRouteListener.php line 96

    at Zikula\Bundle\CoreBundle\EventListener\LegacyRouteListener->onKernelRequest(object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
        in  line 

    at call_user_func(array(object(LegacyRouteListener), 'onKernelRequest'), object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher.php line 393

    at Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher->Symfony\Component\HttpKernel\Debug\{closure}(object(GetResponseEvent), 'kernel.request', object(Zikula_EventManager))
        in  line 

    at call_user_func(object(Closure), object(GetResponseEvent), 'kernel.request', object(Zikula_EventManager))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\EventDispatcher.php line 164

    at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure), object(Closure)), 'kernel.request', object(GetResponseEvent))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\EventDispatcher.php line 53

    at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher.php line 167

    at Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', object(GetResponseEvent), null)
        in C:\xampp\htdocs\core\src\lib\legacy\Zikula\EventManager.php line 99

    at Zikula_EventManager->dispatch('kernel.request', object(GetResponseEvent))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher.php line 140

    at Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\HttpKernel.php line 107

    at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Zikula_Request_Http), '1')
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\HttpKernel.php line 66

    at Symfony\Component\HttpKernel\HttpKernel->handle(object(Zikula_Request_Http), '1', true)
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel.php line 64

    at Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel->handle(object(Zikula_Request_Http), '1', true)
        in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\HttpKernel\Kernel.php line 188

    at Symfony\Component\HttpKernel\Kernel->handle(object(Zikula_Request_Http))
        in C:\xampp\htdocs\core\src\index.php line 21

[craigh]

deleting users is not supported so IMO this is not a bug... it should break.... shouldn't it?

[damon18]

The user module has a process for deleting users.

[damon18]

So no, shouldn't break. I would suggest that in the case of a post or topic where the username attached to the post UID is gone that the post/topic should remain and display "unknown" or "removed" as username.

[ghost]

You might just need to move the doctrine calls to the controller so they are not being called in the template because otherwise there is no way to trap the not found exception. Unless there is a way to put magic in the entity methods, I would simply move the logic into the controller and use try/catch.

[craigh]

It's not that simple.

[ghost]

This looks promising http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/partial-objects.html

[craigh]

PRs are welcome to show POC. I do not see anyway to solve this other than

1. reassign orphaned data
2. don't actually delete the user, just mark as deactivated

I'm leaning toward option 2 now (rather than my own core PR) but of course that is a core issue.

[ghost]

Here is an example that you can use exceptions. Obviously it can be done differently, but this is a POC. views/lastPostBy.tpl:

<span>
    {if $last_post}
        {php}
            try {
               $this->_tpl_vars['last_post']['poster']['user']['uid'];
            } catch (Exception $e) {
               $this->_tpl_vars['not_found'] = true;
            }
        {/php}
        {if isset($not_found)}
            {gt text="Last post by UNKNOWN USER"}<br />
        {else}
        {gt text="Last post by %s" tag1=$last_post.poster.user.uid|profilelinkbyuid}<br />
        {/if}
        {$last_post.post_time|dateformat:'datetimebrief'}
        <a class="tooltips" title="{gt text="View latest post"}" href="{lastTopicUrl topic=$last_post.topic}"><i class='icon-hand-right'></i> {$last_post.topic.title|strip_tags|truncate:25:'...'}</a>
    {/if}
</span>

[ghost]

It would seem to be that it would be more robust if Diskus actually stored the username in the "shadow" table. This way it can always get it. The uname does not change, just like the uid so it would be a much more robust way and you get better than "unknown user" you get the real user.

You could even go a step further - this was discussed with @rmburkhead a few years back - you can use the delete event to copy over the relevant data, like uname etc, to the "shadow" users table.

Either way, this reduces coupling. In reality, direct coupling to another module's entity is not fantastic, but rather using APIs which can provide an abstract interface and thus you couple to an external API of the users module rather than it's internals. That's ideal of course, but I dont know if such APIs exist. But it's something we can think about moving forward.

[craigh]

you still don't understand. did you try your POC? did it work?

[ghost]

yes. it's a copy paste from a working example using func=index page.

[craigh]

well that's good news. I'm going to look into solutions at the Entity level.

[ghost]

I'll also try to ask @guilhermeblanco by email if he has time to give advice. If there is a quick solution, I imagine he'll know immediately.

[craigh]

my thinking is to add some checks to the getUser() method in the Entity. If the user is not set, then construct a "fake" and return that instead.

[craigh]

if @guilhermeblanco actually looks here, the entity I'm talking about is this one: https://github.com/zikula-modules/DizkusModule/blob/master/Zikula/Module/DizkusModule/Entity/ForumUserEntity.php

[ghost]

Here is his answer

It's very easy to add this support.
It's all around you entity mapping. We can natively rely on RDBMS drivers to achieve this.
Here is a sample on how to achieve this inside of your ForumUserEntity:

/**
 * @ORM\ManyToOne(targetEntity="UserEntity", nullable=true)
 * @ORM\JoinColumn(name="user_id", referencedColumnName="id", onDelete="SET NULL")
 */
protected $user;

That should do the work.
Once querying, don't forget to use LEFT JOIN, otherwise it'd strip all ForumUserEntity where UserEntity is NULL. =)

[guilhermeblanco]

@drak that would not perfectly work in this file because it's also marked as an @ORM\Id field. You have to create an "id" field which will be your primary key, otherwise you'll end up having NULL in the user field, causing DB constraint issues.

[rmburkhead]

Be careful about substituting uname for uid as the id field. As I mentioned elsewhere, it is possible that unames can be reused if deleted. That renders them not unique over time.

[craigh]

@guilhermeblanco - if I do $forumUserEntity->getUser() what does Doctrine return if the associated entity is deleted but the column still contains the id value? I'm guessing either null or an exception?

[guilhermeblanco]

@craigh it should not happen, since you're the responsible to keep your Object Graph into a consistent state. This means you are responsible to trigger an update query if your user is deleted.

Using the excerpt I provided to @drak, we would rely on RDBMS to fix this for us. By adding an onDelete="SET NULL", this means that if the PK of User is removed, all ForumUserEntity will be updated setting the user_id to NULL when this user_id is the PK that you have removed, so you don't need to bother about it.

But supposing by any mean you forgot it and you have an exceptional situation where user does not exist, it does exist in a separate table, but your foreign key constraint got removed somehow. In this case, Doctrine will return you a Proxy of that User and once you attempt to initialize it, it'll throw a NoResultFoundException for you. =)

[ghost]

@guilhermeblanco the problem here is the relation to UserEntity is one outside of this module's control and jurisdiction. If a UserEntity is deleted, in this case, @craigh does not want to cascade the delete because he wants to keep the forum posts in-tact and just mark then as "unknown user". I've provided a different way to do it here but some entity level thing would be great. catching exceptions in the template is not as straightforward.

[craigh]

What I was hoping to do is something like this in the Entity:

public function getUser()
{
    try {
        $user = $this->user;
    } catch (EntityNotFoundException $e) {
        $user = 2; // actually I would create a "fake" user entity here
    }
    return $user;
}

but I just tested something like this and it doesn't work as I hoped

@guilhermeblanco - you're saying I cannot use the $user as the PK for any of these items to work... yes?

[ghost]

It wont because the proxy is wrapping it. Unless it's possible to override the proxy @guilhermeblanco ? I searched the docs and did see references for that.

[craigh]

right. I'm just seeing that myself now. $this->user is an uninitialized instance of the proxy class. What I'm wondering about is just testing for initialization. If not, then construct a fake.... possible?

[craigh]

I think this is sort of doing what I had in mind:

public function getUser()
{
    $props = $this->getReflection()->getDefaultProperties();
    if (isset($props['__isInitialized__']) && $props['__isInitialized__']) {
        $user = $this->user;
    } else {
        $user = new ZikulaUser();
        $user->setUid(1);
        $user->setUname('user deleted');
    }
    return $user;
}

[ghost]

i dont think so because the proxy is wrapping the class. You would need to be able to customise the proxy class.

[craigh]

I think this is sort of doing what I had in mind:

well it is sort of working...

[ghost]

Excellent if it works. Not sure how it does but excellent.

[craigh]

hmmm. not as well as I had hoped. the __isInitialized__ property seems to always be false :unamused:

[craigh]

ok, this is sort of working:

public function getUser()
{
    try {
        $uname = $this->user->getUname();
    } catch (\Exception $e) {
        $this->user = new ZikulaUser();
        $this->user->setUid(self::FAKE_USER_ID);
        $this->user->setUname('user deleted');
    }
    return $this->user;
}

[craigh]

Comments welcome on the PR.

[damon18]

In practice, how would the admin change the text "user deleted", just using translations or could there be an admin setting.

Also, this would set the same user deleted text for any missing user regardless of why they are missing.

Since you are using getUname to check if a user exists instead of the UID how would this affect the case where a users username has been changed? On a busy site I get requests to change members usernames about once a week.

[craigh]

In practice, how would the admin change the text "user deleted", just using translations or could there be an admin setting.

is there? no. could there be? yes, probably.

Also, this would set the same user deleted text for any missing user regardless of why they are missing.

Any deleted user - yes. same text.

Since you are using getUname to check if a user exists instead of the UID how would this affect the case where a users username has been changed?

It would not. The only time it will trigger this code is if the user is deleted entirely from the Core's UserEntity table.

[craigh]

fixed in #216

[craigh]

This may have only been working because the cache was not cleared. as soon as I (was forced to) cleared my cache there are a bunch of errors again. :disappointed:

[damon18]

I just tried to test this again with a fresh Dizkus install, but when I tried to delete the test user the user module threw out a page of errors. Starting with

Runtime Notice: call_user_func() expects parameter 1 to be a valid callback, non-static method Zikula\Module\DizkusModule\EventHandlers\SystemListeners::deleteUser() should not be called statically in C:\xampp\htdocs\core\src\vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\EventDispatcher.php line 164
500 Internal Server Error - ContextErrorException

Should I open an issue in Core?

[craigh]

update and try again.

[craigh]

I've still not come up with any solutions to this other than reassigning all posts to another user. I'm thinking assigning to "anonymous" would work (uid=1)

[ghost]

Let me throw some more fuel onto the fire.

I see a lot of problems in the future for any modules that couple themselves to anther module's internals. Making a direct relation to another module's entities is a recipe for disaster and difficulty. Now you would be expecting the module to maintain BC on it's entities - but they are not a pubic API and it's not a reasonable assumption to make. Unlike class inheritance, there is no way to make an entity invisible, but in any case, a module that makes this kind of coupling is always going to have to play catch up. There surely must be a better way.

I've refrained from posting this before, but I'm looking into the future and trying to minimise difficulties in the future.

[craigh]

@damon18 - please test again. I think I've got it the way it should be.

[craigh]

@damon18 - please test again. I think I've got it the way it should be.