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

Fixed issue #1742 where the creation of GPM object tried to connect t… #1746

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

aguvillalba
Copy link
Contributor

…o Remote repositories even if only Local package was needed

Now, it only tries to connect to the outside world when really required and not when just creating a new GPM object.
This could break client classes with the following code:

$gpm = new GPM();
$gpm->grav->getVersion(); //or any other grav method

The above code must be replaced with:

$gpm = new GPM();
$gpm->loadRemoteGrav();
$gpm->grav->getVersion(); //or any other grav method

…nnect to Remote repositories even if only Local package was needed
/**
* Loads Remote Grav package available
*/
public function loadRemoteGrav()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot see this being called from anywhere...?

@aguvillalba
Copy link
Contributor Author

It's used in the admin plugin, in the classes/gpm.php to allow to load the Remote Grav repository.

@mahagr
Copy link
Member

mahagr commented Nov 14, 2017

The issue I'm seeing here is that it is a new function which wasn't there before. So it's not compatible backwards, which needs to be noted.

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to test this. Also it will require the dependency of the admin on this Grav update as without it you will get an error.

These kinds of changes are a bit scary because if you manually update admin before core, the GPM will be broken because the method will not exist, and you won't be able to update core past that point. I would prefer a more backwards compatible approach to prevent this scenario.

@aguvillalba
Copy link
Contributor Author

@rhukster I have already made the modifications and PR in the admin plugin to use this new function, so if the PR in the admin plugin is accepted, it will not cause any issue. As you suggested in the admin plugin, checking if the method exists before using it is a good solution for now.
IMHO, the problem that I see here is that the design of the application is not using real dependency injection (only service locator), so the design is not flexible and it is not straight forward to solve things like this elegantly without breaking things. For instance, the GPM class exposes to the outside the Grav instance as a public property, which breaks the encapsulation.
Anyway, as you suggested in the admin plugin, checking for the method before using it allows backwards compatibility.

@rhukster rhukster merged commit 2cc3415 into getgrav:develop Nov 28, 2017
rhukster added a commit that referenced this pull request Dec 6, 2017
…onnect to Remote repositories even if only Local package was needed (#1746)"

This reverts commit 2cc3415.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants