fix(database): handle virtual properties when generating a query from a model

[mattdinthehouse]

My bad - in #966 I only did the "read" part, this PR does the "write" part that actually threw the original error in #962

[brendt]

I think this isn't the right approach, since readonly properties should also be writeable. The thing with the mapper is that it allows to create partial objects (which is why the constructor is skipped, you asked about it in #969 (comment) as wel). We allow partial objects because we support lazy relations.

Anyway, long story just to say that, with Tempest's mappers, there could be scenarios where a readonly property isn't written to when the object was initially constructed, but it did load afterwards. So we cannot simply expand the readonly check. Shouldn't it work without those readonly property changes? Also we'd need a test :)

[mattdinthehouse]

Sorted - I've separated out the "is writable" check into it's own method.

I realise that part's semi-unrelated to the Virtual attribute but it's to avoid trying to set the value on a hooked property with no set handler on it when reloading the model and copying the properties over, which is the scenario I have that inspired this whole thing in the first place 😅

Actually just thinking about it now, because there's likely to be logic on hooked properties should we even be trying to copy them over like this in the load() method?

[brendt]

Actually just thinking about it now, because there's likely to be logic on hooked properties should we even be trying to copy them over like this in the load() method?

Isn't that what we're preventing with the added isWritable check? Maybe I misunderstand what you mean though

[mattdinthehouse]

Isn't that what we're preventing with the added isWritable check?

The isWritable method will return true if there's a "set" hook available, so it would run whatever logic is in there

So both of these cases:

public string $foo;

public string $bar = {
    set(string $value) {
        $this->bar = strtoupper($value);
    }
}

but not for this one because there's no setter:

public string $bar = {
    get => 'haha'
}

which might lead to unexpected behaviour if the set hook does something less trivial - I wouldn't want someone to call $myModel->load('relation') and get a corrupted model.

I don't know the background about why IsDatabaseModel::load() recreates the model and copies over the values so I'll need your guidance on when/if/how to skip hooked properties regardless of whether they're marked Virtual or not

[brendt]

Gotcha, thanks for explaining!

I don't know the background about why IsDatabaseModel::load() recreates the model and copies over the values

Convenience at the time 😇

I'll need your guidance on when/if/how to skip hooked properties regardless of whether they're marked Virtual or not

I don't think we need to explicitly do anything with them for now. If people decide to add a set hook for a relation, I'd say they are on their own 😅

Would you be able to add a test for this one?