Don't break with `Model::shouldBeStrict()` enabled

[nick-potts]

see this test for what is this fixes:

https://github.com/nick-potts/virtualcolumn/blob/bd7097323d3d6dc45e418363562bfc13b56bcce6/tests/VirtualColumnTest.php#L133

[stancl]

Does this PR address the same thing as this?

protected static $modelsShouldPreventAccessingMissingAttributes = false;

[nick-potts]

Yes and no. I have strict enabled project wide in dev, as is somewhat common.

I would prefer strict stays enabled so the model behaves the same as everything else in my project, but there's some personal preference in there.

[stancl]

Would need a more detailed description of what exactly this PR is solving then, the test doesn't add much info.

[nick-potts]

Will in the AM

[nick-potts]

In my case, I have Model::shouldBeStrict() enabled.

Here's a quick explanation of the specific one that's causing this issue: https://planetscale.com/blog/laravels-safety-mechanisms#attribute-typos-and-renamed-columns

For example, I do have this on my user model, and I would like to ensure that custom columns are indeed loaded. The issue is, I'm not able to do User::pluck('id)->toArray(), as this package tries to decode an unloaded data column.

[stancl]

I see. The snippet I posted above is what we use in Tenancy for Laravel for use cases where you might do e.g.:

if ($tenant->logo) { ... }

as a sort of "has attribute" check.

The case you're describing here is about models that haven't been saved to the DB yet? Or I guess when they're being created at first. Once they're pulled from the DB, the data column should always be initialized since it should have a default value of an empty JSON object.

[nick-potts]

I'm purely running a Order::query()->pluck('id');

the only attribute that is fetched by the database is the id, but this package is trying to decode the data column and it causes it to break.

The test I wrote demonstrates that behaviour.

[stancl]

I see now, so some ways of fetching the model completely omit the data column. Could we use hasAttribute() or something similar for an early return instead of the try-catch?

[nick-potts]

Actually just checking the column exists and returning early in the retrieved event listener works. Would also help stop its current behaviour of silently failing when someone does a pluck('custom') or a get(['custom'])

[stancl]

Actually just checking the column exists and returning early in the retrieved event listener works. Would also help stop its current behaviour of silently failing when someone does a pluck('custom') or a get(['custom'])

Can you expand on the behavior difference? What you mean by silently failing specifically, compared to the behavior after the change

[nick-potts]

When you do a pluck('id') call, the current behaviour is to always decode no matter what data came from the database.

So the decodeVirtualColumn will always run, the getAttribute(static::getDataColumn()) will return null and get coalesced to [].

This change now checks if the data column exists before running decodeVirtualColumn.

[nick-potts]

An unintended behaviour of this is that you can accidentally wipe all the custom data from a column without being careful.

        FooModel::create([
            'id' => 1,
            'virtual' => "data I don't want to lose" // in the `data` column
        ]);

        $foo = FooModel::query()->first(['id']);
        $foo->bar = 'baz'; // save to the `data` column
        $foo->save();

        $foo = FooModel::query()->first();
        $this->assertSame('data I don't want to lose', $foo->virtual);
    }

This would actually fail because only bar would be persisted.

I've added a proof of concept that checks if the data was loaded from the database. There might be a better way to do it.

[stancl]

That makes sense. So in the try-catch implementation, the data attribute got added to Thing::pluck('id'), decodeVirtualColumn() failed, leaving data null, which got cast to [] and would result in the data being lost if save() was called on that model.

Whereas now with the listener not being triggered, the data column doesn't get added at all?

Though then I don't get what the last change is for, I think I'm misunderstanding which implementation (if any) you're saying would prevent the data column from getting set to [].

[nick-potts]

The last change marks when you choose to selectively pull columns from the database, doesn't decode and prevents you from trying to write when the data column when saving.

[nick-potts]

I'd be fine with not including the save side of things