Accessing modules externally without knowing their ID

[ghost]

@vercotux commented on Jul 10, 2017, 4:44 AM UTC:

Consider the following scenario:

1. Take any module with a '_form' view containing an ActiveForm.
2. Render module's '_form' outside of module (eg. in main app site/index view). For example: $this->render('@somevendor/somemodule/frontend/views/post/_form', ['model'=>$model]);

The rendered ActiveForm 'action' URL in our module's '_form' will be incorrect no matter what we do:

• If the form uses a relative route (eg. ['post/create']) then the URL will be pointing to the wrong controller (app's instead of module's).
• If the form uses a fixed absolute route (eg. ['//module/post/create']) then the URL will be pointing to the potentially wrong module ID (it could be called something other than "module").
• If we try to dynamically grab the module's ID (eg. by doing ['//'.Module::getInstance()->id.'/post/create']) we will end up with a "Trying to get property of non-object" exception.

The mechanism to access the module externally without knowing its ID in advance seems to be missing and/or is not documented.

This issue was moved by samdark from yiisoft/yii2#14421.

[ghost]

@samdark commented on Jul 10, 2017, 8:44 AM UTC:

Sorry. Hit the wrong label...

[ghost]

@vercotux commented on Jul 10, 2017, 8:45 AM UTC:

No problem.

To clarify a bit more. There is a fundamental problem with the module architecture.

Look at the ugly workarounds module developers are forced to resort to just to get their code to work:

• https://github.com/yii2mod/yii2-comments/blob/aae211a6e93b8e06ced1218441a6dc845efec464/traits/ModuleTrait.php
• https://github.com/CostaRico/yii2-images/blob/432286dc94f1a71e8ab9548480c148fd145100a6/ModuleTrait.php#L27
• https://github.com/rmrevin/yii2-comments/blob/3ab2ab67925e9e0f2baf50d2f79dd3248f1ad850/Module.php#L61
• https://github.com/fabiomlferreira/yii2-file-manager/blob/9d1de302b529fbfc63419d8be031965285fdcb70/models/Mediafile.php#L331
• https://github.com/amilna/blog/blob/a98d8798d7e604d08802304f9b1fbdf73609d9e9/models/Post.php#L137
• https://github.com/vova07/yii2-users-module/blob/e9ab1803ac6abcd09d4fccba98499e46efd31836/traits/ModuleTrait.php#L25
• https://github.com/kartik-v/yii2-dynagrid/blob/d1a65dcc4eaec89188de28aceeeda71c9e41e1a6/Module.php#L28 [fixed now]

Most of them rely on hardcoding the module ID at some point.

[ghost]

@samdark commented on Jul 10, 2017, 8:46 AM UTC:

I think it would be something like:

$this->controller->module->id . '/controller/action'

[ghost]

@vercotux commented on Jul 10, 2017, 8:46 AM UTC:

Sadly, that only works inside the modules. Modules also come with widgets and other components which may need to be used externally. If we run that code outside the module $this->controller->module will be null.

[ghost]

@samdark commented on Jul 10, 2017, 8:47 AM UTC:

Then these aren't part of modules.

[ghost]

@vercotux commented on Jul 10, 2017, 8:58 AM UTC:

Well they still rely on the modules (eg. they need their controllers like in my original example or they need to read the module's config parameters). But they cannot get to the modules they were shipped along with without knowing in advance what the ID will be, because it is determined by the module user. So unless the module developer has a crystal ball, there's no way to provide the necessary components with that package. Other than hardcoding the ID and then demanding the module user to use that ID, which is just sad.

[ghost]

@samdark commented on Jul 10, 2017, 10:26 AM UTC:

In your original example you're rendering a view by providing a path to it. You're not giving any context to what this view belongs to or anything so you have to supply module's ID explicitly. For example, like this:

$this->render('@somevendor/somemodule/frontend/views/post/_form', [
    'model'=>$model,
    'action' => ['/moduleID/controllerID/actionID'],
]);

[ghost]

@samdark commented on Jul 10, 2017, 10:28 AM UTC:

Not sure how you'd like to make it better...

[ghost]

@rob006 commented on Jul 10, 2017, 10:35 AM UTC:

Render module's '_form' outside of module (eg. in main app site/index view). For example: $this->render('@somevendor/somemodule/frontend/views/post/_form', ['model'=>$model]);

It looks like you're going to create really nasty spaghetti code. You should extract your form to widget and pass routes/module ID as widget configuration.

[ghost]

@vercotux commented on Jul 10, 2017, 10:46 AM UTC:

samdark That would work, it's just inconvenient plus it will only work with modules which are specifically designed to take the 'action' parameter in their views.

EDIT: Another scenario where that would never work: Inter-dependent modules. A module cannot possibly know what another module's ID will be.

I wish there was some sort of a way for modules to "auto-bootstrap" on demand (not on every pageload), whenever one of their dependant components are initialized, then they would always be available via Module::getInstance().

rob006 Absolutely. I was merely using that to illustrate the issue.

[ghost]

@samdark commented on Jul 10, 2017, 10:58 AM UTC:

"auto-bootstrap" on demand isn't possible. Either it's auto or it's on demand. Either you're doing it on each request or triggering it from the code explicitly or implicitly.

[ghost]

@rob006 commented on Jul 10, 2017, 11:09 AM UTC:

samdark I think that main problem is that Module::getInstance() does not support instantiating module which makes this method useless in many cases. You can't safely use MyModule::getInstance() in model, because this model can be used outside of module and then getInstance() will return null, so you can't rely on it.

[ghost]

@samdark commented on Jul 10, 2017, 11:10 AM UTC:

How would you suggest to solve it?

[ghost]

@rob006 commented on Jul 10, 2017, 11:58 AM UTC:

IMO getInstance() - should always return module instance or throw exception if module can not be instantiated. We could traverse modules config tree to find config for given class and/or provide some fallback in configuration, like:

return [
    'modulesMap' => [
        MyModule::class => function () {
            return Yii::$app->getModule('my-module');
        },
    ],
    // ...
];

[ghost]

@samdark commented on Jul 10, 2017, 5:17 PM UTC:

What if the module is used twice w/ different configs/IDs?

[ghost]

@rob006 commented on Jul 10, 2017, 6:03 PM UTC:

If module is instantiated - return instance. If there is a configured callback for module class in modulesMap - return result of callback (this allows to define preferred module). If not - traverse modules config and pick first configured module with specified class. When everything fails - throw exception.

[ghost]

@vercotux commented on Jul 11, 2017, 1:30 AM UTC:

What rob006 is proposing is definitely an improvement. But I agree with samdark. The module instance returned should never be ambiguous. I'm not a big fan of the "pick first" approach. Perhaps instead there could be another way to explicitly specify the "default" module? Adding something like a static $defaultModule property (or even method) containing the default module ID into yii\base\Module comes to mind, although I'm not sure how a static property like that would be configurable other than overriding. Module devs would still need to hardcode an ID, but at least users would have a way to change it, plus most modules could be made to work without bootstraping which is pretty huge.

[ghost]

@rob006 commented on Jul 11, 2017, 7:00 AM UTC:

What is wrong with configuring default module via modulesMap?

[ghost]

@vercotux commented on Jul 11, 2017, 7:04 AM UTC:

Nothing. I was describing the case where something like moduleMap is not configured by the user (instead of doing a fallback to first in config array). It still has to work out-of-the-box so to speak, without requiring a map.

[ghost]

@rob006 commented on Jul 11, 2017, 7:10 AM UTC:

If not user, the who should configure default module ID for app?

[ghost]

@vercotux commented on Jul 11, 2017, 7:31 AM UTC:

It should be the user. But they already do (as the module config key). Requiring the user to set the exact same ID twice in two different places is just wrong (not to mention it would break every existing app configuration that uses modules out there). It should be mandatory no more than 1 time. In my proposal the user has to do it once, and the second time is optional, only if they want to customize it.

[ghost]

@rob006 commented on Jul 11, 2017, 7:37 AM UTC:

In my proposal the user has to do it once, and the second time is optional, only if they want to customize it.

Same here - modulesMap is required only if you use the same module more than once (and you don't want to use first as default) or module is not explicitly configured.

[ghost]

@vercotux commented on Jul 11, 2017, 7:48 AM UTC:

In that case moduleMap itself is still fine by me. But you said "If not - traverse modules config and pick first configured module with specified class" I'm pretty sure Yii devs won't love this part. IMO we need something better instead. What do you think samdark?

[ghost]

@dynasource commented on Jul 11, 2017, 8:36 AM UTC:

there are not many options here. Either you use a mapping solution mentioned by rob006, or you pass around the $module instance. The latter has my preference as mapping creates extra complexity.

[ghost]

@vercotux commented on Jul 11, 2017, 8:52 AM UTC:

Apparently module devs are not very fond of passing around the $module instance, because they all end up hardcoding an ID instead which is the worst solution of all. Something needs to be done about this.

[ghost]

@dynasource commented on Jul 11, 2017, 9:51 AM UTC:

there are >100K Yii users out there. Obviously some of them will choose quick fixes.

Hardcoded module IDs should be prevented in the development of extensions, as developers should be able to customize it. Instead the $module instance should be used as it contains the customized ID. This directly implies that the $module instance should be passed in order to make routing stable.

[ghost]

@vercotux commented on Jul 11, 2017, 10:42 AM UTC:

Well it's evidently quite more than "some". The first 5/6 random modules I found all do this. We need a real solution. Even if it's just providing stricter module guidelines/specifications.

[ghost]

@bizley commented on Jul 11, 2017, 11:15 AM UTC:

I did the same mistake with hardcoded id you are talking about when making my Podium module, it's fixed now and I admit it was error on my part, not Yii's.

Also I don't think Yii should do anything to address the problem of developers keeping hardcoded module's id. All the common solutions of dealing with the module's id are provided. If you are the end user of module you know what is the id you've set. If you are the module's creator you can deal with id from inside the module; if you want to use other module - provide the way to configure and link it to yours.

The only thing I can think of is when there are modules added dynamically and you can not determine the id easily but this is completely different problem.

[ghost]

@vercotux commented on Jul 11, 2017, 11:24 AM UTC:

bizley Your solution I see was to just bootstrap the entire module. I know I'll get flack for this, but I don't think that is a proper solution, although better than a hardcoded ID, it's more of a workaround. It means your module is being bootstrapped on every pageload, even when it is not required. That sucks.

[ghost]

@rob006 commented on Jul 11, 2017, 11:28 AM UTC:

Also I don't think Yii should do anything to address the problem of developers keeping hardcoded module's id. All the common solutions of dealing with the module's id are provided. If you are the end user of module you know what is the id you've set. If you are the module's creator you can deal with id from inside the module.

But as a end user I can not use components from these modules outside of them. For example I can not safely use bizley\podium\models\User model because it blindly assumes that Podium module is already instantiated.

[ghost]

@bizley commented on Jul 11, 2017, 11:32 AM UTC:

vercotux Bootstraping there is not a solution to the "finding id" problem (although I admit it's not optimized). rob006 You are right but it was never ment to be used outside of the module - you guys are looking for a way to use something that was never designed for.

[ghost]

@vercotux commented on Jul 11, 2017, 11:37 AM UTC:

bizley It's a way to avoid needing an ID entirely at the expense of making your users read the documentation every time they want to include your module just to see whether it needs to be bootstrapped. The design is flawed and we are trying to improve it.

[ghost]

@rob006 commented on Jul 11, 2017, 11:42 AM UTC:

You are right but it was never ment to be used outside of the module

Why not? If I would like to use topics and posts from your module, as a base for comments engine in my news module, this issue is the main problem I will have.

[ghost]

@bizley commented on Jul 11, 2017, 11:46 AM UTC:

vercotux You are not reading documentation before using 3rd party packages? Anyway, as I said, bootstrapping there is used for internal logs and routing - I'm using id from inside the module so it's not a problem for me, user can set any id. Remove Podium from bootstrap and it will still work (more or less ;)

rob006 You are right again and now I'm working on separate API for this so you can use separate parts of it (like you want) - but this is the thing: I'm preparing the module to be used like that.

[ghost]

@rob006 commented on Jul 11, 2017, 11:50 AM UTC:

Is there any other problem to solve besides getInstance()?

[samdark]

There are more comments at https://github.com/yiisoft/yii2/issues/14421

[samdark]

Closing since implementation is too different now.