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

Fix: Added Factories and Inject dependencies for ViewHelper and Module Service #263

Merged
merged 19 commits into from
Dec 11, 2014

Conversation

ins0
Copy link
Contributor

@ins0 ins0 commented Dec 9, 2014

@localheinz
This PR extends your PR #262 and add factories for all necessary viewhelper and the module service

@localheinz
Copy link
Member

@ins0

The build failed because of CS issues.

@localheinz
Copy link
Member

@ins0

Can you also try to avoid merge commits and just rebase on top of master? It's always a good idea to

$ git checkout master
$ git fetch upstream
$ git merge upstream/master
$ git push origin master

first before checking out a new branch.

Thank you!

@ins0
Copy link
Contributor Author

ins0 commented Dec 10, 2014

@localheinz i will keep that in my mind 👍

@ins0
Copy link
Contributor Author

ins0 commented Dec 10, 2014

@localheinz is this merge commit a problem and needs some interaction?

*/
protected $moduleMapper;
public function __construct(\ZfModule\Mapper\Module $moduleMapper, Client $githubClient)
Copy link
Member

Choose a reason for hiding this comment

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

Just as a suggestion - can you import ZfModule\Mapper? Helps avoiding FCQNs!

'zfmodule_service_module' => 'ZfModule\Service\Module',
),
'aliases' => array(
'zfmodule_service_module' => 'ModuleService'
Copy link
Member

Choose a reason for hiding this comment

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

For now I'd avoid using aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realize the service name is only used once. so the alias is useless what is a bp for service names ZfModule\Service\Module or zfmodule_service_module ?

Copy link
Member

Choose a reason for hiding this comment

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

@ins0

It probably depends - if we were using PHP5.5, I'd go for ZfModule\Service\Module::class. Can you leave the service identifier as it was - zfmodule_service_module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@localheinz
Copy link
Member

Thank you, @ins0!

localheinz added a commit that referenced this pull request Dec 11, 2014
Fix: Added Factories and Inject dependencies for ViewHelper and Module Service
@localheinz localheinz merged commit 4d9a6ba into zendframework:master Dec 11, 2014
@ins0 ins0 deleted the fix/factories branch December 11, 2014 20:54
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.

2 participants