Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement `respondsTo` in `Object`, `StaticObject` and child classes. #807

Merged
merged 1 commit into from Feb 1, 2013

Conversation

Projects
None yet
3 participants
Member

blainesch commented Jan 29, 2013

Fixes #806

Member

blainesch commented Jan 30, 2013

I'm really glad this checks against the current repo's .travis.yml file instead of the repo owners current one. It worried me when I made that hotfix on it.

@nateabele Should my quality updates be added to the original commit or do you have a preference on commit short description?

Contributor

jails commented Jan 30, 2013

But why this is not enough ?

try{
       $object->method();
} catch (BadMethodCallException $e) {
       //doing the stuff we need to do in a if(Class::respondsTo() === false)
}
Member

blainesch commented Jan 30, 2013

Calling a bad method will throw a "fatal error" which isn't catch-able. You could then do a try/catch with an if (method_exists()) { but now it's starting to look nasty.

Contributor

jails commented Jan 30, 2013

I personnaly think all the public/private/protected check should be left to a kind of Inspector class instead of beeing integrated in the core (i.e moving the respondsTo to Inspector for example). And If you really need to support magic call, I think adding a isMagicMethod('method') which returns true/false is a better option. Then with a (method_exists() && is_callable()) || Class::isMagicMethod() you can filter for all public methods or you can use the ReflexionXXX if you need more stuff.

But I'm not sure this need is common enough. May adding a isMagicMethod which returns false in Object && StaticObject and let users implementing it (or not) would be enough.

@nateabele nateabele and 1 other commented on an outdated diff Jan 31, 2013

data/Model.php
@@ -1089,11 +1102,11 @@ public function save($entity, $data = null, array $options = array()) {
* @see lithium\data\Entity::errors()
* @param string $entity Model entity to validate. Typically either a `Record` or `Document`
* object. In the following example:
- * {{{
- * $post = Posts::create($data);
- * $success = $post->validates();
- * }}}
- * The `$entity` parameter is equal to the `$post` object instance.
+ * {{{
@nateabele

nateabele Jan 31, 2013

Owner

I'm pretty sure this was outdented because indenting it plays hell with the formatting in li3_docs.

@blainesch

blainesch Jan 31, 2013

Member

Well it sounds bad to fix li3_docs and/or li3_quality to work around this. I'd suggest that I go back and refactor the comment and get the code into the description where it belongs.

@nateabele

nateabele Jan 31, 2013

Owner

Yeah, fair enough. Are you volunteering? ;-)

@nateabele nateabele commented on an outdated diff Jan 31, 2013

data/source/MongoDb.php
@@ -581,7 +594,8 @@ public function delete($query, array $options = array()) {
protected function _deleteFile($conditions, $options = array()) {
$defaults = array('safe' => true);
$options += $defaults;
- return $this->connection->getGridFS($this->_config['gridPrefix'])->remove($conditions, $options);
+ return $this->connection->getGridFS($this->_config['gridPrefix'])
@nateabele

nateabele Jan 31, 2013

Owner

I'd prefer it if the result of getGridFS() were assigned to a variable here instead. This kind of formatting would make sense if fluent interfaces were pervasive throughout the core, but as it stands, I don't think this formatting is used anywhere else.

@nateabele nateabele and 1 other commented on an outdated diff Jan 31, 2013

template/helper/Form.php
@@ -612,11 +625,12 @@ protected function _selectOptions(array $list, array $scope) {
$result .= $this->_render('select', 'option-group', $params);
continue;
}
- $selected = (
@nateabele

nateabele Jan 31, 2013

Owner

Does li3_quality flag this as bad, currently?

@blainesch

blainesch Jan 31, 2013

Member

Yes, operators should have spaces on both sides so $i || would fail without the trailing space.

@nateabele

nateabele Jan 31, 2013

Owner

I get that. As a coding construct, however, the former approach is more cohesive and easily comprehensible. Would it be possible to count \n as whitespace, without otherwise introducing inconsistency?

@blainesch

blainesch Jan 31, 2013

Member

I can edit the rule. should this just apply to the following?

&&, ||, and, or, and xor

@nateabele

nateabele Jan 31, 2013

Owner

Cool, thanks. Yeah, I think those should be sufficient, I can't think of any others that would be valid in this scenario.

Owner

nateabele commented Jan 31, 2013

Hey guys, sorry it's taken me so long to properly review this. I have to say, I was initially pretty skeptical about adding a method like this to the base classes, but skimming through the patch, I can see how it makes a lot of sense.

@blainesch If you can burn through my comments quick, I'll get this merged in. In response to your question, it makes sense to have each of those as separate commits, so I'm good with leaving them as they are. Come to think of it, maybe I should update the contributor guide to reflect the idea that commits shouldn't always be flatten if they actually represent different changes.

@nateabele nateabele and 2 others commented on an outdated diff Jan 31, 2013

analysis/Inspector.php
@@ -49,6 +50,32 @@ class Inspector extends \lithium\core\StaticObject {
);
/**
+ * Will determine if a method can be called.
+ *
+ * @param string|object $class Class to inspect.
+ * @param string $method Method name.
+ * @param integer $visibility Visibility (0 for public, 1 for public
+ * and protected, and 2 for all visibility)
+ * @return bool
+ */
+ public static function callable($object, $method, $visibility) {
@nateabele

nateabele Jan 31, 2013

Owner

For the sake of consistency with other methods that return boolean results, I think isCallable() would make for a better name.

@blainesch

blainesch Jan 31, 2013

Member

It wouldn't match the prototype for it's parent class StaticObject since this introspects $object not self.

@nateabele

nateabele Jan 31, 2013

Owner

Sorry, I'm not following. I don't see a callable() method defined anywhere currently (unless I'm misunderstanding your meaning of 'prototype').

@jails

jails Jan 31, 2013

Contributor

And why not simply return is_callable(array($object, $method)) && method_exists($object, $method) if $visibility === 0 ?

@nateabele

nateabele Jan 31, 2013

Owner

Actually, yeah, that reminds me that using the Reflection API is a huge no-no in production code. Unless respondsTo() is only ever meant for testing (and hey, even if not), we'd do well to optimize it as best we can. I can see a few other examples where a simple reversal of || conditions should do the trick. (Sorry this is turning into such a novel, btw!)

@blainesch

blainesch Jan 31, 2013

Member

@nateabele StaticObject has a respondsTo($method, $visibility) so this cannot have a respondsTo($object, $method, $visibility). Even if we could overwrite it we'd also be unable to check respondsTo on Inspector with a consistent api.

I'll look into getting rid of Reflection. Thoughts on dropping private support? I'm sure with just is_callable() and method_exists() we can determine just public and protected without Reflection.

@nateabele

nateabele Jan 31, 2013

Owner

Okay, I'm really hoping this is the source of the confusion, otherwise I need to go get another cup of coffee. :-)

StaticObject has a respondsTo($method, $visibility) so this cannot have a respondsTo($object, $method, $visibility).

Yeah, I said change callable() to isCallable(), not callable() to respondsTo(). Hopefully now this all makes sense. :-)

@jails

jails Jan 31, 2013

Contributor

Imo nateabele talked about renaming Inspector::callable($object, $method, $visibility) instead of Inspector::isCallable($object, $method, $visibility) ?

@jails

jails Jan 31, 2013

Contributor

But still thinking the visitility doesn't make sense here since protected/private won't ever "respondsTo" something. And Inspector::isCallable() should simply return is_callable(array($object, $method)) && method_exists($object, $method) to allow the respondsTo feature on public methods. Moreover I would tend to say a ::isCallable should return false on protected/private methods.

Member

blainesch commented Feb 1, 2013

@nateabele finally done! :D

@nateabele nateabele added a commit that referenced this pull request Feb 1, 2013

@nateabele nateabele Merge pull request #807 from BlaineSch/feature/respondsTo
Implement `respondsTo` in `Object`, `StaticObject` and child classes.
05abf7b

@nateabele nateabele merged commit 05abf7b into UnionOfRAD:dev Feb 1, 2013

1 check passed

default The Travis build passed
Details

@jails jails commented on the diff Feb 1, 2013

analysis/Inspector.php
@@ -49,6 +49,22 @@ class Inspector extends \lithium\core\StaticObject {
);
/**
+ * Will determine if a method can be called.
+ *
+ * @param string|object $class Class to inspect.
+ * @param string $method Method name.
+ * @param bool $internal Interal call or not.
+ * @return bool
+ */
+ public static function isCallable($object, $method, $internal = false) {
+ $methodExists = method_exists($object, $method);
+ $callable = function($object, $method) {
+ return is_callable(array($object, $method));
+ };
+ return $internal ? $methodExists : $methodExists && $callable($object, $method);
@jails

jails Feb 1, 2013

Contributor

I don't really understand why a closure is needed here. Calling directly is_callable(array($object, $method)); was not enough ?

@blainesch

blainesch Feb 1, 2013

Member

Imagine cat is StaticObject and cougar is Inspector.

<?php

class cat {
    protected function speak() {
        return 'meow';
    }
}

class lion extends cat {}

class cougar extends cat {
    public function __construct() {
        $lion = new lion();
        echo $lion->speak(); // meow
    }
}


new cougar();
@jails

jails Feb 1, 2013

Contributor

Ok, that's what I thought but this "scoping trick" won't work for PHP 5.4 and above. So function($object, $method) {return is_callable(array($object, $method));} and is_callable(array($object, $method)); will return the same result. If you want to take this specific case into account, better to not extends Inspector from \lithium\core\StaticObject.

@jails jails commented on the diff Feb 1, 2013

data/model/Relationship.php
@@ -135,6 +135,17 @@ public function __call($name, $args = array()) {
return $this->data($name);
}
+ /**
+ * Custom check to determine if our given magic methods can be responded to.
+ *
+ * @param string $method Method name.
+ * @param bool $internal Interal call or not.
+ * @return bool
+ */
+ public function respondsTo($method, $internal = false) {
+ return is_callable(array($this, $method), true);
@jails

jails Feb 1, 2013

Contributor

isset($this->_config[$method]) || parent::respondsTo($method, $internal); ?

@blainesch blainesch deleted the unknown repository branch Feb 1, 2013

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