Skip to content
This repository has been archived by the owner on May 1, 2019. It is now read-only.

Fix: Move plugin functionality into appropriate service #365

Merged
merged 12 commits into from
Feb 8, 2015

Conversation

localheinz
Copy link
Member

This PR

  • moves the functionality of the ZfModule\Mvc\Controller\Plugin\ListModule controller plugin into ZfModule\Service\Module as this seems to be the appropriate service (same dependencies as the plugin)
  • creates a factory for User\Controller\UserController and injects the the aforementioned service
  • creates an alias for the zfcuser controller so we can actually use a factory
  • adds a bunch of tests for Service\Module::listModule() so we can move on and refactor
  • extracts Service\Module::listAllModules() and Service\Module::listUserModules() from Service\Module::listModule(), removes the latter
  • renames methods

Follows #362, especially #362 (comment).

💡 Not sure about renaming methods, but to be honest, I have recently gotten tired of all of this getFoo() and setBar() stuff, does it make sense?

@@ -5,9 +5,12 @@

return [
'controllers' => [
'invokables' => [
'aliases' => [
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to alias, otherwise the configuration of https://github.com/ZF-Commons/ZfcUser/blob/1.0.0/config/module.config.php#L8-L12 kicks in.

$user = isset($options['user']) ? $options['user'] : false;

//limit modules to only user modules
if ($user) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius

While I am at it, I would like to remove the options, since there's only one call to this method (YAGNI), but maybe you prefer a separate PR? Still in the mood for cleaning things before moving on to actually implementing new features.

Copy link
Member

Choose a reason for hiding this comment

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

@localheinz why not just providing a custom method? Cleaner and more expressive, IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius

Wait till I finish my taxes, will follow up with a separate PR!

@Ocramius
Copy link
Member

Ocramius commented Feb 5, 2015

@localheinz much better :-)

@localheinz
Copy link
Member Author

@Ocramius

Glad you like it!

@localheinz
Copy link
Member Author

@Ocramius

Added tests for Service\Module::listModule() and refactored the code a bit, please have a look!

*/
public function currentUserModules()
{
$repositories = $this->githubClient->api('current_user')->repos([
Copy link
Member

Choose a reason for hiding this comment

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

Isn't current_user hitting the modules.zendframework.com user token's user?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius

To be honest, I have no idea, haven't had much of a look at the API yet. But I'm sure that @ins0 has. Nonetheless, this should be a separate issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius

I'd assume, though, that this refers to the currently authenticated user, no?

Edit

See this:

/**
* Return all Repositories from current authenticated GitHub User
*
* @param array $params
*
* @return RepositoryCollection
*/
public function getAuthenticatedUserRepositories($params = [])
{
return $this->githubClient->api('current_user')->repos($params);
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and that is very weird: the service keeps a reference to the user in the auth service? (you see where I'm going with this...)

Copy link
Contributor

Choose a reason for hiding this comment

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

jep current_user will request the user repositorys from the current linked github account of a user

Copy link
Member

Choose a reason for hiding this comment

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

$this->someService->getUserRepos(); would still need the $user ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ocramius
one more try 😃

$userRepoInDb = $this->someService->getUserRepos($this->identity());
foreach($userRepoInDb as $repository){
        $repoData = $this->repositoryRetreiver->getRepositoryMetaData($identity->getUsername(), $repository->getName());
       // do stuff check for etc?
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's the stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for your explanation @Ocramius

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius @ins0

How about this PR, then, are you fine with it?

@ins0
Copy link
Contributor

ins0 commented Feb 7, 2015

👍 looks good!

Ocramius added a commit that referenced this pull request Feb 8, 2015
Fix: Move plugin functionality into appropriate service
@Ocramius Ocramius merged commit aa444d5 into zendframework:master Feb 8, 2015
@Ocramius
Copy link
Member

Ocramius commented Feb 8, 2015

@localheinz merged, thanks!

@localheinz
Copy link
Member Author

Thank you for reviewing, @Ocramius and @ins0!

@localheinz localheinz deleted the fix/plugin-vs-service branch February 8, 2015 09:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants