Skip to content
Permalink
Browse files

Improve method mapping in BehaviorRegistry.

* Omit methods on base class.
* Use ReflectionClass instead of global functions.
* Ignore methods starting with _. This omits methods like __construct
  and other PHP magic methods.
* Make method lookups case insensitive as this is how PHP works
  internally as well.
  • Loading branch information...
markstory committed Oct 28, 2013
1 parent e37a70a commit 193f89415657b9bdbbdec9bb5532d1f733768618
@@ -121,41 +121,54 @@ protected function _create($class, $alias, $settings) {
* Store the map of behavior methods and ensure there are no duplicates.
*
* Use the implementedEvents() method to exclude callback methods.
* In addition methods starting with `_` will be ignored, as will
* method declared on Cake\ORM\Behavior
*
* @param Cake\ORM\Behavior $instance
* @return void
* @throws Cake\Error\Exception when duplicate methods are connected.
*/
protected function _mapMethods(Behavior $instance, $class, $alias) {
$events = $instance->implementedEvents();
$methods = get_class_methods($instance);
$reflection = new \ReflectionClass($instance);

This comment has been minimized.

Copy link
@AD7six

AD7six Nov 3, 2013

Member

I think this should be encapsulated as a public method on the behavior parent/abstract class.

Something like:

public function implementedMethods(Table $table) {
    ...
    $reflection = new \ReflectionClass($this);
    return $reflection->getMethods(\ReflectionMethod::IS_PUBLIC);
}

This would permit, for example, implementing a public method that is not intended to be accessible via a model using the behavior.

But, perhaps it shouldn't stop there. Should it be possible to alias a behavior method? so that you can route Table->foo() to SomeBehavior->bar(...) ?

In the code below an exception is thrown if two behaviors are bound implementing the same method names - in certain circumstances I don't expect that to be a good thing, the "best" example that comes to mind would be the tree behavior and a "list" behavior - both implementing a "move up" function (but - "move up" in a tree with only a parent_id field would be disabled/inaccessible). Alternatively it will lead to behaviors needing to force unique public method names to avoid the risk of causing exceptions when used with random-other-behavior. IMO having a duplicate method should at most trigger a warning so that the developer knows about it (it's trivial enough to implement a table method to make an inaccessible behavior method accessible, whereas it's not trivial to ignore that using x behavior with y behavior simply doesn't work).

If it were possible to alias behavior methods - then throwing an exception seems appropritate - since userland code can account for it.

This comment has been minimized.

Copy link
@markstory

markstory Nov 3, 2013

Author Member

Seems reasonable that the reflection logic for a behavior would be in the behavior. Would you be able to put together a pull request of how you think it should work?

This comment has been minimized.

Copy link
@AD7six

AD7six Nov 3, 2013

Member

sure thing

$eventMethods = [];
foreach ($events as $e => $binding) {
if (is_array($binding) && isset($binding['callable']) && isset($binding['callable'])) {
$binding = $binding['callable'];
}
$index = array_search($binding, $methods);
unset($methods[$index]);
$eventMethods[$binding] = true;
}
foreach ($methods as $method) {
$isFinder = substr($method, 0, 4) === 'find';
if (($isFinder && isset($this->_finderMap[$method])) || isset($this->_methodMap[$method])) {
foreach ($reflection->getMethods(\ReflectionMethod::IS_PUBLIC) as $method) {
if ($method->getDeclaringClass()->getName() === 'Cake\ORM\Behavior') {
continue;
}
$methodName = $method->getName();
if (strpos($methodName, '_') === 0 || isset($eventMethods[$methodName])) {
continue;
}
$methodName = strtolower($methodName);
if (isset($this->_finderMap[$methodName]) || isset($this->_methodMap[$methodName])) {
$error = __d(
'cake_dev',
'%s contains duplicate method "%s" which is already provided by %s',
$class,
$method,
$this->_methodMap[$method]
$method->getName(),
$this->_methodMap[$methodName]
);
throw new Error\Exception($error);
}
$isFinder = substr($methodName, 0, 4) === 'find';
if ($isFinder) {
$this->_finderMap[$method] = $alias;
$this->_finderMap[$methodName] = $alias;
} else {
$this->_methodMap[$method] = $alias;
$this->_methodMap[$methodName] = $alias;
}
}
return $instance;
}
/**
@@ -168,6 +181,7 @@ protected function _mapMethods(Behavior $instance, $class, $alias) {
* @return boolean
*/
public function hasMethod($method) {
$method = strtolower($method);
return isset($this->_methodMap[$method]);
}
@@ -181,6 +195,7 @@ public function hasMethod($method) {
* @return boolean
*/
public function hasFinder($method) {
$method = strtolower($method);
return isset($this->_finderMap[$method]);
}
@@ -193,14 +208,17 @@ public function hasFinder($method) {
* @throws Cake\Error\Exception When the method is unknown.
*/
public function call($method, array $args = []) {
$method = strtolower($method);
if ($this->hasMethod($method)) {
$alias = $this->_methodMap[$method];
return call_user_func_array([$this->_loaded[$alias], $method], $args);
}
if ($this->hasFinder($method)) {
$alias = $this->_finderMap[$method];
return call_user_func_array([$this->_loaded[$alias], $method], $args);
}
throw new Error\Exception(__d('cake_dev', 'Cannot call "%s" it does not belong to any attached behaviors.', $method));
}
@@ -16,4 +16,8 @@
use Cake\ORM\Behavior;
class PersisterOneBehavior extends Behavior {
public function persist() {
}
}
@@ -55,10 +55,14 @@ public function tearDown() {
* @return void
*/
public function testLoad() {
Plugin::load('TestPlugin');
$settings = ['alias' => 'Sluggable', 'replacement' => '-'];
$result = $this->Behaviors->load('Sluggable', $settings);
$this->assertInstanceOf('TestApp\Model\Behavior\SluggableBehavior', $result);
$this->assertEquals($settings, $result->settings);
$result = $this->Behaviors->load('TestPlugin.PersisterOne');
$this->assertInstanceOf('TestPlugin\Model\Behavior\PersisterOneBehavior', $result);
}
/**
@@ -130,6 +134,12 @@ public function testLoadDuplicateMethodError() {
public function testHasMethod() {
$this->Behaviors->load('Sluggable');
$this->assertTrue($this->Behaviors->hasMethod('slugify'));
$this->assertTrue($this->Behaviors->hasMethod('SLUGIFY'));
$this->assertFalse($this->Behaviors->hasMethod('__construct'));
$this->assertFalse($this->Behaviors->hasMethod('settings'));
$this->assertFalse($this->Behaviors->hasMethod('implementedEvents'));
$this->assertFalse($this->Behaviors->hasMethod('nope'));
$this->assertFalse($this->Behaviors->hasMethod('beforeFind'));
$this->assertFalse($this->Behaviors->hasMethod('findNoSlug'));
@@ -144,9 +154,11 @@ public function testHasFinder() {
$this->Behaviors->load('Sluggable');
$this->assertTrue($this->Behaviors->hasFinder('findNoSlug'));
$this->assertFalse($this->Behaviors->hasFinder('findnoslug'));
$this->assertFalse($this->Behaviors->hasFinder('findnoslug'));
$this->assertTrue($this->Behaviors->hasFinder('findnoslug'));
$this->assertTrue($this->Behaviors->hasFinder('FINDNOSLUG'));
$this->assertFalse($this->Behaviors->hasFinder('slugify'));
$this->assertFalse($this->Behaviors->hasFinder('beforeFind'));
$this->assertFalse($this->Behaviors->hasFinder('nope'));
}

0 comments on commit 193f894

Please sign in to comment.
You can’t perform that action at this time.