[Debug] added the component (closes #6828, closes #6834, closes #7330) #7441

Merged
merged 7 commits into from Apr 7, 2013

Conversation

Projects
None yet
5 participants
Owner

fabpot commented Mar 20, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #6828, #6834, #7330
License MIT
Doc PR symfony/symfony-docs#2479

You can use the individual tools, or register them all:

use Symfony\Component\Debug\Debug;

Debug::enable();

Changes in Symfony SE: symfony/symfony-standard#523

Owner

fabpot commented Mar 20, 2013

This is just a quick start to begin the discussion.

@stof stof and 1 other commented on an outdated diff Mar 20, 2013

src/Symfony/Component/Debug/composer.json
+ "authors": [
+ {
+ "name": "Fabien Potencier",
+ "email": "fabien@symfony.com"
+ },
+ {
+ "name": "Symfony Community",
+ "homepage": "http://symfony.com/contributors"
+ }
+ ],
+ "require": {
+ "php": ">=5.3.3"
+ },
+ "autoload": {
+ "psr-0": { "Symfony\\Component\\Debug\\": "" }
+ },
@stof

stof Mar 20, 2013

Member

It need to suggest (and probably require-dev as well) HttpFoundation (required to use the ExceptionHandler) and PsrLog (optionally used by the ErrorHandler to log deprecations)

@fabpot

fabpot Mar 20, 2013

Owner

Sure, I did not added anything yet as this is just to start the discussion. And dependencies is indeed one topic.

@stof

stof Mar 20, 2013

Member

Would it be possible to eliminate the dependency to HttpFoundation to make it standalone ?

@fabpot

fabpot Mar 20, 2013

Owner

In fact, it is not required but if you have it, you have some extra features. The same goes with HttpKernel (and the special handling of HTTP exception classes).

@stof

stof Mar 20, 2013

Member

The ExceptionHandler has a hard dependency on the Response class. Its handle method is $this->createResponse($exception)->send();

@fabpot

fabpot Mar 20, 2013

Owner

That's not a hard dep as you can get the content and the css directly.

@stof

stof Mar 20, 2013

Member

The response is not a hard dependency of the FlattenException, this is true. But it is a hard dependency of the ExceptionHandler. You cannot register the exception handler without HttpFoundation

@stof

stof Mar 20, 2013

Member

well, it is a hard dependency if we want to use it as an exception handler (which is its goal)

@stof stof commented on the diff Mar 20, 2013

src/Symfony/Component/Debug/composer.json
@@ -0,0 +1,31 @@
+{
+ "name": "symfony/debug",
+ "type": "library",
+ "description": "Symfony Debug Component",
+ "keywords": [],
@stof

stof Mar 20, 2013

Member

you should add some keywords so that it can be found more easily through the Packagist search

Member

stof commented Mar 20, 2013

Apart from my comments,, this looks good to me.

and it is great to have the deprecation handling standalone. It means it is possible to trigger E_USER_DEPRECATED in libraries when deprecating something and users can get the handling without having to use HttpKernel. 👍

@bamarni bamarni commented on the diff Mar 20, 2013

UPGRADE-3.0.md
@@ -34,6 +34,16 @@ UPGRADE FROM 2.x to 3.0
* `Symfony\Bridge\Monolog\Logger`
* `Symfony\Component\HttpKernel\Log\NullLogger`
+ * The `Symfony\Component\HttpKernel\Kernel::init()` method has been removed.
@bamarni

bamarni Mar 20, 2013

Contributor

Could this be reconsidered (or at least rediscussed)? maybe in a separate PR (as this is not a blocking step for creating this standalone component)?

@bamarni bamarni commented on the diff Mar 20, 2013

src/Symfony/Component/HttpKernel/Kernel.php
@@ -94,21 +91,11 @@ public function __construct($environment, $debug)
$this->init();
}
+ /**
+ * @deprecated Deprecated since version 2.3, to be removed in 3.0. Move your logic in the constructor instead.
@bamarni

bamarni Mar 20, 2013

Contributor

This can't be moved to the constructor AFAIK because it'd make issues when unserializing a kerne (#6834)l, unless it has been fixed and I've missed it.

But i thought you wanted to move the logic to front controllers?

@stof

stof Mar 20, 2013

Member

@bamarni your custom logic should be moved to the constructor, so that it is still called once the call to init is removed from the constructor.

As you can see, the kernel is not responsible to register the debug error handler anymore.

@bamarni

bamarni Mar 20, 2013

Contributor

I don't understand this change, it looks quite natural to me that a boolean parameter called "debug" enables debug stuff, IIUC on the standard edition you're going to move all this code to web/app_dev.php and also duplicate it to app/console too (in a special condition on the environment, not the debug mode...)?

Currently if a page doesn't show up in prod mode but is ok in dev mode (for whatever reason, prod cache dir not writable or anything else), I'd just switch the flag from false to true and expect the ghost to give me some tips, it wouldn't be possible anymore after this change, for any PHP / Symfony beginner it will become much more complicated to handle such situations.

@stof

stof Mar 21, 2013

Member

@bamarni Registering an error handler in the kernel constructor is causing issues, as it is registered several times if you instantiate it several times.

@bamarni

bamarni Mar 23, 2013

Contributor

@fabpot @stof : it wouldn't with this helper class @b29ca150ffc53e23d85332103b35c9d0e466a44a, even though I still prefer the current way, I won't argue with this deprecation, the helper class is an acceptable compromise to me.

It remains that this is a BC break (about what a user currently expects from the debug flag), while the PR states it is BC, what about enabling the debug helper in here until 3.0? It doesn't register things several times anymore.

@mpdude

mpdude Mar 31, 2013

Contributor

@fabpot @stof If I understand that correctly, initialization of the debug stuff is no longer performed when the Kernel constructor is called with debug = true. Instead, Debug::enable() is to be called from the front controller? That should probably be reflected in the symfony-standard app_dev.php?

@fabpot

fabpot Apr 1, 2013

Owner

That will be changed when this PR is merged.

@fabpot

fabpot Apr 7, 2013

Owner

The changes for Symfony SE is here: symfony/symfony-standard#523

Contributor

bamarni commented Mar 21, 2013

@fabpot : your last commits are awesome, I have no objection anymore :)

@stof stof commented on an outdated diff Mar 21, 2013

src/Symfony/Component/Debug/.gitignore
@@ -0,0 +1,5 @@
+vendor/
+composer.lock
+phpunit.xml
+Tests/ProjectContainer.php
+Tests/classes.map
@stof

stof Mar 21, 2013

Member

These are not necessary AFAIK. They are left-over of the HttpKernel copy-paste

@stof stof commented on an outdated diff Mar 21, 2013

src/Symfony/Component/Debug/Debug.php
+ *
+ * If the Symfony ClassLoader component is available, a special
+ * class loader is also registered.
+ *
+ * @param integer $errorReportingLevel The level of error reporting you wan
+ */
+ public static function enable($errorReportingLevel = null)
+ {
+ if (static::$enabled) {
+ return;
+ }
+
+ static::$enabled = true;
+
+ error_reporting(-1);
+ ini_set('display_errors', 0);
@stof

stof Mar 21, 2013

Member

This is redundant as the error handler is already doing it

@stof stof commented on an outdated diff Mar 21, 2013

src/Symfony/Component/Debug/composer.json
+ "authors": [
+ {
+ "name": "Fabien Potencier",
+ "email": "fabien@symfony.com"
+ },
+ {
+ "name": "Symfony Community",
+ "homepage": "http://symfony.com/contributors"
+ }
+ ],
+ "require": {
+ "php": ">=5.3.3"
+ },
+ "require-dev": {
+ "symfony/http-kernel": "2.2.*",
+ "symfony/http-foundation": "2.2.*"
@stof

stof Mar 21, 2013

Member

2.2 ?

@bendavies bendavies and 1 other commented on an outdated diff Mar 21, 2013

src/Symfony/Component/Debug/README.md
+
+You can also use the tools individually:
+
+ use Symfony\Component\Debug\ErrorHandler;
+ use Symfony\Component\Debug\ExceptionHandler;
+
+ error_reporting(-1);
+
+ ErrorHandler::register($errorReportingLevel);
+ if ('cli' !== php_sapi_name()) {
+ ExceptionHandler::register();
+ } elseif (!ini_get('log_errors') || ini_get('error_log')) {
+ ini_set('display_errors', 1);
+ }
+
+Not that the `Debug::enable()` call also registers the debug class loader from
@bendavies

bendavies Mar 21, 2013

Contributor

Not -> Note

@bendavies

bendavies Apr 1, 2013

Contributor

@fabpot think you missed this.

@bamarni bamarni commented on the diff Mar 23, 2013

src/Symfony/Component/Debug/Debug.php
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class Debug
+{
+ private static $enabled = false;
+
+ /**
+ * Enables the debug tools.
+ *
+ * This method registers an error handler and an exception handler.
+ *
+ * If the Symfony ClassLoader component is available, a special
+ * class loader is also registered.
+ *
+ * @param integer $errorReportingLevel The level of error reporting you wan
@bamarni

bamarni Mar 23, 2013

Contributor

want

fabpot referenced this pull request in symfony/symfony-docs Apr 7, 2013

Merged

added documentation for the new Debug component #2479

@fabpot fabpot added a commit that referenced this pull request Apr 7, 2013

@fabpot fabpot merged branch fabpot/debug-component (PR #7441)
This PR was merged into the master branch.

Discussion
----------

[Debug] added the component (closes #6828, closes #6834, closes #7330)

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #6828, #6834, #7330
| License       | MIT
| Doc PR        | symfony/symfony-docs#2479

You can use the individual tools, or register them all:

```php
use Symfony\Component\Debug\Debug;

Debug::enable();
```

Changes in Symfony SE: symfony/symfony-standard#523

Commits
-------

f693128 fixed typos
1ab1146 [Debug] fixed minor bugs
daa3a3c [Debug] changed composer to accept more versions
e455269 [Debug] ensured that the Debug tools can only be registered once
946bfb2 [Debug] made the exception handler independant of HttpFoundation
2b305c2 added a main Debug class to ease integration
2ff0927 [Debug] added the component (closes #6828, closes #6834, closes #7330)
6d552c9

fabpot closed this Apr 7, 2013

@fabpot fabpot merged commit f693128 into symfony:master Apr 7, 2013

1 check passed

default Scrutinizer: No Comments — Travis: Pending
Details

mpdude referenced this pull request in symfony/symfony-docs Apr 7, 2013

Closed

Add a section on front controllers and the AppKernel #2465

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