-
Notifications
You must be signed in to change notification settings - Fork 159
Fix: View helper should actually be a controller plugin #362
Fix: View helper should actually be a controller plugin #362
Conversation
bc5a0ba
to
af3da63
Compare
@localheinz needs a rebase |
af3da63
to
83a3ba6
Compare
83a3ba6
to
b8410f0
Compare
Rebased, @Ocramius! Thank you for the review. |
@@ -23,7 +23,6 @@ | |||
<div class="tab-content"> | |||
<div class="tab-pane active" id="modules"> | |||
<?php | |||
$modules = $this->listModule(['user' => true]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually agree with this. Why was it moved to a controller plugin? What's the rationale behind it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see from what the view helper does:
- it accesses the database layer
- it makes calls to an external API
- it returns an array of data
As far as my experience goes, these are all things I would never expect from a view helper.
Instead, I would see the responsibility of a view helper in accepting some data and preparing it for presentation in the view, for example, by applying some formatting or filling the data into a view template.
Please note that, as far as I can see, I wouldn't even create a controller plugin just yet, as it's only ever used once. Instead, I'd probably leave it to a service.
This was just the smallest refactoring possible, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are all things I would never expect from a view helper
I disagree with this, but I also see why you'd think that. As long as these are all read operations, IMO it's fine to have them in view helpers. Yes, they sould not cause changes/events to be triggered.
Instead, I'd probably leave it to a service
Possibly a good solution, yes.
*/ | ||
class UserController extends ZfcUserController | ||
{ | ||
public function indexAction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller has no associated tests
Once the missing tests are introduced (integration with |
1dab7d0
to
3744c8a
Compare
Added a test for the case when the authentication service has no identity. Tried to add a test for the success case and have almost been mocking myself to death and/or trying to detach listeners, still getting failing tests because of that. |
553153d
to
076ac84
Compare
15ab3eb
to
557e7da
Compare
) | ||
; | ||
|
||
$listModule = $this->getMockBuilder(ListModule::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mocking $listModule
as function () {...}
is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if it makes your life easier: otherwise, current approach is fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mocking
$listModule
asfunction () {...}
is enough
Doesn't work, as PluginManager
is validating and asking for an implementation of Zend\Mvc\Controller\Plugin\PluginInterface
. However, I mocked out all of the view helpers giving me a headache, tests should pass now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grrr @ PluginManager
.
Yeah, mocking controllers is a mess
84a9f53
to
099812e
Compare
099812e
to
c81f8f8
Compare
…-helper Fix: View helper should actually be a controller plugin
@localheinz let it be merged! |
😊 |
This PR
ZfcUser\Controller\UserController
in order to set view variables in the view model returned from theindexAction()