Skip to content

Added web profiler and toolbar #19

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

Merged
merged 2 commits into from
Jan 10, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions Collector/MessageJournal.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
<?php

namespace Http\HttplugBundle\Collector;

use Http\Client\Exception;
use Http\Client\Plugin\Journal;
use Http\Message\Formatter;
use Http\Message\Formatter\SimpleFormatter;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\DataCollector\DataCollector;

/**
* @author Tobias Nyholm <[email protected]>
*/
class MessageJournal extends DataCollector implements Journal
{
/**
* @var Formatter
*/
private $formatter;

/**
* @param Formatter $formatter
*/
public function __construct(Formatter $formatter = null)
{
$this->formatter = $formatter ?: new SimpleFormatter();
$this->data = ['success' => [], 'failure' => []];
}

/**
* {@inheritdoc}
*/
public function addSuccess(RequestInterface $request, ResponseInterface $response)
{
$this->data['success'][] = [
'request' => $this->formatter->formatRequest($request),
'response' => $this->formatter->formatResponse($response),
];
}

/**
* {@inheritdoc}
*/
public function addFailure(RequestInterface $request, Exception $exception)
{
if ($exception instanceof Exception\HttpException) {
$formattedResponse = $this->formatter->formatResponse($exception->getResponse());
} elseif ($exception instanceof Exception\TransferException) {
$formattedResponse = $exception->getMessage();
} else {
$formattedResponse = sprintf('Unexpected exception of type "%s"', get_class($exception));
}

$this->data['failure'][] = [
'request' => $this->formatter->formatRequest($request),
'response' => $formattedResponse,
];
}

/**
* Get the successful request-resonse pairs.
*
* @return array
*/
public function getSucessfulRequests()
{
return $this->data['success'];
}

/**
* Get the failed request-resonse pairs.
*
* @return array
*/
public function getFailedRequests()
{
return $this->data['failure'];
}

/**
* Get the total number of request made.
*
* @return int
*/
public function getTotalRequests()
{
return count($this->data['success']) + count($this->data['failure']);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

{
// We do not need to collect any data form the Symfony Request and Response
}

/**
* {@inheritdoc}
*/
public function getName()
{
return 'httplug';
}
}
18 changes: 14 additions & 4 deletions DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is styleci

})
->then(function ($v) {
foreach ($v['classes'] as $key => $class) {
Expand Down Expand Up @@ -72,8 +71,19 @@ public function getConfigTreeBuilder()
->scalarNode('stream_factory')->defaultNull()->end()
->end()
->end()
->end()
;
->arrayNode('toolbar')
->addDefaultsIfNotSet()
->info('Extend the debug profiler with inforation about requests.')
->children()
->enumNode('enabled')
->info('If "auto" (default), the toolbar is activated when kernel.debug is true. You can force the toolbar on and off by changing this option.')
->values([true, false, 'auto'])
->defaultValue('auto')
->end()
->scalarNode('formatter')->defaultNull()->end()
->end()
->end()
->end();
Copy link
Collaborator

Choose a reason for hiding this comment

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

same logic here, if we add another section, there will be an unnecessary diff on this line.


return $treeBuilder;
}
Expand Down
18 changes: 17 additions & 1 deletion DependencyInjection/HttplugExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
namespace Http\HttplugBundle\DependencyInjection;

use Http\HttplugBundle\ClientFactory\DummyClient;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
Expand All @@ -27,6 +27,18 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('services.xml');
$loader->load('plugins.xml');
$loader->load('discovery.xml');

$enabled = is_bool($config['toolbar']['enabled']) ? $config['toolbar']['enabled'] : $container->hasParameter('kernel.debug') && $container->getParameter('kernel.debug');
if ($enabled) {
$loader->load('data-collector.xml');
$config['_inject_collector_plugin'] = true;

if (!empty($config['toolbar']['formatter'])) {
$container->getDefinition('httplug.collector.message_journal')
->replaceArgument(0, new Reference($config['toolbar']['formatter']));
}
}

foreach ($config['classes'] as $service => $class) {
if (!empty($class)) {
$container->removeDefinition(sprintf('httplug.%s.default', $service));
Expand Down Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@dbu dbu Jan 11, 2016 via email

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

$def = $container->register('httplug.client.'.$name, DummyClient::class);

if (empty($arguments['plugins'])) {
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@ 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.logger<br> httplug.plugin.redirect<br> httplug.plugin.retry | These are built in plugins that live in the `php-http/plugins` package. These servcies are not public and may only be used when configure HttpClients or services.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets make an explicit note on the history plugin. that plugin needs to be set up with a journal, so you need to define your own service for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made it "internal". Configuration for plugins should be solved by #17

\* *These services are always an alias to another service. You can specify your own service or leave the default, which is the same name with `.default` appended. The default services in turn use the service discovery mechanism to provide the best available implementation. You can specify a class for each of the default services to use instead of discovery, as long as those classes can be instantiated without arguments.*


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
Expand Down
17 changes: 17 additions & 0 deletions Resources/config/data-collector.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<services>
<service id="httplug.collector.message_journal" class="Http\HttplugBundle\Collector\MessageJournal" public="false">
<tag name="data_collector" template="HttplugBundle::webprofiler.html.twig" priority="200"
id="httplug"/>
<argument>null</argument>
</service>

<service id="httplug.collector.history_plugin" class="Http\Client\Plugin\HistoryPlugin" public="false">
<argument type="service" id="httplug.collector.message_journal"/>
</service>
</services>
</container>
3 changes: 1 addition & 2 deletions Resources/config/plugins.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

<services>
<service id="httplug.plugin.content_length" class="Http\Client\Plugin\ContentLengthPlugin" public="false" />
<service id="httplug.plugin.decoder" class="Http\Client\Plugin\DecoderPlugin" public="false">
</service>
<service id="httplug.plugin.decoder" class="Http\Client\Plugin\DecoderPlugin" public="false" />
<service id="httplug.plugin.error" class="Http\Client\Plugin\ErrorPlugin" public="false" />
<service id="httplug.plugin.logger" class="Http\Client\Plugin\LoggerPlugin" public="false">
<argument type="service" id="logger"/>
Expand Down
75 changes: 75 additions & 0 deletions Resources/views/webprofiler.html.twig
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
{% extends '@WebProfiler/Profiler/layout.html.twig' %}

{% import _self as macro %}

{% block toolbar %}
{% if collector.totalRequests > 0 %}
{% set icon %}
{{ include('@WebProfiler/Icon/ajax.svg') }}
<span class="sf-toolbar-status">{{ collector.totalRequests }}</span>
{% endset %}

{% set text %}
<div class="sf-toolbar-info-piece">
<b>Successful requests</b>
<span>{{ collector.sucessfulRequests|length }}</span>
</div>
<div class="sf-toolbar-info-piece">
<b>Faild requests</b>
<span>{{ collector.failedRequests|length }}</span>
</div>

{% endset %}
{% include 'WebProfilerBundle:Profiler:toolbar_item.html.twig' with { 'link': profiler_url } %}
{% endif %}
{% endblock %}

{% block head %}
{# Optional. Here you can link to or define your own CSS and JS contents. #}
{{ parent() }}
{% endblock %}

{% block menu %}
{# This left-hand menu appears when using the full-screen profiler. #}
<span class="label {{ collector.totalRequests == 0 ? 'disabled' }}">
<span class="icon">{{ include('@WebProfiler/Icon/ajax.svg') }}</span>
<strong>Httplug</strong>
</span>
{% endblock %}

{% block panel %}
<h2>HTTPlug</h2>
{% if (collector.failedRequests|length > 0) %}
<h3>Failed requests</h3>
{{ macro.printMessages(collector.failedRequests) }}
{% endif %}

{% if (collector.sucessfulRequests|length > 0) %}
<h3>Successful requests</h3>
{{ macro.printMessages(collector.sucessfulRequests) }}
{% endif %}

{% if collector.totalRequests == 0 %}

<div class="empty">
<p>No request were sent.</p>
</div>
{% endif %}

{% endblock %}

{% macro printMessages(messages) %}
<table>
<tr>
<th>Request</th>
<th>Response</th>
</tr>

{% for message in messages %}
<tr>
<td>{{ message['request'] }}</td>
<td>{{ message['response'] }}</td>
</tr>
{% endfor %}
</table>
{% endmacro %}
2 changes: 1 addition & 1 deletion Tests/Resources/Fixtures/config/empty.php
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<?php

$container->loadFromExtension('httplug', array());
$container->loadFromExtension('httplug', []);
24 changes: 12 additions & 12 deletions Tests/Resources/Fixtures/config/full.php
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
<?php

$container->loadFromExtension('httplug', array(
'main_alias' => array(
'client' => 'my_client',
$container->loadFromExtension('httplug', [
'main_alias' => [
'client' => 'my_client',
'message_factory' => 'my_message_factory',
'uri_factory' => 'my_uri_factory',
'stream_factory' => 'my_stream_factory',
),
'classes' => array(
'client' => 'Http\Adapter\Guzzle6\Client',
'uri_factory' => 'my_uri_factory',
'stream_factory' => 'my_stream_factory',
],
'classes' => [
'client' => 'Http\Adapter\Guzzle6\Client',
Copy link
Member

Choose a reason for hiding this comment

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

Is this styleci as well? AFAIK symfony preset unaligns double arrows, doesn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests is ignored by styleci i think. not sure if we really want to ignore them. consistent CS in the tests sounds like a valuable thing to have.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Well, the reason why I ignored tests by default was that spec tests are snake cased and function names does not contain visibility. I agree it would make sense here, so let's just remove the test paths from exclusions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, That is styleci.
Ignoring sounds like a good idea.

'message_factory' => 'Http\Message\MessageFactory\GuzzleMessageFactory',
'uri_factory' => 'Http\Message\UriFactory\GuzzleUriFactory',
'stream_factory' => 'Http\Message\StreamFactory\GuzzleStreamFactory',
),
));
'uri_factory' => 'Http\Message\UriFactory\GuzzleUriFactory',
'stream_factory' => 'Http\Message\StreamFactory\GuzzleStreamFactory',
],
]);
4 changes: 2 additions & 2 deletions Tests/Resources/app/AppKernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ class AppKernel extends Kernel
*/
public function registerBundles()
{
return array(
return [
new \Symfony\Bundle\FrameworkBundle\FrameworkBundle(),
new \Http\HttplugBundle\HttplugBundle(),
);
];
}

/**
Expand Down
8 changes: 8 additions & 0 deletions Tests/Unit/DependencyInjection/ConfigurationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ public function testEmptyConfiguration()
'stream_factory' => null,
],
'clients' => [],
'toolbar' => [
'enabled' => 'auto',
'formatter' => null,
],
];

$formats = array_map(function ($path) {
Expand Down Expand Up @@ -68,6 +72,10 @@ public function testSupportsAllConfigFormats()
'stream_factory' => 'Http\Message\StreamFactory\GuzzleStreamFactory',
],
'clients' => [],
'toolbar' => [
'enabled' => 'auto',
'formatter' => null,
],
];

$formats = array_map(function ($path) {
Expand Down