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

"new static" to "Yii::createObject" fix #895

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bscheshirwork
Copy link
Contributor

Result of finder (activeQuery createModels) is correct configure new model

Example usage (without fix not included/trigger on beforeDelete):

    'modules' => [
        'user' => [
            'class' => 'dektrium\user\Module',
            'enableRegistration' => false,
            'adminPermission' => 'administrateUser',
            'modelMap' => [
                'User' => [
                    'class' => 'dektrium\user\models\User',
                    'on ' . ActiveRecord::EVENT_AFTER_INSERT => function ($event) {
                        $id = $event->sender->getId();
                        $roleName = 'user';
                        $auth = Yii::$app->authManager;
                        if (!array_key_exists($roleName, $auth->getRolesByUser($id))) {
                            $userRole = $auth->getRole($roleName);
                            $auth->assign($userRole, $id);
                        }
                    },
                    'on ' . ActiveRecord::EVENT_BEFORE_DELETE => function ($event) {
                        $id = $event->sender->getId();
                        $auth = Yii::$app->authManager;
                        return $auth->revokeAll($id);
                    },
                ],
            ],
        ],
    ],
Q A
Is bugfix? yes

Result of finder (activeQuery)
now included
configured User
@thiagotalma
Copy link
Member

thiagotalma commented Dec 27, 2017

Help is needed to review this.

@SamMousa @philippfrenzel @thyseus @bscheshirwork @tsdogs

@SamMousa
Copy link
Contributor

I disagree with this change. I'd rather not add a dependency on the DI container to AR. The framework doesn't do it either so we'd be deviating from "normal" AR.

Instead I'd recommend you implement a behavior instead.

@bscheshirwork
Copy link
Contributor Author

I will set config of User class into modelMap. I expect "create new object with config". Right?

@SamMousa
Copy link
Contributor

SamMousa commented Jan 9, 2018

@bscheshirwork
Copy link
Contributor Author

@bscheshirwork
Copy link
Contributor Author

... And so many times in framework code using DI in same case. Case "set class throw config"

@SamMousa
Copy link
Contributor

I don't get what you mean

@bscheshirwork
Copy link
Contributor Author

modelMap property is a config to set you own classes

@SamMousa
Copy link
Contributor

I know that; but I don't get the arguments. Anyway go ahead; I don't use the module anymore so if you feel it's a good addition I don't mind it :)

@bscheshirwork
Copy link
Contributor Author

bscheshirwork commented Jan 11, 2018

If we use

new static();

we lose any config settings
and use standard AR class
in data population after find.

I don't use the module anymore

p.s.: @SamMousa
what solution you use?

@SamMousa
Copy link
Contributor

p.s.: @SamMousa
what solution you use?

I use my own solution (meaning it's different for every project)

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

Successfully merging this pull request may close these issues.

3 participants