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

GPM always connects to getgrav server, even when only want to get Local Packages #1742

Closed
aguvillalba opened this issue Nov 8, 2017 · 7 comments

Comments

@aguvillalba
Copy link
Contributor

I have disabled the option enable_auto_updates_check:
enable_auto_updates_check: false

The partial template user/plugins/admin/themes/grav/templates/partials/nav.html.twig tries to get the number of installed plugins:
{{ admin.plugins|length }} (line 61)

This calls to:
Grav\Plugin\Admin\Admin::plugins(true) --->
Grav\Plugin\Admin\Admin::gpm() --->
Grav\Common\GPM::__construct()

And in that construct you have this code:

$this->installed = new Local\Packages();
try {
        $this->repository = new Remote\Packages($refresh, $callback);
        $this->grav = new Remote\GravCore($refresh, $callback);
} catch (\Exception $e) {
}

So, independently of whether I want to the local plugins (Admin::plugins(true)) or not, the constructor of GPM will always try to connect to getgrav.org server to get the plugins repository.
IMHO this does not makes sense, since I want the local plugins, there is no need to connect to the external server.
The connection to external server to get the remote packages should be removed from the constructor and place it in a explicit function that is called whenever is needed to grab the remote repository.

I will try to make a pull request to fix this.

Thank you!

@rhukster
Copy link
Member

rhukster commented Nov 9, 2017

Cool, i'll look forward to your PR.

@mahagr
Copy link
Member

mahagr commented Nov 10, 2017

I would use DI to lazy load remote packages only if they are needed.

@aguvillalba
Copy link
Contributor Author

Me too @mahagr, that's my idea as well. There is not dependency injection manager/framework in this project yet. Changing to DI is not straight forward now since it could break other things where the GPM is used (plugins for instance).
I have already asked in the slack channel about using DI manager/framework (I would not use a service locator), but no answer yet.
In the meantime, I am changing the code to do a "pseudo lazy-loading", so the Remote package is only created when it is actually needed.

@mahagr
Copy link
Member

mahagr commented Nov 10, 2017

Grav uses DI, but inits all the classes early, eg. it doesn't lazy load most things.

@aguvillalba
Copy link
Contributor Author

Well, Pimple is a service locator, not a proper dependency injection manager that's why I said that Grav is not using DI manager, and yes, it does not lazy load dependencies, which is also bad.

aguvillalba added a commit to aguvillalba/grav that referenced this issue Nov 11, 2017
…nnect to Remote repositories even if only Local package was needed
@aguvillalba
Copy link
Contributor Author

PR added to solve this issue

rhukster pushed a commit that referenced this issue Nov 28, 2017
…o Remote repositories even if only Local package was needed (#1746)

All good, thanks!
@rhukster rhukster closed this as completed Dec 6, 2017
rhukster added a commit that referenced this issue Dec 6, 2017
…onnect to Remote repositories even if only Local package was needed (#1746)"

This reverts commit 2cc3415.
@mahagr
Copy link
Member

mahagr commented Dec 11, 2017

@aguvillalba You may want to take a look into the issue as your change was reverted because of it broke plugin/theme installation.

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

No branches or pull requests

3 participants