Added web profiler and toolbar #19

Nyholm · 2016-01-08T08:30:21Z

As mentioned in #3.

This PR implements a web debug toolbar. It uses the HistoryPlugin.

dbu · 2016-01-08T09:03:00Z

Collector/MessageJournal.php

+    /**
+     * {@inheritdoc}
+     */
+    public function collect(Request $request, Response $response, \Exception $exception = null)


is this intentionally empty? if so, please add a comment why it is there and empty.

Okey, thank you.

A upcoming PR maybe want to log which Symfony Request that made which Httplug Request. But it should be empty for now.

dbu · 2016-01-08T09:08:52Z

very cool! i am impressed how simple this looks!

Nyholm · 2016-01-08T09:14:41Z

very cool! i am impressed how simple this looks!

Thank you. So am I. I barley wrote any code just some clue to make Httplug\* classes talk with Symfony.

dbu · 2016-01-08T10:13:05Z

README.md

@@ -51,14 +51,16 @@ For information how to write applications with the services provided by this bun
 | httplug.stream_factory | Service* that provides the `Http\Message\StreamFactory`
 | httplug.client.[name] | This is your Httpclient that you have configured. With the configuration below the name would be `acme_client`.
 | httplug.client | This is the first client configured or a client named `default`.
-| httplug.plugin.content_length <br> httplug.plugin.decoder<br> httplug.plugin.error<br> httplug.plugin.logger<br> httplug.plugin.redirect<br> httplug.plugin.retry | These are build in plugins that lives in the `php-http/plugins` package. These servcies are not public and may only be used when configure HttpClients or services. 
+| httplug.plugin.content_length <br> httplug.plugin.decoder<br> httplug.plugin.error<br> httplug.plugin.history<br> httplug.plugin.logger<br> httplug.plugin.redirect<br> httplug.plugin.retry | These are built in plugins that lives in the `php-http/plugins` package. These servcies are not public and may only be used when configure HttpClients or services.<br><br>*The History plugin is only available when the kernel runs on debug mode and `httplug.toolbar: true`. It gets automatically injected to all clients.* 


s/that lives/that live/

dbu · 2016-01-08T15:10:08Z

DependencyInjection/HttplugExtension.php

@@ -27,6 +27,13 @@ public function load(array $configs, ContainerBuilder $container)
        $loader->load('services.xml');
        $loader->load('plugins.xml');
        $loader->load('discovery.xml');
+        if ($config['toolbar']['enabled'] && $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug')) {


is this state of the art to detect whether the debug toolbar is active? what if somebody has it active in prod env or deactivated in dev env? i guess in the latter case, we just waste some cpu cycles in dev mode which is no big deal. the other might be a problem. or is debug toolbar hard coupled to kernel.debug?

You are correct. I should redo this.
What I really want to know is if the service profiler exits and if its configuration for toolbar is true.

I'll investigate some more.

maybe look at what other bundles that plug into the toolbar do

or is debug toolbar hard coupled to kernel.debug?

No it is not. To get the toolbar you need framework.profiler enabled and of course the WebProfilerBundle. There is nothing that ties it to kernel.debug. So this is technically wrong, but it works for most cases. Others are doing the same.

https://github.com/egeloen/IvoryHttpAdapterBundle/blob/master/DependencyInjection/IvoryHttpAdapterExtension.php#L62

I could use a compiler pass and check for the web_profiler.debug_toolbar.mode parameter if it is enabled or disabled. But I think that will complicate how I inject the HistoryPlugin because that would also be done in the compiler pass.

Question: Are we happy with postponing support for the use case where you want the debug Httplug request via the toolbar with kernel.debug: false?

maybe look at what other bundles that plug into the toolbar do

I've looked at a few. It is common that you inject a logger or a collector to a decorating class. That injection is done in a compiler pass. Some bundles (like GeocoderBundle do always inject the logger, no matter debug or environment.

It's always a good idea to inject logger. Fingers Crossed handler can
decide then when and what to log.

Tobias Nyholm [email protected] ezt írta (2016. január 8., péntek):

In DependencyInjection/HttplugExtension.php
#19 (comment):

@@ -27,6 +27,13 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('services.xml');
$loader->load('plugins.xml');
$loader->load('discovery.xml');

if ($config['toolbar']['enabled'] && $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug')) {

maybe look at what other bundles that plug into the toolbar do

I've looked at a few. It is common that you inject a logger or a collector
to a decorating class. That injection is done in a compiler pass. Some
bundles (like GeocoderBundle
https://github.com/geocoder-php/BazingaGeocoderBundle do always inject
the logger, no matter debug or environment.

—
Reply to this email directly or view it on GitHub
https://github.com/php-http/HttplugBundle/pull/19/files#r49206816.

It's always a good idea to inject logger. Fingers Crossed handler candecide then when and what to log.

Yes, I meant more of a DataCollector to collect data for the profiler. That is injected all the time

dbu · 2016-01-09T09:24:17Z

DependencyInjection/Configuration.php

@@ -34,8 +34,7 @@ public function getConfigTreeBuilder()
                    return !empty($v['classes']['client'])
                        || !empty($v['classes']['message_factory'])
                        || !empty($v['classes']['uri_factory'])
-                        || !empty($v['classes']['stream_factory'])
-                    ;
+                        || !empty($v['classes']['stream_factory']);


is this styleci? my idea was that adding another check only does a diff on one line, like trailing comma in arrays.

Yeah, this is styleci

sagikazarmark · 2016-01-09T12:33:10Z

README.md

 If you need a more custom setup, define the services in your application configuration and specify your service in the `main_alias` section. For example, to add authentication headers, you could define a service that decorates the service `httplug.client.default` with a plugin that injects the authentication headers into the request and configure `httplug.main_alias.client` to the name of your service.

 ```yaml
 httplug:
-  clients:
+  clients: 


Is this a trailing space?

Nyholm · 2016-01-09T14:28:08Z

Thank you for your feedback. I've updated the PR.

sagikazarmark · 2016-01-10T17:58:57Z

Sorry, CS fixes probably introduced a few conflicts.

Nyholm · 2016-01-10T20:27:59Z

No worries, I've updated the PR.

sagikazarmark · 2016-01-10T20:33:51Z

Cool. How far is it? I am eager to try it in a project. 😄

Nyholm · 2016-01-10T20:46:22Z

I'm done with the implementation, I've received a lot feedback and made some improvements. I have addressed all the issues that were brought up. Now Im waiting for @dbu to give this PR the final thumbs up.

sagikazarmark · 2016-01-10T20:56:53Z

Cool.

Added web profiler and toolbar

dbu · 2016-01-10T21:54:12Z

thanks a lot for this one, good job!

sagikazarmark · 2016-01-10T21:57:10Z

👍

Nyholm · 2016-01-10T22:01:56Z

Thank you for merging. I'm also very exited to try this in a project =)

dbu · 2016-01-11T09:21:04Z

DependencyInjection/HttplugExtension.php

@@ -54,6 +66,10 @@ protected function configureClients(ContainerBuilder $container, array $config)
                $first = $name;
            }

+            if (isset($config['_inject_collector_plugin'])) {
+                array_unshift($arguments['plugins'], 'httplug.collector.history_plugin');


actually, adds the plugin first. does that mean we log before anything else? that will mean that if we have the redirect plugin active, we will not see that a redirect happened. then again, if redirect was activated, we might not care about it.
but it also means we will not log the request exactly as it was sent, with base url or credentials or whatever was added by the plugins. do we maybe need a flag to let the user decide whether to add to the beginning or to the end?

If you have the CachePlugin and put the HistoryPlugin last you will not see anything... But that might be okey, since no request was actually made.

true. hm. it would be cool to see whether requests where served from cache or the actual backend. we could try to have two history plugins and see whether a request goes to the client or not, and also see whether there is a redirect to serve one answer.

created #26 for this idea. i guess what we have now is ok, but the other thing sure would be cool.

Nyholm force-pushed the toolbar branch 2 times, most recently from c678539 to 2b2ad74 Compare January 8, 2016 08:39

dbu reviewed Jan 8, 2016
View reviewed changes

Nyholm force-pushed the toolbar branch from 838a775 to 151a727 Compare January 8, 2016 15:09

dbu reviewed Jan 8, 2016
View reviewed changes

Nyholm force-pushed the toolbar branch from 07f07ad to 8155b31 Compare January 8, 2016 16:15

dbu reviewed Jan 9, 2016
View reviewed changes

Nyholm force-pushed the toolbar branch 2 times, most recently from 5a17b31 to 5f71398 Compare January 9, 2016 12:30

sagikazarmark reviewed Jan 9, 2016
View reviewed changes

Nyholm force-pushed the toolbar branch from 9d38d87 to 63679be Compare January 9, 2016 14:19

Added web profiler and toolbar

633209f

Nyholm force-pushed the toolbar branch from 63679be to 633209f Compare January 10, 2016 19:11

Style fixes

d32e4b1

dbu added a commit that referenced this pull request Jan 10, 2016

Merge pull request #19 from Nyholm/toolbar

4760142

Added web profiler and toolbar

dbu merged commit 4760142 into php-http:master Jan 10, 2016

dbu mentioned this pull request Jan 10, 2016

DataCollector for the symfony toolbar #3

Closed

Nyholm deleted the toolbar branch January 10, 2016 22:01

dbu reviewed Jan 11, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added web profiler and toolbar #19

Added web profiler and toolbar #19

Nyholm commented Jan 8, 2016

dbu Jan 8, 2016

Nyholm Jan 8, 2016

dbu Jan 8, 2016

dbu commented Jan 8, 2016

Nyholm commented Jan 8, 2016

dbu Jan 8, 2016

dbu Jan 8, 2016

Nyholm Jan 8, 2016

dbu Jan 8, 2016 via email

Nyholm Jan 8, 2016

Nyholm Jan 8, 2016

sagikazarmark Jan 8, 2016

Nyholm Jan 8, 2016

dbu Jan 9, 2016

Nyholm Jan 9, 2016

sagikazarmark Jan 9, 2016

Nyholm commented Jan 9, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

dbu commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

dbu Jan 11, 2016

Nyholm Jan 11, 2016

dbu Jan 11, 2016 via email

dbu Jan 11, 2016

Added web profiler and toolbar #19

Added web profiler and toolbar #19

Conversation

Nyholm commented Jan 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jan 8, 2016

Nyholm commented Jan 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu Jan 8, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Jan 9, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

dbu commented Jan 10, 2016

sagikazarmark commented Jan 10, 2016

Nyholm commented Jan 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu Jan 11, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment