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

ContainsMany does not fire ref model hook when set or save #881

Closed
ibelar opened this issue Jun 2, 2021 · 12 comments · Fixed by #1004
Closed

ContainsMany does not fire ref model hook when set or save #881

ibelar opened this issue Jun 2, 2021 · 12 comments · Fixed by #1004

Comments

@ibelar
Copy link
Contributor

ibelar commented Jun 2, 2021

Consider code below:

class Client extends \Atk4\Data\Model
{
    public $table = 'client';
    public $caption = 'Client';

    protected function init(): void
    {
        parent::init();

        $this->addField('name');
        $this->containsMany('Accounts', ['model' => [Account::class]]);
    }
}

class Account extends \Atk4\Data\Model
{
    public $caption = ' ';

    protected function init(): void
    {
        parent::init();

        $this->addField('email', [
            'required' => true,
            'ui' => ['multiline' => [Multiline::INPUT => ['icon' => 'envelope', 'type' => 'email']]],
        ]);
    }
}

When changing Accounts reference value, then the Account model hook should be fire. Actually id does not.

Ex:

$m = new Client($app->db);
// loading client that has 3 related account record.
$clientEntity = $m->load(6);

$ac = $clientEntity->get('Accounts');
array_pop($ac);
$clientEntity->set('Accounts', $ac);
$clientEntity->save();

Then, when setting Account or saving Client, the Account model hook should be fired.

@mvorisek mvorisek added the bug label Jun 2, 2021
@mvorisek mvorisek changed the title containsMany field reference does not fire ref model hook when set or save ContainsMany does not fire ref model hook when set or save Nov 12, 2021
@mvorisek
Copy link
Member

mvorisek commented May 30, 2022

in #999 I have fixed traversing issue with ContainsOne/Many

ContainsOne/Many model (Account is your example) is currently expected to be saved manually which triggers save in the "contained in" model (Client).

In the example, I do not see such manual save called - @ibelar when you reload $clientEntity, can you confirm you really get Accounts /wo the last/popped item?

@ibelar
Copy link
Contributor Author

ibelar commented May 30, 2022

Getting the Accounts data correctly was not the issue here.

What I meant was that when Account model hook are used, like Account::HOOK_BEFORE_SAVE, the hook_before_save is never trigger when setting Account model data using $clientEntity->set('Accounts', $ac) or using $clientEntity->save()

class Account extends \Atk4\Data\Model
{
    public $caption = ' ';

    protected function init(): void
    {
        parent::init();
        // previous code here
      
       $this->onHook(Model::HOOK_BEFORE_SAVE, function ($m) {
             // This is never fire when $clientEntity-set('Account', $ac) is run.
       });
    }
}

It might not have been implement before as well, so I am not sure if this is a bug or more of a feature request.
But definitely a good to have implementation.

I am also not fully aware of the latest change in Data so its perhaps even fixed.

@mvorisek
Copy link
Member

mvorisek commented May 31, 2022

see #1004 test

  • when $ac is saved, a $clientEntity hook is triggered
  • when $ac is deleted, a $clientEntity hook is triggered

$clientEntity->set('Account', $ac) is currently untested not supported - what should it do? is it used anywhere?

however, I still do not undestand this issue ticket completely - what you want to test

$ac->loadAny()->delete();
here?

but there is a delete bug - when root model is deleted, delete hooks for the inner models are not fixed (I will fix this shortly)

@mvorisek
Copy link
Member

mvorisek commented Jun 6, 2022

@ibelar please update the test code in the #1004 PR to reproduce your issue

@ibelar
Copy link
Contributor Author

ibelar commented Jun 6, 2022

Not sure what more you need. This is relatively simple.
When setting Account via parent model set, i.e. $clientEntity->set('Accounts', []) or saving the parent model $clientEntity->save() after Account was set, then hook set within the Account model are not fire.

Like I said before, It might be the intended way that Account hook are not fired. If this is the case then there is no issue.

@mvorisek
Copy link
Member

mvorisek commented Jun 6, 2022

@ibelar In #1004 I have put a test case together and I want you to edit it to replicate the issue you describe. Please fork the PR, edit it and push the failing test case directly to the PR branch or post it here.

Please also see #881 (comment). In atk4/data, you can not use Model::set method to set HasMany/ContainsMany data. What you need is to traverse the relation with ->ref('Accounts') first and then use ->createEntity() + save to add a new record, ->load + save for a record update and ->load + delete for a record delete. I stress this as your example uses $clientEntity->set('Accounts', []) which is not supported.

@ibelar
Copy link
Contributor Author

ibelar commented Jun 6, 2022

From what I know, you can set containsMany model via Model::set() parent model.

Setting Account field the way below work fine:

$m = new Client($app->db);
// loading client that has 3 related account record.
$clientEntity = $m->load(6);

