lithium
lithium copied to clipboard
UnionOfRAD
Model finders can no longer be invoked from within an instance method.
This bug was introduced in e73ffe6 (Issue #744)
Apparently, now that Model has both a __call() and __callStatic() method, when I attempt to invoke a finder from within an instance method of the model itself using the "syntactic magic" from Model's __callStatic(), it will go through the __call() method instead.
Example:
class Users extends \lithium\data\Model {
public function getNext($entity, $token = null) {
$conditions = [...];
return static::first(compact('conditions'));
}
}
It invokes Model::__call() instead of Model::__callStatic() and I get the exception Unhandled method call 'first'.
If I define it this way, it'll work... but it's not ideal, and the magic method used to work just fine.
class Users extends \lithium\data\Model {
public function getNext($entity, $token = null) {
$conditions = [...];
return static::find('first',compact('conditions'));
}
}
Related stackoverflow threads:
- http://stackoverflow.com/questions/13232445/call-catches-static-method-calls
- http://stackoverflow.com/questions/8757696/how-to-force-callstatic-for-unknown-static-methods-even-if-instance-method-wit
This is a known php bug, i already talked about it on IRC. Actually it's not really a bug but more of a design decision. Parent calls wouldn't work if this wasn't the case. What I did is move most of my finders to static methods with explicit names. This makes the code somewhat more readable. But it isn't a fix I know.
It's not a huge nuisance to change my function calls, but I guess my main point is that this was an unintended BC break... these calls did used to work at one point.
So I guess the debate is, is it preferable to revert this commit and retain the prior ability to call these magic "finder" methods from within a model's instance methods, or is it preferable to keep the feature(s) added by pull request #744?
The previous behavior didn't respect the separation of concerns. Imo this "PHP ambiguity issue" should be managed by the Model class. Why not overriding Model::__call in your base model class with something like this ?
public function __call($method, $params) {
if ($method === 'all' || isset($this->_finders[$method]) {
array_shift($params);
return $this->_callFinders($method, $params);
}
return parent::__call($method, $params);
}
protected function _callFinders($method, $params) {
$isFinder = isset($this->_finders[$method]);
if ($isFinder && count($params) === 2 && is_array($params[1])) {
$params = array($params[1] + array($method => $params[0]));
}
if ($method === 'all' || $isFinder) {
if ($params && !is_array($params[0])) {
$params[0] = array('conditions' => static::key($params[0]));
}
return static::find($method, $params ? $params[0] : array());
}
throw new BadMethodCallException(...)
}
This should provide the behavior you're expecting imo.
If there's a simple work-around for this that doesn't add any overhead, that's fine. Strictly speaking, though, this is not a bug. The static keyword is only intended to be used in static contexts, so the fact that you're using it in a method (getNext()) that isn't statically-defined is wrong.
perhaps it's bad practice to use the static keyword here, though if I were to replace static::first() with Users::first() the issue still exists...
