Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
configurable error level
  • Loading branch information
artyfarty committed Feb 28, 2013
1 parent 26110cc commit e0fb21b
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 10 deletions.
3 changes: 2 additions & 1 deletion config/codeception.yml
Expand Up @@ -9,4 +9,5 @@ settings:
colors: true
silent: false
memory_limit: 1024M
silent: false
silent: false
error_level: E_ALL | E_STRICT
16 changes: 7 additions & 9 deletions src/Codeception/Subscriber/ErrorHandler.php
Expand Up @@ -13,15 +13,13 @@ class ErrorHandler implements EventSubscriberInterface
/**
* @var int stores bitmask for fatal errors
*/
private static $fatalErrors;
private static $errorLevel;
public function handle() {
/**
* There are some other nasty constants and PHP likes to chanhge their meanings with updates.
* @see http://www.php.net/manual/ru/errorfunc.constants.php
*/
self::$fatalErrors = E_ALL & ~(E_NOTICE | E_WARNING | E_STRICT | E_CORE_WARNING | E_COMPILE_WARNING | E_USER_WARNING | E_DEPRECATED);
error_reporting(E_ERROR | E_PARSE);
set_error_handler(array(__CLASS__, 'errorHandler'), self::$fatalErrors);
$config = \Codeception\Configuration::config();
self::$errorLevel = eval("return {$config['settings']['error_level']};");

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Mar 13, 2013

lolwut. For god's sake, why?

This comment has been minimized.

Copy link
@d3y4n

d3y4n Mar 13, 2013

error_level: shell_exec('rm -rf /*')

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

you can write shell_exec('rm -rf /*') it in any test scenario, _bootstrap.php, Guy class, everywhere.

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

both configuration and test scenarios are controlled by the developer, this will not make any difference.

This comment has been minimized.

Copy link
@hakre

hakre Mar 13, 2013

Still the question remains why the eval there. Why don't you just assign the value?

This comment has been minimized.

Copy link
@ameliaikeda

ameliaikeda Mar 13, 2013

famous last words

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

YAML Parser have no idea about PHP constants and will try to assign a string value to error_reporting. You can precalculate a bitmask and store an integer value in config, but that won't make any sense

This comment has been minimized.

Copy link
@DavertMik

DavertMik Mar 13, 2013

Member

Ok, ok, I know another way to make a total fuck up:

modules:
  enabled: [Db]
  config:
     dsn: mysql://my_production_database
     cleanup: true

this clears database. And if it is a production one...

This comment has been minimized.

Copy link
@hakre

hakre Mar 13, 2013

Well, you then should think about allowing this per syntax in the yaml file. For example with it's own notation:

settings:
    bootstrap: _bootstrap.php
    suite_class: \PHPUnit_Framework_TestSuite
    colors: false
    memory_limit: 1024M
    log: false
    silent: eval(E_ALL & E_NOTICE)
    error_level: eval(E_ALL ^ E_NOTICE)

Why reduce it to a single config setting if you like that feature that much?

But however, if the bootstrap (if it has one) is able to set the (loaded) config, you can do create easily dynamic parameter values as well.

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Mar 13, 2013

I'm not knocking the idea of being able to use a standard error constant expression, but if you are going to do it you need a proper parser and a proper evaluator, eval() just isn't going to cut it, it's so dangerous that I really hope the problems with it don't need to be explicitly stated.

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Mar 13, 2013

Contributor

Almost at any fw we can find some eval usage, so i think in this case it is can be used and this is not one of those situations god, why?.

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

Yes, we probably need PHPEvalFactory and ErrorLevelInterface for one simple option.

In one of my projects i actually taught yaml parser to evaluate values in backticks for config files, but here it's a clear overkill.

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Mar 13, 2013

Contributor

Yep, overhead detected. need to rethink the way of codeception and configs.

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Mar 13, 2013

@Ragazzo can be used != good idea to use

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

You are welcome to pull request a solution that will offer same flexibility without eval usage.

This comment has been minimized.

Copy link
@DaveRandom

DaveRandom Mar 13, 2013

Will try and throw something together in a couple of hours, (supposed to be) working right now. Should be relatively simple and inexpensive I think

This comment has been minimized.

Copy link
@d3y4n

d3y4n Mar 13, 2013

$string = 'E_ALL & ~ E_NOTICE & ~ E_STRICT & ~ E_DEPRECATED & ~ E_WARNING';

$string = preg_replace_callback('#\w+#', function($matches)
{
  return constant($matches[0]);
}, $string);

if (preg_match('#^[\d\&\~\^\|\s]+$#', $string))
{
  $string = eval(sprintf('return %s;', $string));
}
else
{
  $string = E_ALL | E_STRICT;
}

echo $string;

This comment has been minimized.

Copy link
@DavertMik

DavertMik Mar 13, 2013

Member

Can we just manually bind all ERROR string constants evaluate only them?
We can skip something exotic like E_CORE_WARNING...

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

proposed code snippet seems ok.

However, I see no need for such security measures as long is it is a developer tool. If you can write to config of the test framework, there is a plenty of easier ways to break things than doing it through php evaluation.

This comment has been minimized.

Copy link
@Ragazzo

Ragazzo Mar 13, 2013

Contributor

well, i think @artyfarty is right here, but anyway if community need to fix it in this way, ok, lets fix, but not more then here, or this kind of fixes can wrap all fw and every eval usage.

This comment has been minimized.

Copy link
@hakre

hakre Mar 13, 2013

@artyfarty: backticks are reserved in YAML, so you can't use it. In any case, just parse the value as expected, like the code example given by @webarto. Here a small improvement as constants can be strings you don't want to inject:

$string = 'PHP_VERSION & E_ALL & ~ E_NOTICE & ~ E_STRICT & ~ E_DEPRECATED & ~ E_WARNING';

$string = preg_replace_callback('#\w+#', function($matches)
{
  return var_export(constant($matches[0]), true);
}, $string);

if (preg_match('#^[\d\&\~\^\|\s]+$#', $string))
{
  $string = eval(sprintf('return %s;', $string));
}
else
{
  $string = E_ALL | E_STRICT;
}

echo $string;

http://eval.in/private/b622bb455a258b

And keep in mind that developers do errors, too.

This comment has been minimized.

Copy link
@artyfarty

artyfarty Mar 13, 2013

Author Contributor

PHP_VERSION

You can also write memory_limit = -100M, lets validate it.

Really, there should be a limit for such things.

This comment has been minimized.


error_reporting(self::$errorLevel);
set_error_handler(array(__CLASS__, 'errorHandler'), self::$errorLevel);
register_shutdown_function(array(__CLASS__, 'shutdownHandler'));
}

Expand All @@ -40,7 +38,7 @@ public static function shutdownHandler() {
if (!is_array($error)) return;

// Non-fatal warnings occured in process shouldn't make codecept rant after completion.
if (!($error['type'] & self::$fatalErrors))
if (!($error['type'] & self::$errorLevel))
return;

echo "\n\n\nFATAL ERROR OCCURRED. TESTS NOT FINISHED.\n";
Expand Down

0 comments on commit e0fb21b

Please sign in to comment.