$ac = $clientEntity->get('Accounts');
array_pop($ac);
$clientEntity->set('Accounts', $ac);
$clientEntity->save();

Setting field value within a Form model after submit is also done this way when Form use a Multiline control that is reference to a containsMany field.

@mvorisek
Copy link
Member

mvorisek commented Jun 6, 2022

@ibelar It works as the internal Account data is an array (stored as JSON) and constructed by an Array_ persistence.

Currently, the data are completely unchecked when Client::save is called. Hook on Client::HOOK_BEFORE_SAVE is fired when the JSON data are changed. That is correct. Field content is handled like any other array/JSON field.

Ah, in #881 (comment) you described you want Account::HOOK_BEFORE_SAVE fired. Please confirm this is the issue you contest here and how it should work?

Also, can you post a link (/w line) to a code/Multiline where the Model::set is called on ContainsOne/Many field? I will try to help with the migration to a native Account::save/delete, so all hooks are fired correctly.

@ibelar
Copy link
Contributor Author

ibelar commented Jun 6, 2022

Ah, in #881 (comment) you described you want Account::HOOK_BEFORE_SAVE fired. Please confirm this is the issue you contest here and how it should work?

Yes!

Also, can you post a link (/w line) to a code/Multiline where the Model::set is called on ContainsOne/Many field? I will try to help with the migration to a native Account::save/delete, so all hooks are fired correctly.

Below is a simple setup. Form will add a Multiline control for the 'Account' field. The field get set in Control class (see: https://github.com/atk4/ui/blob/5af98f3ca91046de0318bab8f9435db664e837b3/src/Form/Control.php#L97) and the value is an array return by Multiline.

/*
CREATE TABLE `client_accounts` (
`id` int(11) unsigned NOT NULL AUTO_INCREMENT,
  `name` varchar(255) NOT NULL,
  `accounts` blob,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=utf8;
*/

    class Client extends \Atk4\Data\Model
    {
        public $table = 'client_accounts';
        public $caption = 'Client';

        protected function init(): void
        {
            parent::init();

            $this->addField('name');
            $this->containsMany('Accounts', ['model' => [Account::class]]);

        }
    }

    class Account extends \Atk4\Data\Model
    {
        public $caption = ' ';
        public $table = 'account';

        protected function init(): void
        {
            parent::init();

            $this->addField('email', [
                'required' => true,
                'ui' => ['multiline' => [Multiline::INPUT => ['icon' => 'envelope', 'type' => 'email']]],
            ]);
            $this->addField('password', [
                'required' => true,
                'ui' => ['multiline' => [Multiline::INPUT => ['icon' => 'key', 'type' => 'password']]],
            ]);
            $this->addField('site', ['required' => true]);
            $this->addField('type', [
                'default' => 'user',
                'values' => ['user' => 'Regular User', 'admin' => 'System Admin'],
                'ui' => ['multiline' => [Multiline::TABLE_CELL => ['width' => 'four']]],
            ]);

            $this->onHook(Model::HOOK_BEFORE_SAVE, function ($m) {
                // not fire when parent model set or save...
            });
        }
  }

$m = new Client($app->db);
$entity = $m->createEntity();

$f = Form::addTo($app);
$f->setModel($entity);

$f->onSubmit(function ($f) {
    $f->model->save();
});

@mvorisek
Copy link
Member

mvorisek commented Jun 7, 2022

@ibelar please review #1004

also please see atk4/ui#1797 - I tested MultiLine with demos/form-control/multiline.php demo:

  • there was an issue with Model::tryLoad returning null (and I fixed it, I will also review any tryLoad use in atk4/ui soon although the testing suite passes)
  • the demo demos/form-control/multiline.php seems to not require any additional change - I tested all basic opearations - save, add row + save, delete row + save

so is this issue with hooks really present in Multiline (or elsewhere in atk4/ui)?

also, the MultiLine control does not have any Behat test, would you be so kind and add a Behat test for the basic operations?

@ibelar
Copy link
Contributor Author

ibelar commented Jun 7, 2022

Please note that the issue is not in Multiline.

if this code below is firing hook in Account model then it will be working in Multiline.

$m = new Client($app->db);
// loading client that has 3 related account record.
$clientEntity = $m->load(6);

$ac = $clientEntity->get('Accounts');
array_pop($ac);
$clientEntity->set('Accounts', $ac);
$clientEntity->save();

Beside, Multiline set up in demo is not related to this. Multiline can be used in three different ways:

  • by populating model data directly; (as in Ui demo);
  • by populating hasMany relation of a model;
  • by populating one field of a model, as a containsMany reference; (like the code I provide previously).

@mvorisek
Copy link
Member

mvorisek commented Jun 7, 2022

Any direct data modifications is not wanted, like updating DB with SQL directly instead of via Model with proper normalization incl. hooks and other business logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants