[2.2][Profiler] relying on config of displayed profile instead of current config #3373

Merged
merged 5 commits into from Jul 2, 2012

Conversation

Projects
None yet
4 participants
Contributor

wodor commented Feb 15, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: code of ProfilerController is not covered by any test
Fixes the following tickets: #3372
Todo: ~

This fixes the exception which is raised when viewed profile has other data collectors than in config of currently run profiler.
explained here
#3372

Owner

fabpot commented Feb 16, 2012

This should probably be done on the 2.0 branch. Also, I think we need to check if the panel is actually available in the current profiler (if not, we won't be able to display it anyway). So, both checks are important.

Owner

wodor commented on abd0eb7 Feb 16, 2012

I thought passing over two arguments (profile and profiler) through 3 methods isn't nice, also I wanted to be able to write tests, so I've pulled out that methods. Although I'm not happy with the name of this class (Profiler\Template).

If this change is too invasive i can revert to passing profile and profiler.

How to move these changes to 2.0 easily ?

- return $this->container->get('templating')->renderResponse($this->getTemplateName($profiler, $panel), array(
+ /** @var $templateManager \Symfony\Bundle\WebProfilerBundle\Profiler\Template */
+ $templateManager = $this->container->get('web_profiler.profiler_template');
+ $templateManager->setProfiler($profiler);
@stof

stof Feb 16, 2012

Member

why doing the injection of the deps by hand ? This is what the container is for

+use Symfony\Bundle\TwigBundle\TwigEngine;
+
+/**
+ * ProfilerController.
@stof

stof Feb 16, 2012

Member

wrong phpdoc

+ * @author Fabien Potencier <fabien@symfony.com>
+ * @author Artur Wielogórski <wodor@wodor.net>
+ */
+class Template {
@stof

stof Feb 16, 2012

Member

When using it, you name the variable TemplateManager. Why naming the class Template ? It does not represent a template.

@wodor

wodor Feb 18, 2012

Contributor

Template is bad name, I was planning to change the name, but I hated TemplateManager too.
For now I moved it to TemplateManager, I'll appreciate better suggestion

+ /**
+ * @var array
+ */
+ public $templates;
@stof

stof Feb 16, 2012

Member

these should be private or protected, not public

Contributor

wodor commented Feb 18, 2012

defects mentioned by Stof are fixed

+ * template should be loaded for 'foo' because other collectors are
+ * missing in profile or in profiler
+ */
+ public function testGetTemplates() {
@stof

stof Feb 18, 2012

Member

please fix the CS. The curly brace should be on its own line

+class TemplateManager {
+
+ /**
+ * @var \Symfony\Bundle\TwigBundle\TwigEngine
@vicb

vicb Mar 22, 2012

Contributor

you don't need to specify the FQCN when useing a class

fabpot added a commit that referenced this pull request Jul 2, 2012

merged branch wodor/profiler_rely_on_profile_3372 (PR #3373)
Commits
-------

1472283 fixed CS
bc73487 renamed template to TemplateManager , moved profiler to the deps of manager
5fd6ed6 properties protected
abd0eb7 generating template names moved out from controller  to another class
6138e80 [Profiler] relying on config of displayed profile  instead of current config.

Discussion
----------

[2.2][Profiler] relying on config of displayed profile  instead of current config

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: code of ProfilerController is not covered by any test
Fixes the following tickets: #3372
Todo: ~

This fixes the exception which is raised when viewed profile has other  data collectors than in config of currently run profiler.
explained here
#3372

---------------------------------------------------------------------------

by fabpot at 2012-02-16T06:11:00Z

This should probably be done on the 2.0 branch. Also, I think we need to check if the panel is actually available in the current profiler (if not, we won't be able to display it anyway). So, both checks are important.

---------------------------------------------------------------------------

by wodor at 2012-02-18T10:15:40Z

defects mentioned by Stof are fixed

@fabpot fabpot merged commit 1472283 into symfony:master Jul 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment