-
Notifications
You must be signed in to change notification settings - Fork 14
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
Server select double controller processing for review (don't merge) #35
base: 8.x-1.x
Are you sure you want to change the base?
Server select double controller processing for review (don't merge) #35
Conversation
For now we bail completely if no matching server is found, showing exception message. @todo Decide what else to show - should we display the default server?
@foxtrotcharlie will you be in drupaleurope conference? |
@digitaldonkey Unfortunately not! I'm in South Africa, was in Europe in July for a month, so not feasible to return right now :-( I don't want to bug you with this issue though - I'm sure I'll figure it out, just thought I'd mention it in case the path I'm heading down is not ideal, and you were urgently wanting it :-) Should be able to make some time over the weekend to look at it. I have figured out how to populate the form default, so don't worry about that. |
Previously, when submitting the form, the status method ran again and tested the server as specified in the path that the form was initially built on. This change checks to see if the form was submitted, and if so, we just render the form and don't do any server testing, because that is done once the form redirects, and uses the route parameter to determine which server to test.
I've figured out a way to do this. But I could do with some eyes on it to check that what I've done is OK. Specifically, please check this commit: foxtrotcharlie@a2b228e (look for my comment on line 89 of src/Controller/EthereumController.php If you'd prefer the form to be submitted via AJAX, let me know. |
Added this in branch foxtrotcharlie-add-server-select-to-status-page That was a very fast one. It might be good to do some more cleanup and get it all in one place. |
Sure, happy to get started on that. It doesn't seem like the "ajaxOperation" method should be in the status report controller because it's not part of the report functionality, right? What if I just change the name of that controller from EthereumUIController to EthereumConnectionManagerController? Then keep the status report in it's own controller, perhaps named EthereumServerStatusController? Please let me know if there's anything specific you want me to clean up, and any comments on the above. I see this redirect has an incorrectly named route which I'll rectify:
I believe this should be:
|
I've created the form on the status page, and have added a route parameter to the status controller. The form when submitted adds the correct server id to the status url, but, when submitting, the status method seems to be called twice, and the first time it uses the parameter from the URL that the form was submitted from, and then it uses the parameter from the form redirect parameter.
For example:
This is after submitting the form from the path: admin/reports/ethereum/infura_rinkeby
After submitting the path is /admin/reports/ethereum/localhost_ganache
So on clicking submit the controller method status runs, and the server from the existing path is used, and then after the page refreshes with the new server in the path, the controller status method is called again and uses the server from the new path. I think I'm missing something fundamental here about forms in controllers.
Any advice would be appreciated :-)
The other issue is how to get the form to retain the last submitted server as the default in the select once the form is rebuilt on the status page after submission (it currently just defaults to the default server) . Would I need to use the request object to do that?