Skip to content
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

Use xdebug for error/exception handling #2741

Conversation

erikhansen
Copy link
Contributor

Xdebug does a great job at displaying backtraces for exceptions/errors. However Magento 2 implements its own error/exception handling that prevents a developer from experiencing the full benefits of Xdebug's backtrace handling.

This pull request modifies the \Magento\Framework\App\Bootstrap::run method so that if developer mode and xdebug are enabled, it will run the application outside the context of a try/catch block (this is necessary in order for the exception backtraces to be accurate). Update: after actively developing for the past few weeks with this PR applied to my local M2 install, I realized that I needed to modify the Bootstrap::run method to only allow Xdebug to handle exceptions when the request comes through index.php. See 4e7be01 for more details.

Note: I attempted to also make Xdebug handle error backtraces by not calling the $this->initErrorHandler(); method when Xdebug was installed and developer mode was on. However doing this resulted in numerous E_WARNING messages being output to screen that were otherwise handled by exceptions being caught. See \Magento\Framework\View\Model\Layout\Update\Validator::isValid for an example of a method that depends on this.

This PR includes a test that covers the new \Magento\Framework\App\Bootstrap::ignoreExceptionsInDeveloperMode method.

Here are example screenshots comparing the before/after backtrace of exceptions:

Exception - before

exception_before

Exception - after

exception_after

Erik Hansen added 4 commits December 16, 2015 16:01
- If developer mode is enabled and xdebug is enabled, use xdebug to
  handle printing exceptions and errors
- Error handling is now always done by Magento, as certain methods
  inside Magento framework require errors to be handled as exceptions.
  For example, the \Magento\Framework\View\Model\Layout\Update\Validator::isValid
  method will output errors on to the page if the Magento error handler
  is not initialized.
- Prelaunch code is always run within a try/catch block (even if
  $this->ignoreExceptionsInDeveloperMode() is true), as certain scenarios
  such as maintenance mode and "not installed"  must be handled by
  \Magento\Framework\AppInterface::catchException
…ex.php

- Since the Bootstrap::run method is called from files like get.php and
  static.php, it is important that exceptions result in 404 headers,
  even when in developer mode.
@mazhalai
Copy link
Contributor

@erikhansen Please merge latest from develop and rerun builds.

@erikhansen
Copy link
Contributor Author

@mazhalai I merged in develop, but the tests are still failing for errors that look like the same issues that have been present for the past few weeks: https://travis-ci.org/magento/magento2/builds/100899663

@mazhalai
Copy link
Contributor

@erikhansen These issues were fixed and merged in, its all green now https://travis-ci.org/magento/magento2

@erikhansen
Copy link
Contributor Author

@mazhalai It looks like you restarted the failed tests yesterday. Commit e1b7552 is now green, although commit be51c1c had a single error that was not caused by any code in that commit. I am guessing that if you restart the test for be51c1c, it will be green.

@erikhansen
Copy link
Contributor Author

@mazhalai It looks like the most recent commit is now green.

@vancoz
Copy link

vancoz commented Jan 19, 2016

Hello @erikhansen, thank you for your contribution. The idea to have better exception presentation and displaying is really good, but current implementation can not be accepted.

  1. we need to disable Magento error handler (in development mode);
  2. we need to change interface of \Magento\Framework\App\Bootstrap::run method;

Probably it is possible to use xdebug interface to pass exeption and get formated output, it can be solution.

@erikhansen
Copy link
Contributor Author

@vancoz I'm glad that you are supportive of better exception presentation. I would like to continue to work on this pull request until it is able to be merged.

1 — When you said that "we need to disable Magento error handler", are you referring to this comment:

/**
 * It is necessary to initialize error handler even when in developer mode as certain methods require
 * errors to be converted into catchable exceptions.
 * For example @see \Magento\Framework\View\Model\Layout\Update\Validator::isValid
*/
$this->initErrorHandler();

Specifically, are you suggesting that if in developer mode (and Xdebug is installed), the $this->initErrorHandler(); method should not run? If that is what you mean, then a fair amount of work will need to be done in order to accomplish this. However I would definitely like to see this happen, as it would mean that both exception AND errors would be handled by Xdebug.

2 — What do you mean by "we need to change interface of \Magento\Framework\App\Bootstrap::run method"?

It sounds like you are suggesting that we could continue to have Magento catch all Exceptions and then use Xdebug to print those exceptions in a better format than the native Magento way? I looked through the Xdebug documentation and it doesn't look like this is possible. On a previous project, I used the https://github.com/Xethron/to-string#exception-to-string library to output pull information from an Exception. However I think that the approach I took in my pull request makes the most sense, as it fully delegates exception handling to Xdebug and doesn't require a dependency on a third-party library like Xethron to format an Exception.

@alankent
Copy link

Thanks Erik, let me know if you need any deeper support - I am keen to get some improvements too if we can,

@erikhansen
Copy link
Contributor Author

@alankent Thanks Alan! I was hoping you'd see this pull request as I expected you'd be in favor of the functionality it accomplishes.

@erikhansen
Copy link
Contributor Author

@vancoz Have you had a chance to review my reply yet?

@erikhansen
Copy link
Contributor Author

@vancoz Bumping this PR.

@vancoz
Copy link

vancoz commented Feb 26, 2016

Hi @erikhansen let me ask to review your PR internally.

@vancoz vancoz added the PS label Feb 26, 2016
* @return void
*/
public function run(AppInterface $application)
public function run(AppInterface $application, $alwaysHandleExceptions = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since some applications require special exception handling, why not use interface polymorphism that is in place already instead of introducing optional argument and IF here? We already have $application->catchException.

@mazhalai mazhalai removed their assignment Mar 3, 2016
@vancoz
Copy link

vancoz commented Mar 3, 2016

Hi @erikhansen, we have reviewed your PR and @antonkril has comment (please see code comments).
Could you please address it?
Thanks you.

@vancoz
Copy link

vancoz commented Mar 22, 2016

Hi @erikhansen, I closed the PR because we did not receive response from you.
If you wish to address @antonkril 's comment and update PR - feel free to do it and reopen PR.

Thank you!

@vancoz vancoz closed this Mar 22, 2016
magento-engcom-team pushed a commit that referenced this pull request Jun 27, 2018
[borg] MAGETWO-86795: Admin Create Customer functional test fails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants