Trait method collision with SoftDeletes

[seancheung]

I have this model:

class Admin extends Authenticatable
{
    use Notifiable;

    use SoftDeletes;

    use NtrustUserTrait;
}

And I noticed that there is a collision between NtrustUserTrait and SoftDeletes: the restore method, which both traits has overridden from Model.

Am I doing wrong or is this a bug?

[klaravel]

@theoxuan Is there any ways can you share code with me?

[seancheung]

Yes, but what I mean is that the NtrustUserTrait has a collision with laravel's built-in SoftDeletes trait. It's because both of them have a method called restore - overrided from Model.

This is Laravel's SoftDeletes.php

<?php

namespace Illuminate\Database\Eloquent;

trait SoftDeletes
{
    //omitted

    /**
     * Restore a soft-deleted model instance.
     *
     * @return bool|null
     */
    public function restore()
    {
        // If the restoring event does not return false, we will proceed with this
        // restore operation. Otherwise, we bail out so the developer will stop
        // the restore totally. We will clear the deleted timestamp and save.
        if ($this->fireModelEvent('restoring') === false) {
            return false;
        }

        $this->{$this->getDeletedAtColumn()} = null;

        // Once we have saved the model, we will fire the "restored" event so this
        // developer will do anything they need to after a restore operation is
        // totally finished. Then we will return the result of the save call.
        $this->exists = true;

        $result = $this->save();

        $this->fireModelEvent('restored', false);

        return $result;
    }

    //omitted

    /**
     * Determine if the model is currently force deleting.
     *
     * @return bool
     */
    public function isForceDeleting()
    {
        return $this->forceDeleting;
    }

    //omitted
}

And here is the provided trait NtrustUserTrait.php

<?php namespace Klaravel\Ntrust\Traits;

use Illuminate\Cache\TaggableStore;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Config;
use InvalidArgumentException;

trait NtrustUserTrait
{
    //omitted

    public function delete(array $options = [])
    {   //soft or hard
        parent::delete($options);
        if(Cache::getStore() instanceof TaggableStore) {
            Cache::tags(Config::get('ntrust.profiles.' . self::$roleProfile . '.role_user_table'))->flush();
        }
    }
    public function restore()
    {   //soft delete undo's
        parent::restore();
        if(Cache::getStore() instanceof TaggableStore) {
            Cache::tags(Config::get('ntrust.profiles.' . self::$roleProfile . '.role_user_table'))->flush();
        }
    }

    //omitted

}

There's a public method isForceDeleting() which you can use in observers to tell if it's a soft deleting. So there is no need to override restore() and delete() since that will cause a conflict with SoftDeletes trait if the model has used both traits(which in my case).

This is how I do in an registerd observer:

<?php

namespace App\Observers;

use App\User;

class UserObserver
{
    //omitted    

    public function deleting(User $user)
    {
        if($user->isForceDeleting()) {
            //hard deleting
            info('hard');
        }
        else {
            //soft deleting
            info('soft');
        }
    }
}

Another approach is to use SoftDeletes trait in a parent model.

[klaravel]

I found issue. I will update you code or give you solution soon.

[klaravel]

@theoxuan

Got solution. Use below code and it will well. Let me know if it will generate other issue.

Replace:

// use SoftDeletes;
// use NtrustRoleTrait;

With:

use SoftDeletes, NtrustRoleTrait
{
    SoftDeletes::restore insteadof NtrustRoleTrait;
    NtrustRoleTrait::restore insteadof SoftDeletes;
}

[seancheung]

Thanks for your solution but I think you are erasing restore() from both traits.

I did a test:

<?php

trait A
{
    function restore()
    {
        echo "A";
    }
}

trait B
{
    function restore()
    {
        echo "B";
    }
}

class Model {
    function restore()
    {
        echo "Model";
    }
}

class User extends Model {
    use A, B {
        A::restore insteadof B;
        B::restore insteadof A;
    }
}

$user = new User;
$user->restore(); //result is 'Model'

[seancheung]

This one will work:

<?php

trait A
{
    function restore()
    {
        parent::restore();
        echo "A";
    }
}

trait B
{
    function restore()
    {
        parent::restore();
        echo "B";
    }
}

class Model {
    function restore()
    {
        echo "Model";
    }
}

class BaseUser extends Model {
    use A;
}

class User extends BaseUser {
    use B;
}

$user = new User;
$user->restore(); //result is 'ModelAB'

[klaravel]

But there is one problem class Model don't have restore() method.

[seancheung]

Hmm sorry I forget that.

In our condition it should be

<?php

trait A
{
    function restore()
    {
        echo "A";
    }
}

trait B
{
    function restore()
    {
        echo "B";
    }
}

class Model {

}

class User extends Model {
    use A, B {
        A::restore as restoreA;
        B::restore as restoreB;
    }

    function restore(){
        $this->restoreA();
        $this->restoreB();
    }
}

