Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add weak conditions/save support #1058

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 91 additions & 2 deletions src/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,30 @@ public function addCondition($field, $operator = null, $value = null)
return $this;
}

/**
* Same as addCondition but condition will be dropped on save event and not be enforced.
* Therefore, it is valid to save an entity where the saved entity will not comply to the
* weak condition anymore.
*
* Example:
*
* $messsageModel->addWeakCondition('unread', true);
* $messageModel->save(['unread' => false]);
*
* @param mixed $field
* @param mixed $operator
* @param mixed $value
*
* @return $this
*/
public function addWeakCondition($field, $operator = null, $value = null)
{

$this->scope()->addCondition(...func_get_args(), true);

return $this;
}

/**
* Adds WITH/CTE model.
*
Expand Down Expand Up @@ -1340,6 +1364,21 @@ public function reload()
return $this;
}

/**
* Try to reload model by taking its current ID, otherwise return null.
*
* @return $this
*/
public function tryReload()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the usecases? If a record is expected to be possibly deleted, you can always check with tryLoad or handle reload exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is legit usecase 👍

{
$id = $this->getId();
$this->unload();

$res = $this->_load(true, true, $id);

return $this;
}

/**
* Keeps the model data, but wipes out the ID so
* when you save it next time, it ends up as a new
Expand Down Expand Up @@ -1384,6 +1423,36 @@ public function saveAndUnload(array $data = [])
return $this;
}

/**
* Store the data into database, but will never attempt to
* reload the data and permits that entity leaves scope after save.
* Additionally, any data will be unloaded. removeConditionFields will
* eliminate all conditions on those fields, set null to ignore all conditions.
*
* @return $this
*/
public function saveWithoutScope(array $data = [], array $removedConditionFields = null)
{
$scopeElementsOrig = $this->getModel()->scope()->elements;
try {
if ($removedConditionFields) {
foreach ($this->getModel()->scope()->elements as $k => $v) {
if ($v instanceof Model\Scope\Condition && (!$removedConditionFields || in_array($v->key, $removedConditionFields))) {
unset($this->getModel()->scope()->elements[$k]);
}
}
} else { $this->getModel()->scope()->elements = array(); }

$this->saveAndUnload($data);

} finally {
$this->getModel()->scope()->elements = $scopeElementsOrig;
}


return $this;
}

/**
* Create new model from the same base class as $this.
*
Expand Down Expand Up @@ -1465,10 +1534,30 @@ public function tryLoadBy(string $fieldName, $value)
return $this->_loadBy(true, $fieldName, $value);
}

protected function invokeCallbackWithoutWeakConditions(Model $model, \Closure $callback)
{
$scopeElementsOrig = $model->scope()->elements;
try {
foreach ($model->scope()->elements as $k => $v) {
if ($v instanceof Model\Scope\Condition && $v->weak) {
unset($model->scope()->elements[$k]);
}
}

return $callback();
} finally {
$model->scope()->elements = $scopeElementsOrig;
}
}

protected function validateEntityScope(): void
{
if (!$this->getModel()->scope()->isEmpty()) {
$this->getPersistence()->load($this->getModel(), $this->getId());

$this->invokeCallbackWithoutWeakConditions($entity->getModel(), function () use ($entity): void {
$this->getPersistence()->load($this->getModel(), $this->getId());
});

}
}

Expand Down Expand Up @@ -1562,7 +1651,7 @@ public function save(array $data = [])
if ($this->idField && $this->reloadAfterSave) {
$d = $dirtyRef;
$dirtyRef = [];
$this->reload();
$this->tryReload();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no try - we want strict behaviour as much as possible, not to silently ignore possible issues

Copy link
Contributor Author

@mkrecek234 mkrecek234 Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this, please provide a testcase for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$this->dirtyAfterReload = $dirtyRef;
$dirtyRef = $d;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Model/Scope.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function __clone()
*
* @return $this
*/
public function addCondition($field, $operator = null, $value = null)
public function addCondition($field, $operator = null, $value = null, $weak = false)
{
if (func_num_args() === 1 && $field instanceof Scope\AbstractScope) {
$condition = $field;
Expand Down
3 changes: 3 additions & 0 deletions src/Model/Scope/Condition.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ class Condition extends AbstractScope
/** @var mixed */
public $value;

/** @var boolean */
public $weak = false;

public const OPERATOR_EQUALS = '=';
public const OPERATOR_DOESNOT_EQUAL = '!=';
public const OPERATOR_GREATER = '>';
Expand Down