XF 2.2 $entity->isChanged('is_banned') not working as expected

[mcatze]

I build a static function in Listener where i ask if $entity->isChanged(['user_group_id', 'secondary_group_ids']) and $entity->isStateChanged('user_state', 'disabled') === 'enter', now i just want to know if a user is set banned.

How can i get this?

I tried $entity->isChanged('is_banned') and $entity->isStateChanged('is_banned', '1') === 'enter' but it doesn't work.

 

[Jeremy P]

This seems to work roughly as expected in my testing. Though it's maybe worth noting that \XF\Mvc\Entity\Entity::isStateChanged is primarily for checking for changes on state columns where there are a handful of different states (visible, moderated, deleted, for example). Since this column is a simple boolean, you could get away with:

if ($entity->isChanged('is_banned') && $entity->is_banned) {
    // ...
}

 

[mcatze]

@Jeremy P thanks for your reply. I've tried this code in Listener.php but it seems not working correctly.

public static function entityPostSaveUser(\XF\Mvc\Entity\Entity $entity)
{
    if ($entity->isChanged('is_banned') && $entity->is_banned)
    {
        if (!$entity->canViewXtMembermap() AND $entity->Profile->xt_mm_show_on_map)
        {
            $entity->removeLocationData(true);
        }
    }
  
    if ($entity->isChanged(['user_group_id', 'secondary_group_ids']))
    {
        if (!$entity->canViewXtMembermap() AND $entity->Profile->xt_mm_show_on_map)
        {
            $entity->removeLocationData(true);
        }
    }

    if ($entity->isStateChanged('user_state', 'disabled') === 'enter')
    {
        if (!$entity->canViewXtMembermap() AND $entity->Profile->xt_mm_show_on_map)
        {
            $entity->removeLocationData(true);
        }
    }
}

When i change the usergroup or set user_state to disabled all the locationData will be removed. But when i ban a user, all data is still there.

 

[Kirby]

Hint: \XF\Entity\User::_postSave()

if ($this->isChanged(['user_group_id', 'secondary_group_ids', 'permission_combination_id', 'user_state']))
{
    // TODO: this is mostly temporary -- this should happen at the getter level
    $this->clearCache('PermissionSet');
}

PS: I'd simplyfy the code and not mix AND and &&

 

[mcatze]

I'd simplyfy the code

What would you sugessting?

and not mix AND and &&

I see this right now..

 

[Kirby]

What would you sugessting?

I'd probbably use smth. like

$checkPermissions = (
    ($entity->isChanged('is_banned') && $entity->is_banned))
    || $entity->isChanged(['user_group_id', 'secondary_group_ids'])
    || $entity->isStateChanged('user_state', 'disabled') === 'enter'
);

if ($checkPermissions
    && $entity->Profile->xt_mm_show_on_map
    && !$entity->canViewXtMembermap()
)
{
    $entity->removeLocationData(true);
}

 

[mcatze]

That makes sense. Thanks for your help.

 

[Kirby]

Just curious:
Why are you using a code event listener if you are extending the User entity anyway?
Doesn't seem to make much sense to me.

Btw: I'd probably move code to delete images to the User Delete Cleanup Service instead of calling from the entity.
For bulk deletes and remote storage (S3, etc.) this could be kinda slow and cause timeouts - the cleanup service does get executed via a resumable job.

 

[Lukas W.]

@Kirby resource standards point 20. If you're only extending stuff of an entity that can be handled via listeners, theoretically you should do that.

 

[Kirby]

Yeah, absolutely. But that listener is for XF\Entity\User and the Add-on already extended the entity with methods removeLocationData and canViewXtMembermap in which case according to @Chris D it would be fine not to use a listener (which would cause a bit more overhead than just the extension which is required anyway)

 

[mcatze]

This is a interesting discussion. @Hoffi says the same like @Lukas W. but on what point it is correctly?
I totally understand your point @Kirby and now i am really confused about that..
Otherwise, it is kind of funny that 3 german guys discuss here in english.

 

[Hoffi]

Lets squeeze in a forth german guy.

Correct is both, as mostly in development issues.

Use a Listener is closer to the XF coding standards.
User it in the entity can make a bit faster.

I always try to avoid extending the save classes if possible to prevent possible issues like we had with the other AddOn what results in a double call of the preSave Event which leds to multiple unneccesary API calls.

For both versions are valid reasons to do it on that way.

 

[Chris D]

Yeah, absolutely. But that listener is for XF\Entity\User and the Add-on already extended the entity with methods removeLocationData and canViewXtMembermap in which case according to @Chris D it would be fine not to use a listener (which would cause a bit more overhead than just the extension which is required anyway)

Yeah if you already have a class extension it is fine to use it instead of a event listener.

 

[mcatze]

So in this case i can extend also protected functions like _postSave or _postDelete?

protected function _postDelete()
{
    parent::_postDelete();

    return $this->removeMinimapIfExists();
}

 

[Chris D]

Yes that’s right.

Minor point but you don’t need to return anything. We don’t expect the _postX methods to return any value so any value you return won’t be used by anything.

Doesn’t really impact anything but worth keeping in mind. If we had strict return types expected here then it could be problematic.

 

[Lawrence]

So in this case i can extend also protected functions like _postSave or _postDelete?

protected function _postDelete()
{
    parent::_postDelete();

    return $this->removeMinimapIfExists();
}

This is how I do it to delete a members message (post) banners, with one difference: I call the parent last as it initiates a job manager at the end.

    protected function _postDelete()
    {
        $messageBanner = $this->app()->service('EAEAddons\MessageBanner:User\MessageBanner', $this);
        $messageBanner->deleteBannerForUserDelete();

        parent::_postDelete();
    }

 

[Kirby]

Use a Listener is closer to the XF coding standards

Is it?

XFMG\XF\Entity\Attachment::getStructure()
XFMG\XF\Entity\Post::getStructure()
XFMG\XF\Entity\User::getStructure()

User it in the entity can make a bit faster.

• All code related to modifications on the entity is in one file instead of (possibly) being scattered over multiple files
• Static methods do make Unit-Tests hard(er)