Improved collector #84

Nyholm · 2016-07-03T17:13:47Z

This PR is work in progress. I just want to show the progress. Please try it out and give me feedback. I believe the collector and extensions work properly. The work that is left is on the design and display of things.

Related to #26

TODO

Only show the first request in the stack by default
Show rest of the stack when you click "Show all"
Hide the body of the request by default and add a "Show body" button.
Add nice design
Maybe put different adapters on different pages. Design it like the translation "Translated"/"Fallback"/"Not translated"

dbu · 2016-07-04T06:38:20Z

great work @Nyholm !

how do you display the information? we will end up with a lot of information if several plugins are active...

Nyholm · 2016-07-04T07:21:32Z

Thank you.

Im not too sure at the moment to be honest... I'll look at Symfony pages and try to do something similar.

dbu · 2016-07-04T07:23:19Z

okay. maybe for each request show the final version that was sent / received by default, and have little tabs to see the other versions between plugins?

Nyholm · 2016-07-13T19:06:07Z

This PR is ready for a review. Im not sure I can make the design look better.

Nyholm · 2016-07-13T19:44:03Z

Here are some screenshots

First view

Show a stack

Show body

Show errors by the error plugin

Show errors from the http client

Nyholm · 2016-07-13T19:56:47Z

Blocked by

Nyholm · 2016-07-13T20:22:48Z

Collector/ClientDataProvider.php

+     *
+     * @return ClientDataProvider
+     */
+    public function setRequests($requests)


Maybe setters should be removed here

...and the getters... They are not used. Only get*Stack is used.

joelwurtz · 2016-07-15T09:17:59Z

Not sure if it's possible to add this here, but i would love to have an option to show request and response content as sometime you want to debug the content of the request you send or the response you receive.

However this can have memory and latency issue for big request / response (i.e. files), so it would be better to add an option in the config (which default to false) for allowing this feature.

WDYT ?

Nyholm · 2016-07-15T09:37:51Z

request and response content? Do you mean the HTTP message body? If so, you can toggle that in the top right corner. Look at image 3. =)

joelwurtz · 2016-07-15T09:40:31Z

Who didn't see that, is this an option for the debugger ? And also does it show also the request body ?

Nyholm · 2016-07-15T09:43:34Z

I use the new FullHttpMessageFormatter to capture the full HTTP message for both requests and responses.

You may configure the FullHttpMessageFormatter to a different limit on the body. Default is 1000 chars. To change that construction parameter is not supported in this PR. I'll leave that for a future update.

joelwurtz · 2016-07-15T10:00:28Z

Just see the formatter and how the request / response is read.

As far as i have see, this formatter mess up with the stream, as __toString is called and the stream is not rewind so the content is retrieve only in the first stream ?

(I may have miss something again ^^)

Nyholm · 2016-07-15T10:04:35Z

I believe you are correct. I created an issue.

So streams should always be rewinded after we read them?

joelwurtz · 2016-07-15T10:22:42Z

So streams should always be rewinded after we read them?

Depends if the stream is rewindable or not, if not we must buffer the stream into memory and replace it in the request and this will have a huge impact on performance IMO

Nyholm · 2016-07-15T10:30:32Z

Who is using streams that you cant rewind (not seekable) in this case?

Guzzle has PumpStream, NoSeekStream, BufferStream and Zend has CallbackStream which never are seekable.

I think we should add allow to opt-out of this if there is a major performance impact.

dbu · 2016-07-15T13:42:06Z

This looks really cool! Some of those screenshots could be used to illustrate the plugin concept in the documentation :-)

regarding streams: i suggest we default to not read anything of the body, but instead have a hint in the debug output that it can be enabled. i would usually expect people to reproduce a call with curl if they want to see the response. (i think we discussed showing a curl command line for a request which would make this trivial - but that is out of scope for this PR)

reading the body means messing with streams, which can pose problems or huge slowdowns if there is larger http bodies involved. if its no bid effort, i am ok to keep the option available, but disabled by default.

sagikazarmark · 2016-07-15T14:07:04Z

i think we discussed showing a curl command line for a request

That would be a cURL formatter in the message package: php-http/message#19

reading the body means messing with streams, which can pose problems or huge slowdowns if there is larger http bodies involved. if its no bid effort, i am ok to keep the option available, but disabled by default.

This is not always possible. In some cases the stream is read-once. We need to provide an alternative stream in that case with the original content.

dbu · 2016-07-15T14:30:40Z

i would prefer to wait with merging this until we have a solution to not break requests with non-rewindable streams.

sagikazarmark · 2016-07-15T14:32:25Z

As @Nyholm said copying is not an option, because the request is immutable. So non-rewindable streams should never be read, otherwise users can optionally turn on this feature.

sagikazarmark · 2016-07-15T15:15:44Z

Collector/RequestStackProvider.php

@@ -68,7 +68,7 @@ public static function createFromCollectedData(array $data)

    /**
     * @param array $messages is an array with keys 'failure', 'request' and 'response' which hold requests for each call to
-     *                        sendRequest and for each depth.
+     *                        sendRequest and for each depth


Not sure if this is a solution. Normally we need that period. See php-http/message#48

sagikazarmark · 2016-07-15T22:11:46Z

@Nyholm it seems symfony changed their CS: symfony/symfony#19198

However, the fixer still seems to be buggy, so I would stick with the full stop everywhere and change it later if necessary. I updated the master branch with the latest styleci config.

Nyholm · 2016-07-15T22:20:58Z

So, No more CS changes in this PR?

Since the merge of php-http/message#47 is there anything left to be done in this PR?

sagikazarmark · 2016-07-15T22:37:22Z

After merging #86 can you please rebase from master and revert 4c33132?

sagikazarmark · 2016-07-15T22:38:09Z

UX: Request 0 -> Request #0

Looks better IMHO, WDYT?

Nyholm · 2016-07-15T22:38:28Z

Sure

sagikazarmark · 2016-07-15T22:52:10Z

Resources/public/script/httplug.js

@@ -0,0 +1,132 @@
+


Why leading newlines?

sagikazarmark · 2016-07-15T22:54:51Z

As always, I am the style guy 😉 Can you please take a look at my comments? I would leave the technical review to @dbu and @joelwurtz as I haven't really followed this conversation.

sagikazarmark · 2016-07-15T23:03:18Z

Can you rebase from master?

Nyholm · 2016-07-15T23:06:54Z

This PR is now rebased and I've addressed all your comments except for the one with the Javascript indentation.

sagikazarmark · 2016-07-15T23:11:56Z

So is this ready for merging then?

Nyholm · 2016-07-15T23:13:50Z

If @joelwurtz and/or @dbu says it is okey, then it's ready to merge IMHO.

sagikazarmark · 2016-07-15T23:14:19Z

Ok

dbu · 2016-07-18T06:35:06Z

ClientFactory/PluginClientFactory.php

@@ -16,11 +16,12 @@
     * @param Plugin[]      $plugins
     * @param ClientFactory $factory
     * @param array         $config
+     * @param array         $pluginClientOptions


can you add a short text that explains the $config and $pluginClientOptions? as its two sets of options, it would help to know what each is to keep them cleanly separated. saying that $pluginClientOptions is forwareded to PluginClient is good enough for me.

dbu · 2016-07-18T06:36:13Z

looks good. mark had some comments about missing @inheritDoc or explanations on the methods, can you adress those as well please? and can you please squash the commits? then looks good to merge to me.

Nyholm · 2016-07-18T06:38:19Z

I believe I already have addressed Marks comments about @inhertdoc. They are just not marked as "comment on an outdated diff"

Nyholm · 2016-07-18T06:53:21Z

Rebased and ready for being merged

dbu · 2016-07-18T07:32:47Z

great job, thanks!

Nyholm · 2016-07-18T07:34:24Z

Thank you all for the reviews and feedback.

sagikazarmark · 2016-07-18T08:32:51Z

Great job as always @Nyholm. Many thanks.

sagikazarmark added the in progress label Jul 3, 2016

Nyholm changed the title ~~[WIP] Improved collector~~ Improved collector Jul 13, 2016

Nyholm reviewed Jul 13, 2016
View reviewed changes

Nyholm mentioned this pull request Jul 15, 2016

Rewind streams in the formatter php-http/message#46

Closed

sagikazarmark reviewed Jul 15, 2016
View reviewed changes

Resources/public/script/httplug.js

@@ -0,0 +1,132 @@

Copy link

Member

sagikazarmark Jul 15, 2016 •

edited

Loading

Choose a reason for hiding this comment

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

Why leading newlines?

Nyholm force-pushed the dev-improved-collector branch from 6cfe83f to 97f7943 Compare July 15, 2016 23:03

sagikazarmark mentioned this pull request Jul 15, 2016

Add a discovery strategy #85

Merged

sagikazarmark mentioned this pull request Jul 15, 2016

detailed reporting in toolbar #26

Closed

Nyholm mentioned this pull request Jul 16, 2016

Discovery bugfixes and more flexible. #87

Merged

sagikazarmark added needs review and removed in progress labels Jul 16, 2016

sagikazarmark modified the milestone: v1.2.0 Jul 16, 2016

Nyholm mentioned this pull request Jul 16, 2016

Update HttplugBundle with new configuration php-http/documentation#114

Closed

dbu reviewed Jul 18, 2016
View reviewed changes

Nyholm force-pushed the dev-improved-collector branch from e7b5002 to 201bcb6 Compare July 18, 2016 06:45

Added improved data collector and web profiler page

3bcdf1d

Nyholm force-pushed the dev-improved-collector branch from 201bcb6 to 3bcdf1d Compare July 18, 2016 06:52

dbu merged commit 68ccd1e into master Jul 18, 2016

dbu deleted the dev-improved-collector branch July 18, 2016 07:32

sagikazarmark removed the needs review label Jul 18, 2016

Improved collector #84

Improved collector #84

Conversation

Nyholm commented Jul 3, 2016 • edited Loading

TODO

dbu commented Jul 4, 2016

Nyholm commented Jul 4, 2016

dbu commented Jul 4, 2016

Nyholm commented Jul 13, 2016

Nyholm commented Jul 13, 2016

First view

Show a stack

Show body

Show errors by the error plugin

Show errors from the http client

Nyholm commented Jul 13, 2016 • edited Loading

Nyholm Jul 13, 2016

Choose a reason for hiding this comment

Nyholm Jul 13, 2016

Choose a reason for hiding this comment

joelwurtz commented Jul 15, 2016

Nyholm commented Jul 15, 2016

joelwurtz commented Jul 15, 2016

Nyholm commented Jul 15, 2016

joelwurtz commented Jul 15, 2016

Nyholm commented Jul 15, 2016

joelwurtz commented Jul 15, 2016

Nyholm commented Jul 15, 2016

dbu commented Jul 15, 2016

sagikazarmark commented Jul 15, 2016

dbu commented Jul 15, 2016 via email

sagikazarmark commented Jul 15, 2016

sagikazarmark Jul 15, 2016

Choose a reason for hiding this comment

sagikazarmark commented Jul 15, 2016

Nyholm commented Jul 15, 2016

sagikazarmark commented Jul 15, 2016

sagikazarmark commented Jul 15, 2016

Nyholm commented Jul 15, 2016

sagikazarmark Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

sagikazarmark commented Jul 15, 2016

sagikazarmark commented Jul 15, 2016

Nyholm commented Jul 15, 2016 • edited Loading

sagikazarmark commented Jul 15, 2016

Nyholm commented Jul 15, 2016

sagikazarmark commented Jul 15, 2016

dbu Jul 18, 2016

Choose a reason for hiding this comment

dbu commented Jul 18, 2016

Nyholm commented Jul 18, 2016

Nyholm commented Jul 18, 2016

dbu commented Jul 18, 2016

Nyholm commented Jul 18, 2016

sagikazarmark commented Jul 18, 2016

Nyholm commented Jul 3, 2016 •

edited

Loading

Nyholm commented Jul 13, 2016 •

edited

Loading

sagikazarmark Jul 15, 2016 •

edited

Loading

Nyholm commented Jul 15, 2016 •

edited

Loading