Suggestion: Overloaded methods #16

Closed
adlawson opened this Issue Oct 21, 2010 · 5 comments

Projects

None yet

2 participants

@adlawson

I use this class as another layer ontop of my view renderer. My views often use view helpers, which are retrieved by __call(). Using method_exists() instead of is_callable() only looks for methods already "known" to the object without using overloading.

After some playing around, I've come up with a solution in the _findVariableInContext() method (~ line 603).

protected function _findVariableInContext($tag_name, $context) {
    foreach ($context as $view) {
        if (is_object($view)) {
            try {
                if(is_callable(array($view, $tag_name))) return $view->$tag_name();
            } catch (Exception $e) {
                // Suppress any exceptions
            }
            if(isset($view->$tag_name)) return $view->$tag_name;
        } else if (isset($view[$tag_name])) {
            return $view[$tag_name];
        }
    }

    if ($this->_throwsException(MustacheException::UNKNOWN_VARIABLE)) {
        throw new MustacheException("Unknown variable: " . $tag_name, MustacheException::UNKNOWN_VARIABLE);
    } else {
        return '';
    }
}

The try/catch suppresses any exceptions thrown during a possible __call() lookup, and the rest is really just the same as before.

@adlawson

Just a thought, but maybe even switch around if(isset($view->$tag_name))... and the try/catch to reduce unnecessary (potential) overhead

@bobthecow
Owner

We wouldn't want to switch the priority, as methods intentionally come before member variables. This allows something like this arguably contrived example:

<?php

class Foo {
    public $title;
    public function title() {
        return '<h1>' . $this->title .'</h1>';
    }
}

But is_callable is definitely a better approach than method_exists. Blindly discarding exceptions, however, isn't ;)

This issue will be fixed shortly. I'm closing this one as a duplicate of #19.

@bobthecow
Owner

... for discussion on method/variable priority, see issue #10

@adlawson

Thanks for the feedback.
I'd agree with you about not blindly discarding exceptions.

If is_callable() is used instead of method_exists(), I would make a note in the documentation that any exceptions thrown trying to call an un-callable method will not be caught (and should therefore be handled by the developer's framework/cms)

@bobthecow
Owner

Yup. I'll be sure to mention that in the release notes and docs. It also warrants a minor version bump, as it definitely has the potential to break existing code with __call methods.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment