Allow a Run to be imported over the network #230
Conversation
Signed-off-by: Jacob Kiers <kiers@comandi.nl>
src/Xhgui/Controller/Run.php
Outdated
| /** | ||
| * @var \Slim\Slim | ||
| */ | ||
| protected $_app; |
There was a problem hiding this comment.
Property name "$_app" should not be prefixed with an underscore to indicate visibility
There was a problem hiding this comment.
I have used the same style as used elsewhere in the application.
There was a problem hiding this comment.
There is some work to be done in order to get the code up to a PSR2 level.
There was a problem hiding this comment.
Is that required for the passing of this PR? I kept as close to the original coding style as possible. I'm perfectly willing to bring the code that I've touched in line with PSR2.
However, adding "Upgrade the whole project to PSR-2" in the critical path would completely destroy the cost-benefit ratio of pursuing this PR.
There was a problem hiding this comment.
You don't need to upgrade the entire project to PSR-2 but you should make sure your code changes pass the build and follow the chosen coding standards.
There was a problem hiding this comment.
Fair enough. As I said: I'm perfectly willing to fix the code I touched.
Already did so, and also a few small other CS fixes. See the last commit.
src/Xhgui/Controller/Run.php
Outdated
| /** | ||
| * @var Xhgui_Profiles | ||
| */ | ||
| private $_profiles; |
There was a problem hiding this comment.
Property name "$_profiles" should not be prefixed with an underscore to indicate visibility
There was a problem hiding this comment.
I have used the same style as used elsewhere in the application.
src/Xhgui/Controller/Run.php
Outdated
| /** | ||
| * @var Xhgui_WatchFunctions | ||
| */ | ||
| private $_watches; |
There was a problem hiding this comment.
Property name "$_watches" should not be prefixed with an underscore to indicate visibility
There was a problem hiding this comment.
I have used the same style as used elsewhere in the application.
src/Xhgui/Controller/Run.php
Outdated
| { | ||
| $request = $this->_app->request(); | ||
|
|
||
| $data = json_decode($request->getBody(), true); |
There was a problem hiding this comment.
There is no authentication, authorization or checksums on this data right now. Should there be?
There was a problem hiding this comment.
Yes, I realize that. It was a conscious choice, since there were none of these in the rest of the project as well.
Furthermore, I assumed that XHGui, being a tool that is primarily used during development (imho), would be deployed in a somewhat trusted environment, like a staging server, or a developers' workstation.
|
I'm willing to add a test. What exactly would you like to see exactly in the test? As said, I've essentially duplicated the code from |
Signed-off-by: Jacob Kiers <kiers@comandi.nl>
jacobkiers
force-pushed
the
add-profile-import-over-network
branch
from
bfaae2f to
9f99698
Compare
That a JSON body can go in and be read out of the database. Having some tests around the API contract of the import endpoint would be nice.
This new endpoint gives us the opportunity to remove that file and have a better tested, more maintainable solution going forwards. |
|
Hi, just quick note to assure you that I haven't forgotten about this. I'm currently unable to work on it, but I expect to be able to address the comments within a month. |
|
This deserves an example of usage, i.e how to post to `/run/import' endpoint. |
|
Example saver can be found from: |
In my use case, I cannot run a MongoDB installation close to my application, and saving and uploading files is hard.
Therefore, I have added the ability to have a profile uploaded. It uses the same process as the existing file importer in
external/import.phpfile.The corresponding PR for the collector is perftools/xhgui-collector#5.