Use increment when saving a new view

[iPurpl3x]

@imorland I made this small change, as I think it is first more safe and also less code. I didn't execute any tests, as I did this change within the browser and didn't even clone the repo locally, but it should work just fine.

[luceos]

Care to explain why this is more safe?

[iPurpl3x]

I don't know if this code will ever be executed in a parallel fashion, but if it is, it would be unsafe to not use increment as there could be situations where we try to add a view in two parallel processes and then end up with only one view added. Does that make sense ?

[luceos]

This is part of the logic behind increment (which also supports mutators):

        $this->{$column} = $this->isClassDeviable($column)
            ? $this->deviateClassCastableAttribute($method, $column, $amount)
            : $this->{$column} + ($method === 'increment' ? $amount : $amount * -1);

So it does exactly the same thing.

If we would want to support highly volatile environments it would make more sense to directly interact with the database with something like this:

Discussion::where('id', $current_discussion->id)->increment('view_count');

As the logic behind this is:

        $wrapped = $this->grammar->wrap($column);

        $columns = array_merge([$column => $this->raw("$wrapped + $amount")], $extra);

        return $this->update($columns);

Which is effectively a query on the database.

---

I'm not trying to be a pain, just showing that diving into the framework sometimes helps understand what is performant and what is not.

[iPurpl3x]

We're getting deep here @luceos 🧐...

I actually tested this and looked into the MySQL logs to see if there is a difference, and there is:

(initial view_count is 20)

$current_discussion->view_count++;
$current_discussion->save();

Will result in

update `discussions` set `view_count` = 21 where `id` = 1; -- ⚠️ unsafe

And

$current_discussion->increment('view_count', 1);

Will result in

update `discussions` set `view_count` = `view_count` + 1 where `id` = 1; -- ✅ safe

[luceos]

Oh yeah, I see it now:

        $query = $this->newQueryWithoutRelationships();

        if (! $this->exists) {
            return $query->{$method}($column, $amount, $extra);
        }

        $this->{$column} = $this->isClassDeviable($column)
            ? $this->deviateClassCastableAttribute($method, $column, $amount)
            : $this->{$column} + ($method === 'increment' ? $amount : $amount * -1);

        $this->forceFill($extra);

This only seems to happen if the current model does not exist, it falls back on the same logic I proposed. If this seems to be the case for simple discussions view, then we should move to your proposed logic 👍

Thanks for explaining and diving into this with me 👍