Make SoftDeleted relationships more "visible" to developers

[dfidler]

PROBLEM STATEMENT:

SoftDeletes is just just a "scope" for a model and that works very well when developers are querying for that model directly.

However, if you start using SoftDeletes in your app, you're inevitably going to get to the point where every single model has at least one relationship with a soft deleted record.

There are several challenges with this:

Difficult to Debug

All of these lines of code behave differently:

# return $user->tenant_id;    # returns ID
# return $user->tenant->id;  # causes stacktrace
# return $user->tenant?->id  # returns null
# return $user->tenant()->withTrashed()->first()->id;  # returns id
# return $user->tenant()->exists();  # returns false, DB query

Unfortunately, because SoftDeletes is just filtering/scoping on the deleted_at column, there is no way to differentiate between a relationship that's been scoped or SoftDeleted, and those two use cases are, semantically, VERY different.

It's easy for a developer to think "ah, that column is not null so...." they don't handle it. When it goes wrong (not if) you get a stacktrace in the logs saying ".ERROR: Attempt to read property "id" on null", which isn't informative or helpful and it causes a failure in production.

No easy way to check for deleted relationships

Even if your developers do think to check for scoped/soft deleted data, chances are, your model has already been through with/load/loadMissing so you now have to go back to the DB to get the data like this:

$user->tenant()->withTrashed()->first();

Of course, you can always add "withTrashed()" to the relationship but that's a bit like smashing a walnut with a nuke and it will, eventually, lead to a bug that shows soft-deleted data to the client because now you have to iterate through each result (in the case of a HasMany, BelongsToMany, etc) to check for trashed();

public function tenant() {   return $this->belongsTo(Tenant::class)->withTrashed();  ]

Or you can handle it on a case-by-case basis when eager loading, per:

$user->with(  ['tenant' => fn($q) => $q->withTrashed() ] );

This works but, again, you have to account for every instance and you're team is going to miss some of these cases and you're going to get failures.

SOLUTION(s)

I'm actually struggling with how to solve this and the following is more of an "is there a better way" so I'd love to hear other people's thoughts on it.

The problem I see is that soft-deleted records are currently filtered out of the result set. For Laravel to be able to intelligently handle soft-deleted relationships, it would need to query the soft-deleted data (on some level, even if it's only to return a single column saying "deleted") during the query and that might also incur performance issues in some circumstances.

One thought I had was to provide a mechanism for this; something akin to:

public function tenant(): {
   return $this->belongsTo(Tenant::class)->withTrashed(exceptions: boolean = false)
}

Then, if the code accesses a soft-deleted relationship ($user->tenant->name) it would immediately throw an "AccessScopedModelException(Model $parent, Relationship $relationship, EloquentScope $scopeClass)" (or maybe it just throws an AccessSoftDeletedModelException)

Notes:

• I know that "EloquentScope" doesn't exist as a common ancestor today, but you get the point.
• The problem with SoftDeletes being a trait, is that determining if a Model uses a specific trait is possible in PHP but it's non trivial (and clunky)

This does two things:

1. The error message in laravel.log is absolutely clear what the issue is because you're told the model that owned the relationship, the relationship that failed and the Scope that caused the failure.
2. Resolving the issue is as simple as adding try / catch around your code

It would also allow us to do easy checks like $user->tenant->isTrashed() (or ->isOutOfScope(null|string|EloquentScope $scopeClass = null)) to determine if the record is intentionally scoped out.

I LOVE exceptions (and I know many don't so I'm sure some will hate the above, which is why I suggest making it optional) but I'm open to alternatives.

But my ultimate goals are to make SoftDeleted accesses explicit and easy to handle by developers when it inevitably goes wrong.

[bulletproof-coding]

I get your pain. If I would had been forced into soft deletes, I would had created special relations that fetch only deleted or only not deleted rows, without scopes because scopes are applied or not on some cases.

I encountered similar issue regarding parent_id and I had to condition always the parent_id column. I never use scopes by the way.

[dfidler]

I would had created special relations that fetch only deleted or only not deleted rows

Interesting idea. I'll have to think about that. I don't think that helps in this situation though.

I never use scopes by the way.

I was skeptical of scopes at first but they are both a great security feature and make for readable code.

Example: We have an event calendar with complex logic for visibility of records. We built that into a global scope so no request will ever return events a user can't see. It's also much cheaper/faster than a 'view' policy or building a service class or putting it in a controller (laravel-actions makes this less onerous as that pattern is sooooooo much better than the Controller pattern).

It has made all of the code around events completely painless and the Devs can't leak event data without explicitly removing the global scopes. We have a few examples like that in our app. It's great.

SoftDelete is a great idea and if you're querying a soft deleted model it's awesome... But soft deleted relationships have a bit of code smell. 🙃

[bulletproof-coding]

This is a case when scopes are not applied #51825

[bulletproof-coding]

@dfidler

But soft deleted relationships have a bit of code smell. 🙃

If the soft deleted row is in a pivot or in a through table then you have to put conditions also on the pivot, so the only way possible is the relation defined with conditions in it.

    public function examples(): BelongsToMany
    {
        return $this->belongsToMany(
            Model::class, // 4
            (new Model())->getTable() . ' as t', // 2 & 3
            't.col2', // 2
            't.col3', // 3
            'id', // 1 $this
            'col3' // 4
        );
    }

    public function validExamples(): BelongsToMany
    {
        return $this->examples()->wherePivotNull('t.deleted_at');
    }
    
    public function deletedExamples(): BelongsToMany
    {
        return $this->examples()->wherePivotNotNull('t.deleted_at');
    }

[shaedrich]

One could probably implement a "wrapper object" (SoftDeletedModel) that would hold no columns (or nullable or throwable accessors or the like) or an empty "relation" to the actual model (that might not be empty when using withTrashed()) 🤔

[dfidler]

One could probably implement a "wrapper object" (SoftDeletedModel) that would hold no columns (or nullable or throwable accessors or the like) or an empty "relation" to the actual model (that might not be empty when using withTrashed()) 🤔

I really like that idea and the "accessors that throw" would be a good solution because it would only take the extra hit for those records that are deleted (presumably it would be an descendant of the Model class)

And that wouldn't be a huge change to the current implementation... because if you use the ->withTrashed() it removes the scope and if you did ->withTrashed(exceptions: true) (or whatever), it wouldn't apply the scope but it would post-process the data to find the deleted records and return the SoftDeletedModel.

@bulletproof-coding 's comments about soft-deleted pivots is also a concern, but one problem at a time.

I really like it. 👍

_Note: the more I think about the "AccessScopedModelException" idea, the more I think it's a bad idea... but only because you can have multiple "scopes" on a query... so in that case, how do you figure out which scope caused the rejection of a query. _

You'd have to query all data, then (in post) identify each of the scopes that apply to the record using collection queries.... except that the collection query capability isn't as rich as Eloquent's. It's not impossible but too much processing for the value it brings.

But I do think that "Deleted" records and "Scoped" records are semantically different...