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

Pass di when register module autoloaders #156

Closed
Yahasana opened this issue Sep 7, 2018 · 7 comments
Closed

Pass di when register module autoloaders #156

Yahasana opened this issue Sep 7, 2018 · 7 comments
Labels

Comments

@Yahasana
Copy link
Contributor

Yahasana commented Sep 7, 2018

it'll be very convenient to access DI instance when ModuleInterface::registerAutoloaders() is changed to ModuleInterface::registerAutoloaders(variable $di) .

    public function registerAutoloaders()
    {
        (new Loader())
            ->addNamespace(__NAMESPACE__ . '\Controllers', __DIR__ . '/controllers/')
            ->register();
    }

VS

    // better performance
    public function registerAutoloaders(Di $di)
    {
        $di->loader->addNamespace(__NAMESPACE__ . '\Controllers', __DIR__ . '/controllers/');
    }
@Yahasana
Copy link
Contributor Author

Yahasana commented Sep 7, 2018

an amazing feature that allow user to set a higher loading order for the current module when prepend the loading namespaces: $di->loader->addNamespace(__NAMESPACE__ . '\Controllers', __DIR__ . '/controllers/', true);

@mruz mruz added the feature label Sep 7, 2018
@mruz
Copy link
Member

mruz commented Sep 7, 2018

It's easy to implement, how about $di = null by default, to keep backward compatibility?
https://github.com/ice/framework/blob/dev/ice/dispatcher.zep#L189
https://github.com/ice/framework/blob/dev/ice/mvc/moduleinterface.zep#L18

@Yahasana
Copy link
Contributor Author

Yahasana commented Sep 7, 2018

@mruz it's fine!

@Yahasana
Copy link
Contributor Author

Yahasana commented Sep 8, 2018

After a sound thought, pass the loader loading the current module to registerAutoloaders($loader) to allow developer control the module loading process.

if people want to access the $di instance, it can be done in registerServices(Di $di)

BTW, add Loader $loader = null to ModuleInterface::registerAutoloaders(Loader $loader = null) doesn't keep backward compatibility. So, ModuleInterface::registerAutoloaders(Loader $loader) is ok

@mruz mruz closed this as completed in 3a4de1c Sep 8, 2018
mruz added a commit that referenced this issue Sep 8, 2018
Modules, pass di to autoloaders fix #156
@Yahasana
Copy link
Contributor Author

Yahasana commented Sep 8, 2018

@mruz please check my last comment. the follow code will not work as expect, as the loader of the module is not the same as the one in $di->loader. it safe to pass $loader but not this->di.

    // better performance
    public function registerAutoloaders(Di $di)
    {
        $di->loader->addNamespace(__NAMESPACE__ . '\Controllers', __DIR__ . '/controllers/');
    }

@mruz
Copy link
Member

mruz commented Sep 8, 2018

I didn't see your comment.

@mruz mruz reopened this Sep 8, 2018
@mruz
Copy link
Member

mruz commented Sep 8, 2018

let loader = this->di->get("loader")

Will get the loader from te di or create a new if not exist, so maybe that way.
Then ModuleInterface::registerAutoloaders(Loader $loader = null), you won't need to update your app if you have just registerAutoloaders() in every module.

@mruz mruz closed this as completed in 8dbabf9 Sep 8, 2018
mruz added a commit that referenced this issue Sep 8, 2018
Modules, use loader from the di fix #156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants