Error when deleting a model

[danielbehrendt]

When trying to delete a model and the virtual column contains any JSON an QueryExceptions is thrown. It seems to be caused by line 87:

$result = parent::fireModelEvent($event, $halt);

Changing the line as follows removes the exception after deleting a model:

$result = ('deleted' !== $event) ? parent::fireModelEvent($event, $halt) : null;

[danielbehrendt]

The previous mentioned code change only fixes the QueryException but is has some side effects. When using in combination with https://github.com/richan-fongdasen/eloquent-blameable the deleted_by won't get written to DB.

Update:

In my case (used with Eloquent Blameable) the problem could be fixed with the following updated fireModelEvent method:

protected function fireModelEvent($event, $halt = true)
{
    $this->decodeIfEncoded();

    if ('deleted' === $event) {
        self::encodeAttributes($this);
    }

    $result = parent::fireModelEvent($event, $halt);

    $this->runAfterListeners($event, $halt);

    return $result;
}

[stancl]

Can you explain exactly when does this happen?

I use this in stancl/tenancy and deleting tenants works just fine.

[danielbehrendt]

@stancl Do you use also the "Eloqunet Blameable" trait? As I mentioned this exception occurs in combination of both packages. I guess the problem is that the blameable trait is calling getDirty method (see: https://github.com/richan-fongdasen/eloquent-blameable/blob/1f065b32e589fddf79ad13516e0055cd8bb75599/src/BlameableTrait.php#L118) and if the model contains the "virtual" attributes the generated query throws and exception (when trying to update).

[stancl]

It doesn't. And are you sure the issue is with this package? Rather than the blameable one

[danielbehrendt]

Yes, I'm sure, because if I don't encode the attributes in case of deleted event the following error occurs:

Illuminate\Database\QueryException
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'manufacturer' in 'field list' 
(SQL: update `articles` set `data` = ?, `deleted_by` = 1, `manufacturer` = ?, `recommended_retail_price` = ?, `vat` = ?, `weight` = 11, `width` = 10, `height` = 20, `length` = 30, `search_keywords` = ? where `id` = 2)

The getCustomColumnsof the model looks like this:

public static function getCustomColumns(): array
{
    return [
        'id',
        'name',
        'created_by',
        'updated_by',
        'deleted_by',
        'created_at',
        'updated_at',
        'deleted_at',
    ];
}

The update query contains columns which are not real columns in the database table and the population of these columns is done by your trait.

[stancl]

Okay.

I can't work on this myself, so if you want this changed, please send a PR.

I'll accept a PR assuming:

1. A regression test is added, ensuring this behavior is fixed forever
2. Existing tests pass
3. stancl/tenancy tests pass (docker-compose up -d && ./test) when you use your modified version of this package - please present a screenshot showing this

Sorry for the complex requirements, but my main priority with this package is to support stancl/tenancy and I don't want to risk breaking any apps just to support some edge case that's not even used by my package.

Thanks!