codeclimate / codeclimate-phpmd

Code Climate PHPMD Engine
MIT License
10 stars 12 forks source link

Revert "Fix uncatchable exception" #28

Closed wfleming closed 8 years ago

wfleming commented 8 years ago

Reverts codeclimate/codeclimate-phpmd#27

A canary in our systems caught that this change did not seem to have the intended effect: exceptions that were previously being caught were no longer being caught.

@landrok I also tried removing the use line and changing to catch(\Exception ... instead: that also didn't seem to actually catch the exceptions. Might this be a difference between PHP versions? I'm going to go ahead with the revert to get master back to a stable state, but please let me know if you have more thoughts about this.

FYI @codeclimate/review

landrok commented 8 years ago

Indeed, that was what I wanted to point. Here, the try...catch has no effect unless you catch the exception and the way it was written does not catch anything because if an exception raised it would cause a fatal error. See the script below, it's a minimalist approach of the 2 ways.

<?php

// Emulates 2 runs on the 2 runners
namespace PHPMD\TextUI
{
  use CodeClimate\PHPMD\Runner;
  use CodeClimate\PHPMD\PatchedRunner;
  use CodeClimate\PHPMD\BonusRunner;

  // With patch
  $runner = new PatchedRunner();
  $runner->run();

  // Without patch
  $runner = new Runner();
  $runner->run();
}

#1 - Class with uncatchable exception
namespace CodeClimate\PHPMD
{
  class Runner
  {
    public function run()
    {
      try
      {
        // PHP Fatal error:  Class 'CodeClimate\PHPMD\Exception' not found
        throw new Exception ('#2 - Not catchable -> Fatal error');
      }
      catch (Exception $e) # trying to catch CodeClimate\PHPMD\Exception, never appends because it does not exist
      {
        echo sprintf("Exception: %s in \n %s \n", $e->getMessage(), $e->getFile(), $e->getTraceAsString());
      }
    }
  }
}

#2 class with a catchable exception
namespace CodeClimate\PHPMD
{
  use Exception; # To scope the exception

  class PatchedRunner
  {
    public function run()
    {
      try
      {
        // In the correct namespace
        throw new Exception ('#1 - Catchable');
      }
      catch (Exception $e)
      {
        echo sprintf("Exception: %s in \n %s \n", $e->getMessage(), $e->getFile(), $e->getTraceAsString());
      }
    }
  }
}

#3 - Class with uncatchable exception, less code, same effect than #1
namespace CodeClimate\PHPMD
{
  class BonusRunner
  {
    public function run()
    {
      // PHP Fatal error:  Class 'CodeClimate\PHPMD\Exception' not found
      throw new Exception ('#3 - Not catchable  but less code');
    }
  }
}
?>

Ultimately, a test by removing the try...catch could confirm this and speed up the execution, a little bit. You can test with replacing 2nd run by new BonusRunner(), you will have the same effect.

If all those thoughts are ok, please let me know, I have a more proper patch for that (bubbling-up).

I can not access to https://github.com/orgs/codeclimate/teams/review. Although I already have an idea, I am interested in taking a look at the impacts.