search

atk4 / data

Data Access PHP Framework for SQL & high-latency databases
https://atk4-data.readthedocs.io
MIT License
273 stars 46 forks source link

Adjusted scope validation #1051

Closed mkrecek234 closed 2 years ago

mkrecek234 commented 2 years ago

The introduced scope validation is fine, but too strict:

We have often situations where through a Save command, the entity will leave the scope of the model. This is and should be handled with saveAndUnload. With the latest https://github.com/atk4/data/pull/1044 even $model->saveAndUnload will not permit for example the SoftDelete code example to mark an entity as 'is_deleted' => true. Because it drops out of the model and will result in a record not found.

I generally agree, but only for save, and not for situations where saveAndUnload is used , that means ->reloadAfterSave is false. Otherwise it is a lot of messing around if you let the user change values which make entities leave the model scope.

mkrecek234 commented 2 years ago

@mvorisek The more I think about it - isn't the ->reload call in this line absolutely sufficient for scope check? https://github.com/atk4/data/blob/86ee2097a0ab5f26a4979d0dd3d71f262e346c81/src/Model.php#L1604

Scope should prevail after all hooks, updates, deletes were done, and only if reloadAfterSave is true.

mvorisek commented 2 years ago

@mvorisek The more I think about it - isn't the ->reload call in this line absolutely sufficient for scope check?

https://github.com/atk4/data/blob/86ee2097a0ab5f26a4979d0dd3d71f262e346c81/src/Model.php#L1604

Scope should prevail after all hooks, updates, deletes were done, and only if reloadAfterSave is true.

The https://github.com/atk4/data/pull/1044 took me quite long time to figure out a solution that works with all pre/post update hooks. I belive the current code cannot be simplified or you will break the tests.

Currently, you already broke the tests, especially for insert. ModelNestedSqlTest::* tests are broken too, but these tests may need an adjustment when the persist/hook order is expected to be different.

mvorisek commented 2 years ago

Closing as explained on Discord.