Fix system condition handling #692

georgehristov · 2020-08-08T08:08:18Z

~~fixes #662~~

mvorisek

This PR introduce so much additional bad design. Better to start from scratch and address #662 in Model with specific method only.

This reverts commit e9e6dfb.

This reverts commit 030b5e0.

mvorisek · 2020-08-08T08:40:07Z

src/Model/Scope/Condition.php

    protected function onChangeModel(): void
    {
        if ($model = $this->getModel()) {
            // 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 === self::OPERATOR_EQUALS && !is_object($this->value) && !is_array($this->value)) {
+            if ($this->system && $this->setsDefiniteValue()) {


this whole code must be moved into Model under specific method

if this is currently used in refs etc., we must investigate, probably we even want to call this special method when traversing (thru refs)

Model condition in ATK is integral part of the model as the data can be scoped down using Model::addCondition or Model::scope()->addCondition. So addCondition is the special method we use.
This present flag routine is necessary as it makes sure:

when Model/Scope are cloned the same defaults are set for the field. So whenever you add new record this value (if definite) is applied as default

when scope is created or cloned and added to another scope it makes sure the flag is set only when we have a definite value

It is not about reference records only. Consider a model People with field Gender. You can define a sub-scope model classMen when adding condition gender = 'male' in init method. So if you create new record in Men the Gender field will be set automatically to 'male' and having it as system field you cannot set it to anything else, which is correct.

The other not-defining way to apply condition is on the query directly after we merge my refactor on queries:
$people->toQuery('select')->where('gender', 'male');
or when I introduce the Model\QueryTrait:
$people->where('gender', 'male');

This does not change the scope on $people. It just returns an iterator for all men.

If you find it more descriptive I can change the setSystem to setSystemFieldIfDefinite and we merge this fix

I know this. I will summarize shortly:

addCondition should never change Field property (thus under no condition set system flag without the user requests it)

I proposed special explicit method in Model thus - and you confirm it with the post above - it is about scoping Model, thus always topmost condition & and junction

this will work will cloning nicely too

I am explaining above that addCondition is this explicit method you are requesting.
It implicates that the model is being changed (not for querying only) as you addCondition to the model

The 'not explicit' method is Model::where

Look at it differently - I use STANDARD addCondition for ex. for event, let say "party".

If want to restrict user to only one event - "party" - then I add condition "= 'party'".

With your proposal, the event field will behave absolutely differently vs. if I set "in('party')". That is unacceptable.

So maybe we mixed 2 things together:
a) there was some default/system behaviour when some definitive condition was set which was broken as we allow deeper nesting of not only and conditions now - this PR may solve this, even I think not optimally, as Condition can be made later made not always true
b) implicit change of Field with side effect to other code - described above, we MUST prevent this

So in this regard I would suggest to approach the other way:

if definite value set on condition - set value as field default

if no definite value set - validate new entries with model scope. So once you set event in ('party', 'gala') then no other values will be accepted for the event field and validation exception will be thrown.
I have the validation routine for Scope but I removed it for the basic functionality.

We also need opinion and input on this from @romaninsh and @DarkSide666

* if definite value set on condition - set value as field default

this may be leaking (like if we compare some secret) and not sure if you meant system as well

I propose to NOT set any Field param based on condition.

I think, that system&default is needed for refs or in definitive model scoping in general, but this should be fully intended by the user and the current approach is for = only (as default can have only one value), thus in in your later example can not be handled or even = when defined with expr.

* if no definite value set - validate new entries with model scope. So once you set `event in ('party', 'gala')` then no other values will be accepted for the event field and validation exception will be thrown. I have the validation routine for Scope but I removed it for the basic functionality.

If you do not have a full SQL parser like DSQL (which parses fully, but it quite limited on the other side), then you can not have a validation routine.

To move this thread forward I propose to implement:

Do not mutate Field::default/system property in scope condition #662 (comment) ie. explicit Model method that calls standard addCondition() + update Field (default/system, like now)
or

Fix system condition handling #692 (comment) ie. only flag for querying, not sure, if enought, there will be break at least in ui

so I stay very very firm to implement is using special method in Model - which I belive is clean, quite BC (100% in refs) and later, we might introduce extended concept - but probably never redefine Field from Condition/Scope

georgehristov · 2020-08-08T10:42:55Z

Specific method in Model is not a good idea.

huge BC break
does not solve issue with cloning Model (and scope)

Feel free to make you own PR. I will not change this one as it fixes the issue quite nicely.

mvorisek · 2020-08-08T10:45:09Z

huge BC break

wanted, in refs, we clone before and we can fix easily

does not solve issue with cloning Model (and scope)

please describe

as it fixes the issue quite nicely.

the issue is that we should never set system/default by default if addCondition is added by an user! and I think this PR does NOT solve it

georgehristov · 2020-08-08T10:49:54Z

I will not support having a special method in Model for it.
It should go along with Model::addCondition and Model::scope()->addCondition

The system indicator should be kept if scope cloned and used for another Model. But removed a when scope cloned and added to another scope

mvorisek · 2020-08-08T11:03:57Z

please describe

does not solve issue with cloning Model (and scope)

I am not strictly againts a flag in a Condition, but only like allowSystemIfAlwaysTrue and default to false - in refs, we can then set to true and there will be no implicit/unwanted behaviour

georgehristov · 2020-08-08T11:22:30Z

Will try to assign this already when references are being set and remove it completely from Condition

georgehristov · 2020-08-08T11:32:03Z

It is not about references but liting the scope.
So what is bothering you - the method name?

setSystem -> allowSystemIfAlwaysTrue (setSystemFieldIfDefinite)

mvorisek

design does NOT address important requirements

see #692 (comment) and issue desc

mvorisek · 2022-07-30T10:28:31Z

Unrequested mutation of a model is forbidden by design. Definitive fields should be resolved at runtime - they must not be editable (Field::isEditable()) and the default field value must be implied dynamically as well.

mvorisek · 2023-09-01T09:41:48Z

fixes #662

this PR does not fix #662 issue, this PR adds a concept of system scope which might be wanted, but the requirements are mainly to be able to loosen/remove "system" scope for soft-delete purposes and possibly cleanup "non-system" scope added during traversing

as not addressed by this PR, no tests, no feedback, closing in favor of #1054

georgehristov added 2 commits August 8, 2020 09:51

[update] introduce and use Scope::create

030b5e0

[update] introduce Condition::isTraversing

e9e6dfb

georgehristov requested review from mvorisek and DarkSide666 August 8, 2020 08:09

georgehristov added 2 commits August 8, 2020 10:21

[fix] introduce AbstractScope::setSystem

0cc4be9

[fix] use correct operator

a332d86

georgehristov force-pushed the fix/introduce-system-condition branch from 1b9f276 to a332d86 Compare August 8, 2020 08:22

mvorisek requested changes Aug 8, 2020

View reviewed changes

georgehristov and others added 2 commits August 8, 2020 10:36

Revert "[update] introduce Condition::isTraversing"

7db95b1

This reverts commit e9e6dfb.

Revert "[update] introduce and use Scope::create"

d12f1cc

This reverts commit 030b5e0.

mvorisek reviewed Aug 8, 2020

View reviewed changes

georgehristov marked this pull request as draft August 8, 2020 11:22

georgehristov marked this pull request as ready for review August 8, 2020 12:07

mvorisek requested changes Aug 10, 2020

View reviewed changes

mvorisek added the waiting for author label Sep 18, 2020

mvorisek added the bug label Mar 3, 2021

mvorisek marked this pull request as draft August 31, 2022 20:32

Merge branch 'develop' into fix/introduce-system-condition

cbdb1c9

mvorisek removed their request for review September 1, 2023 09:10

fix merge

50c8d75

mvorisek force-pushed the fix/introduce-system-condition branch from daa576a to 50c8d75 Compare September 1, 2023 09:32

mvorisek mentioned this pull request May 24, 2023

Add native soft-delete support #1054

Draft

mvorisek closed this Sep 1, 2023

mvorisek deleted the fix/introduce-system-condition branch September 1, 2023 09:41

mvorisek added enhancement and removed bug waiting for author labels Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix system condition handling #692

Fix system condition handling #692

georgehristov commented Aug 8, 2020 •

edited by mvorisek

Loading

mvorisek left a comment

mvorisek Aug 8, 2020

georgehristov Aug 10, 2020 •

edited

Loading

mvorisek Aug 10, 2020

georgehristov Aug 10, 2020

mvorisek Aug 10, 2020

georgehristov Aug 10, 2020 •

edited

Loading

mvorisek Aug 10, 2020 •

edited

Loading

georgehristov commented Aug 8, 2020

mvorisek commented Aug 8, 2020 •

edited

Loading

georgehristov commented Aug 8, 2020

mvorisek commented Aug 8, 2020

georgehristov commented Aug 8, 2020

georgehristov commented Aug 8, 2020 •

edited

Loading

mvorisek left a comment

mvorisek commented Jul 30, 2022

mvorisek commented Sep 1, 2023

Fix system condition handling #692

Fix system condition handling #692

Conversation

georgehristov commented Aug 8, 2020 • edited by mvorisek Loading

mvorisek left a comment

Choose a reason for hiding this comment

mvorisek Aug 8, 2020

Choose a reason for hiding this comment

georgehristov Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

mvorisek Aug 10, 2020

Choose a reason for hiding this comment

georgehristov Aug 10, 2020

Choose a reason for hiding this comment

mvorisek Aug 10, 2020

Choose a reason for hiding this comment

georgehristov Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

mvorisek Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

georgehristov commented Aug 8, 2020

mvorisek commented Aug 8, 2020 • edited Loading

georgehristov commented Aug 8, 2020

mvorisek commented Aug 8, 2020

georgehristov commented Aug 8, 2020

georgehristov commented Aug 8, 2020 • edited Loading

mvorisek left a comment

Choose a reason for hiding this comment

mvorisek commented Jul 30, 2022

mvorisek commented Sep 1, 2023

georgehristov commented Aug 8, 2020 •

edited by mvorisek

Loading

georgehristov Aug 10, 2020 •

edited

Loading

georgehristov Aug 10, 2020 •

edited

Loading

mvorisek Aug 10, 2020 •

edited

Loading

mvorisek commented Aug 8, 2020 •

edited

Loading

georgehristov commented Aug 8, 2020 •

edited

Loading