Stdlib\Options: Misc enhancements

[Pittiplatsch]

• Minor performance tweak - consolidate calls to method_exists
• Replace method_exists with more reliable is_callable
• CS fixes

[Martin-P]

Replace method_exists with more reliable is_callable

Behaviour is different, is_callable() also returns true for magic methods. This can be considered a BC break. Furthermore, is_callable() is slightly slower than method_exists().

[Martin-P]

ZF2 normally uses sprintf() for exception messages.

throw new Exception\BadMethodCallException(sprintf(
    'The option "%s" does not  have a callable '
    . '"%s" setter method which must be defined',
    $key,
    $setter
));

[Pittiplatsch]

is_callable returning true for magic methods isn't relevant here due to predefined method prefixes get / set which obviously don't touch magic methods.
However, is_callable respects method visibility / accessibility, where method_exist doesn't. Moreover, is_callable has improved testability when it comes to mocked methods, while mocked methods don't play well with method_exists.

[Martin-P]

When someone uses __call() in their class, this change is a BC break:

<?php

namespace MyNamespace;

use Zend\Stdlib\AbstractOptions;

class CustomOptions extends AbstractOptions
{
    public function __call($methodName, $arguments)
    {
    }
}

$options = new CustomOptions();
var_dump(is_callable(array($options, 'getFoo'))); // true
var_dump(method_exists($options, 'getFoo'));      // false

[Pittiplatsch]

Uh, you're right - didn't see that :-(

Don't know though if this (undoubtly existing) BC is relevant.
It could also be treated as new feature "allow getters / setters implemented via magic __call". I can hardly think of a use-case where this BC applies, as it doesn't prevent any existing functionality, but allows new implementation.

[Ocramius]

You can swap this conditional by making the setter call if is_callable(), and handling exceptions later on. Simplifies the method by a lot.

[Ocramius]

Wondering if a static local cache would bring any benefit here, or if it's a total overkill.

[Martin-P]

@Ocramius I think the milestone should be 2.4.0 because is_callable() introduces a BC break. See my previous comment: #6884 (comment)

[Ocramius]

When someone uses __call() in their class, this change is a BC break

Requires a documentation change: using __call() on this kind of class (in a subclass) is at the limit of nonsense IMO :-)

And yes, moved to 2.4

[Ocramius]

@Pittiplatsch can you check the new behavior with a test that throws an exception with protected and private methods?

[Pittiplatsch]

Will think about it tomorrow.

[Pittiplatsch]

Tests added, but one of them fails although IMO it should succeed.
This however looks like a general issue about visibility I never realized.
See this gist for details: https://gist.github.com/Pittiplatsch/f0ca0485d8e96ca91318

[Ocramius]

Played with your gist a bit: http://3v4l.org/mQ57X

May be related with https://bugs.php.net/bug.php?id=29210 and https://bugs.php.net/bug.php?id=33277, not sure tho.

It seems to me that is_callable() considers the current scope when detecting if a method is callable.

This makes sense though, as it is indeed not possible to call private methods from the scope of parent or child classes.

[Pittiplatsch]

IMO is_callable() returns correct results in whether a method can / may actually be called at the position of testing it.
However, I am struggling with the fact that methods being defined in child class cannot be called from a parent's method, although the class instance (and therefore as I thought the scope?!?) exactly is the child which obviously IS allowed to access its own private method.
As I already stated in the beginning of my gist, with the given situation there is no way of implementing IOC with abstract private methods, right?

[Ocramius]

As I already stated in the beginning of my gist, with the given situation there is no way of implementing IOC with abstract private methods, right?

Correct. private members are meant for implementation details.

[Pittiplatsch]

"Again what learned" ;-)
Ergo: Those unit tests which fail do so justifiably => I'll adjust them accordingly, ok?

[Ocramius]

@Pittiplatsch right: I think we have to leave the BC Break label though.

[Pittiplatsch]

Unit tests are passing now.

BC break: IMO this isn't necessary, because switching from method_exists to is_callable() is:

• a bug fix (method_exists(ChildClass::setParentPrivate) => true, although ChildClass::setParentPrivate() actually cannot be called a few lines later (https://github.com/Pittiplatsch/zf2/blob/2d77203e9f8b32daf801b7fcb9a9bd2429afbed7/library/Zend/Stdlib/AbstractOptions.php#L109), which finally results in a PHP fatal!)
• a new feature (usage of __call() is now possible -> Stdlib\Options: Misc enhancements  #6884 (comment))

[Ocramius]

@Pittiplatsch it still needs documentation for these edge-cases (documented limitation)

[Ocramius]

@Pittiplatsch merged into develop at 729e4be, thanks!