Skip to content

Conversation

wfleming
Copy link
Contributor

@wfleming wfleming commented Oct 6, 2016

Reverts #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

@wfleming wfleming merged commit ae8605e into master Oct 6, 2016
@wfleming wfleming deleted the revert-27-patch-4 branch October 6, 2016 22:45
@landrok
Copy link
Contributor

landrok commented Oct 7, 2016

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.

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

Labels

None yet

2 participants