Do not mutate Field::default/system property in scope condition

[mvorisek]

see https://github.com/atk4/data/pull/660/files/e1e0ae7947f60c268245e51ca12eafa5a45810dd#diff-d11d2f7d35e15c2e79f43737267e1c00

and related code:

data/src/Model/Scope/Condition.php

Lines 101 to 115 in 2f89724

	// if we have a definitive scalar value for a field
	// sets it as default value for field and locks it
	// new records will automatically get this value assigned for the field
	// @todo: consider this when condition is part of OR scope
	if ($this->operator === '=' && !is_object($this->value) && !is_array($this->value)) {
	// key containing '/' means chained references and it is handled in toQueryArguments method
	if (is_string($field = $this->key) && !str_contains($field, '/')) {
	$field = $model->getField($field);
	}
	if ($field instanceof Field) {
	$field->system = true;
	$field->default = $this->value;
	}
	}

[DarkSide666]

It's how it was before - BC way of doing things.
But of course we should revise that and see if that's still good approach or need to move that somehow to UI only.
In case of nested conditions this start to look less usable.

[mvorisek]

you are right, this is actually hide field (because of system) with nested conditons like (a > 5 or (a = 5)) (the code will trigger becauseof = - but in whole complex it is not only one possible value)

[georgehristov]

It can be implemented to be set to the value only when the condition is definitive - all parent scopes up to root scope have AND junction.

We have to look into alternatives to set it as well.

[mvorisek]

one possibility, but this can imply default arguments for API etc., which is not fully correct (too implicit)

I think we should drop it - what code relies on it?

[georgehristov]

The issue is that this functionality is used on model references.
E.g if removed this line (among many others) fails.

$a = new Account($this->db);

$a->save(['name' => 'AIB']);
$a->ref('Payment')->save(['amount' => 10]);

[mvorisek]

please post PR to discuss more concrete - save() works on either loaded row/ID or a new one, right?

so if this is only a ref issue, we should be addresses there - you mean the issue is that you will otherwise have to set payment_id on a new row?

[georgehristov]

The issue comes from sub-scoping models like here. So new entries are expected to have the value assigned permanently.
The only way I come up is to introduce a separate method on Model to set a condition and default field value.
Other ideas?

[mvorisek]

sub-scoping model

This is quite legit argument - only in this case it can be changed to system

But = equal condition definitely does not imply that.

I think, we have only two options:

• leave this to the user, so he can set system/default manually (in model init)
• add protected method addEqualToSystemCondition($field, $value) which will allow only this input and a) call setConditon($field, '=', $value) and b) set the system/default. Name is for discussion.

[georgehristov]

What we can do is check if condition is definite (all parents have AND junction) and only then set the value as deault and lock the field.
This is in the spirit of implemented functionality already.

Quite easy to implement and BC

[mvorisek]

we can not - scope can be further modified

[georgehristov]

Question is if that changes the definitiveness of the condition.
When adding conditions definitiveness does not change.
Only negation can change definitiveness and we can disable negation once scope assigned to a model.
Then we dont need RootScope at all. Model can be assigned to owner property.

[mvorisek]

The only usecase I see is strictly "helpful/simplified logic for conditions creation in model setup"

so I stay firm behind my "two options" - drop completely or allow to define condition with full control - like proposed addEqualToSystemCondition - as to be available at Model level only, it will always add a AND conditon to the whole resulting where.

my question is - what does this approach NOT solve?

[georgehristov]

to be available at Model level only
The logic is for sub-scoping and this devs should be able to achieve inline as well.

$invoice = (clone $document)->addCondition('type', 'invoice');
$invoice->{do whatever};

When adding it should save type invoice as it is now.

what does this approach NOT solve?

• huge BC-break
• introduces second slightly different way of sub-scoping which will cause confusion. How to define the difference between addCondition when it is definite and addEqualToSystemCondition (or however we call it)?

[mvorisek]

How to define the difference between addCondition when it is definite and addEqualToSystemCondition (or however we call it)?

as addEqualToSystemCondition will call addCondition, it behaves 1:1, only with addEqualToSystemCondition there will be side effect of system/default on related field

everything other stays, conditions are not definitive, like now, do basically whatever you want ;-)

[georgehristov]

I agree with you.
We can add BC support purely in Model::addCondition method.
Question remains about the name. I would say Model::addSystemCondition

[mvorisek]

We can add BC support purely in Model::addCondition method.

this and the sensetence below are two different things, right? :) addCondition should not support this, you have to explictly use the special method

Question remains about the name. I would say Model::addSystemCondition

Something liek that, I tried to stress is MUST ALWAYS BE with = operator and we will support only $field, $value signature

for data/ui, we can dump first which functions/usages were using this

[georgehristov]

Solution with a method in Model does not work as this needs to be triggered on model change (in case of clones or copying of scopes).

I believe the best and cleanest solution is:

• lock negation on scopes once model assigned
• on change of model check if condition is definitive (has '=' scalar value + all parent scopes are AND) and then set field default value and as system field
• remove RootScope class as it will not be needed

[mvorisek]

Model does not work as this needs to be triggered on model change

absolutely not - you use addEqualsToSystemCondition and it sets immediatelly, very predictable and fully controlled

[mvorisek]

I understand the idea of this, for modelling, if and only if a condition is definitive (is always valid), default value can be set, but:

a) PK/ID should never be set (fixed in b24d858)
b) it must never mutate another modelling chain - we probably must support model of model

[mvorisek]

as mentioned in #662 (comment), we need to check if definitive definitely :)

the current design seems to work well, but the problem is when a condition is removed, is this is wanted to be fully supported, we should track the default/system changes from definitive conditions and revert if the conditions are removed. Complicated, but removed conditions should revert the model state completely.

Another issue is removed conditions but model used before for traversal /w materialized conditions. But I belive this is fine, as the traversal does not store the originating model.