-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added AbstractConfigurableAction class and tests, fixed #25 #26
Conversation
uuf6429
commented
Jul 23, 2016
- AbstractConfigurableAction
- Unit test
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function execute($eval, $context, $rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method be final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is not supposed to be replaced by the child class, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Unfortunately, in PHP we don't have method overloading, otherwise this would have been better:
final public function execute($eval, $context, $rule)
{
$configExpr = $this->getConfigExpression();
$config = is_null($configExpr) ? [] : $eval->evaluate($configExpr);
$this->execute($eval, $context, $rule, (array) $config);
}
abstract protected function execute($eval, $context, $rule, $config);
@mstovicek, @arvydasvapsva what do you think? This is very bare-bones class, but should still be a good start. |
Current coverage is 91.89% (diff: 100%)@@ master #26 diff @@
==========================================
Files 23 24 +1
Lines 429 444 +15
Methods 78 80 +2
Messages 0 0
Branches 0 0
==========================================
+ Hits 392 408 +16
+ Misses 37 36 -1
Partials 0 0
|
} | ||
$result[] = sprintf( | ||
'%s: (%s)', | ||
var_export($name, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $name
is scalar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstovicek, this is not a problem. Apparently, one cannot use a complex type as key in a PHP array, so it's pointless to check the type because the value will already be supported.