Model finders can no longer be invoked from within an instance method.

[mikegreiling]

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

[marcghorayeb]

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.

[mikegreiling]

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?

[jails]

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.

[nateabele]

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.

[mikegreiling]

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...

[DrRoach]

I fixed this issue in #1161 and made a pull request.