Add weak conditions/save support #1058
Conversation
|
Usecases? What is "weak" and when? Sometimes traversing should be weak, sometimes not, we should probably start with all possible usecases and then find a well understandable solution. |
| * | ||
| * @return $this | ||
| */ | ||
| public function tryReload() |
There was a problem hiding this comment.
What are the usecases? If a record is expected to be possibly deleted, you can always check with tryLoad or handle reload exception.
There was a problem hiding this comment.
The use case is as follows - consider a Crud which displays unread messages with addWeakCondition('unread', true) . You click on edit, and change the unread flag to false. You click save.
The crud then runs a normal ->save function. Since save allows that - in case a field is changed that moves it outside of a weak condition, the save command should only try to reload. If it is successful, the crud will be updated, if it is null, then the entity left the scope, and thus the same animation as in a delete function will be run.
| @@ -1562,7 +1651,7 @@ public function save(array $data = []) | |||
| if ($this->idField && $this->reloadAfterSave) { | |||
| $d = $dirtyRef; | |||
| $dirtyRef = []; | |||
| $this->reload(); | |||
| $this->tryReload(); | |||
There was a problem hiding this comment.
no try - we want strict behaviour as much as possible, not to silently ignore possible issues
There was a problem hiding this comment.
This is not a possible issue but a designed situation - if you do not have any weak condition then we could think about throwing an exception and not null. I think we should not be too dogmatic, especially if we destroy then logics which are present in Ui.
There was a problem hiding this comment.
I do not understand this, please provide a testcase for it.
There was a problem hiding this comment.
I placed the tryReload for the usecase above (the crud remove unread message from scope on save) - so when a model saves something, the associated view gets a feedback if the entity is still there (in scope, so not null) or if it is gone - so save function only tries to reload, and if it is not successful it should return null.
There was a problem hiding this comment.
I understand, but we cannot remove this check because of some need.
I have no solution is my head, maybe we need to add some support to ui for it.
In atk4/data, we have currently BEFORE_SAVE and AFTER_SAVE hooks, maybe we need some concept of try/finally SAVE hook.
In the past we discussed an immutable models. I am still not fully decided if a condition removal should be supported at all. I have no plan to disallow it in the near future, but to support the conditions removal fully, we need to implement #662 first.
If you want to pursue this feature, please start with a real Crud test in atk4/ui. If you need one or two atk4/data classes modified there, you can extend the atk4/data classes in atk4/ui. Please keep in mind, this feature should work also for other ui components like Multiline.
There was a problem hiding this comment.
Why not rund a proper reload if no weak condition exists - only if a weak condition exists, you only try to reload and return null of it left the scope.
There was a problem hiding this comment.
Because we need to be able to rely on reload. If the record became out of scope, it can be some error as well, with tryReload, we can be left with old data.
|
please make the CS/stan CI happy and unit testing to finish at least this is good practice so we can focus on the real issues |
mvorisek
changed the title
|
closing in favor of #1054 |
related initial issue #967