$user = new User;
$user->restore(); //result is 'AB'

[klaravel]

Great. That one will work perfectly.

Thanks for solution. I did not test on my solution.

I will update document.

Let me know if you will find any other solution directly on trait then I will apply on that on.

I might use restore event $this->fireModelEvent('restored', false); and it will work.

[seancheung]

There is one more thing. Why is there a parent::restore() call in NtrustUserTrait::restore ?

[klaravel]

I have actually copied this package from other and fix many laravel 5.0 to 5.3 bugs.

So no idea why they calling parent::restore() event parent don't have restore method. And I did no focus for testing soft delete so missed that area.

I will update this package in day or two with event thing. It will fix all this issue.

Or you can fork package and make pull request and I will merge it.

[seancheung]

Pull request created.

Closed.

[klaravel]

Hey @theoxuan

Thanks for pull request. I have merge that one and applied other setting for all traits also now you can use normal.

use SoftDeletes;
use NtrustUserTrait;

Can you please try with "klaravel/ntrust": "dev-master" one and let me know if it's work for you with caching.

Thanks for fix and help to build great package.

[seancheung]

Sorry dude I forgot to give you my feedback.

This package is just not Laravel-5.3 ready. A lot of internal function calls are broken.

In the end I implemented my own permission controlling system.

Thanks anyway!

[ajmerainfo]

Hi @seancheung Thanks for feedback.

It would be great if you can give me one or two example like what thing is not working.

I am using in my live project this roles and permission things and working great for me but if it has any bug then I can fix it.

[seancheung]

@ajmerainfo
I actually created my own package:
panoscape/privileges

It's inspired by your package(and the original package you forked).

[ajmerainfo]

Great. Thank you for information.

[seancheung]

Dude I just found a method named bootTraits when digging Laravel's Mode.php.

I think it's better to stop overriding boot method in traits, instead name it as bootTraitName(TraitName is the name of the trait) and it will be called when the model is being booting.

Here is how it's handled in Model.php

    /**
     * The "booting" method of the model.
     *
     * @return void
     */
    protected static function boot()
    {
        static::bootTraits();
    }

    /**
     * Boot all of the bootable traits on the model.
     *
     * @return void
     */
    protected static function bootTraits()
    {
        $class = static::class;

        foreach (class_uses_recursive($class) as $trait) {
            if (method_exists($class, $method = 'boot'.class_basename($trait))) {
                forward_static_call([$class, $method]);
            }
        }
    }

Also do not override model events but listen to them. Register listening in bootTraitName.

This will completely remove those collisions with ease.

[klaravel]

@seancheung

Good to know. Thank you for great information.

I will implement that one in this package.

About model event I am using like below is that perfect?

static::saved(function()
{
    if(Cache::getStore() instanceof TaggableStore) {
        Cache::tags(Config::get('ntrust.profiles.' . self::$roleProfile . '.role_user_table'))
            ->flush();
    }
});
static::deleted(function($user)
{
    if(Cache::getStore() instanceof TaggableStore) {
        Cache::tags(Config::get('ntrust.profiles.' . self::$roleProfile . '.role_user_table'))
            ->flush();
        $user->roles()->sync([]);
    }
});
if(method_exists(self::class, 'restored')) {
    static::restored(function($user)
    {
        if(Cache::getStore() instanceof TaggableStore) {
            Cache::tags(Config::get('ntrust.profiles.' . self::$roleProfile . '.role_user_table'))
                ->flush();
            $user->roles()->sync([]);
        }
    });
}

[seancheung]

It is alright. But I think a separate observer would be cleaner:

<?php

namespace Your\Package;

class YourModelObserver
{
    /**
    * Listen to the Model saved event.
    *
    * @param  mixed $model
    * @return void
    */
    public function saved($model)
    {
        //
    }

    /**
    * Listen to the Model deleting event.
    *
    * @param  mixed $model
    * @return void
    */
    public function deleting($model)
    {
        if($model->forceDelete()) {
              //softdelete
        }
        else {
           //hard delete
       }
        //
    }

    /**
    * Listen to the Model restored event.
    *
    * @param  mixed $model
    * @return void
    */
    public function restored($model)
    {
        //
    }
}

Just register it to the model like this

<?php

namespace Your\Package;

trait YourModelTrait
{
    public static function bootYourModelTrait()
    {
        static::observe(YourModelObserver::class);
    }
}

With this you don't event need to check method existence(like restored in SoftDeletes trait) .

Another suggestion is to pick Cache to separate trait.

I think there are some problems(or my misunderstanding?) with the logic in your code.

$user->roles()->sync([]); detaches all roles of the model, why do you call them in restored?
I think it should be:

• when deleting, if the model uses SoftDeletes, keep the relationships, flush the cache;
• when saved, flush the cache;
• when restored, never mind about the relationships, just flush the cache.