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

Refactor controllers #36

Open
wants to merge 6 commits into
base: foxtrotcharlie-add-server-select-to-status-page
Choose a base branch
from

Conversation

foxtrotcharlie
Copy link
Contributor

@foxtrotcharlie foxtrotcharlie commented Oct 5, 2018

@digitaldonkey I've refactored as suggested here:

Moved status report functionality into a separate controller, called "EthereumStatusPageController". Moved the previous EthereumUIController code into EthereumController - we can use this for miscellaneous ethereum module controller routines.

I renamed the "ajaxOperation" method to "performOperation" - this was done because the method itself actually functions to perform an operation, and the ajax functionality is just an optional part of it, even though we do enforce the ajax in our current implementation. Feels more appropriate to me...

In the new EthereumStatusPageController I just extended the EthereumController class, as I see this was done in, e.g. EthereumSignupController.

If you'd prefer me to extend ControllerBase let me know. I also see that some controllers (e.g. RestUIController) implement ContainerInjectionInterface. I'm relatively new to D8 development so will need to do some reading in this regard to understand the most appropriate implementation.

ControllerBase.php says the following:

Controllers that use this base class have access to a number of utility methods and to the Container, which can greatly reduce boilerplate dependency handling code. However, it also makes the class considerably more difficult to unit test. Therefore this base class should only be used by controller classes that contain only trivial glue code. Controllers that contain sufficiently complex logic that it's worth testing should not use this base class but use ContainerInjectionInterface instead, or even better be refactored to be trivial glue code.
@digitaldonkey or @amateescu - if you guys have any thoughts on the best approach here, feel free to point me in the right direction :-)

Charles Tanton added 6 commits September 17, 2018 14:19
The name of the controller and the method name didn't reflect the functionality of the controller. The Controller performs operations on Ethereum server entities, and while it may use ajax, it doesn't necessarily so the method name 'ajaxOperation' was misleading.
Move status report functionality into a separate controller. Move the previous EthereumUIController code into EthereumController. Rename the "ajaxOperation" method to "performOperation" - this was done because the method itself actually functions to perform an operation, and the ajax functionality is just an optional part of it.
This method has been moved to the EthereumController class.
@foxtrotcharlie
Copy link
Contributor Author

foxtrotcharlie commented Oct 10, 2018

I've added another iteration to the above, in a separate branch: foxtrotcharlie@ecb4a85

Basically, this simplifies the EthereumController, moving the performOperation method to it's own controller. That way the controllers that extend EthereumController don't inherit a method they don't need. I also implemented ContainerInjectionInterface in the new EthereumPerformOperation controller, just to see how it would work. I know it's not really necessary, as extending ControllerBase works fine so let me know if you prefer the ControllerBase implementation instead. I also changed the redirect in performOperation so that it doesn't use the trait that will be deprecated in Drupal 9. It now uses \Drupal\Core\Url instead.

If you are happier with this approach, let me know and I will submit a pull request.

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.

None yet

1 participant