Fix #68475 - Support $callable() syntax for all callable strings.

[trowski]

The $callable() syntax when used with a string of the form 'Class::method' would error as an undefined function, even if the string returned true from is_callable() or the callable type-hint. This fix adds support for the $callable() syntax for any string accepted by is_callable() or the callable type-hint.

class TestClass
{
    public static function staticMethod()
    {
        echo "Static method called!\n";
    }
}

function test(callable $callback)
{
    $callback();
}

$callable = 'TestClass::staticMethod';

var_dump(is_callable($callable));
test($callable);

Prior output:

bool(true)
PHP Fatal error: Call to undefined function TestClass::staticMethod() in ...

After patch:

bool(true)
Static method called!

[laruence]

I think a proper ERROR should also be triggered here.

[trowski]

This does trigger a 'Class not found' error in the same way passing an array ['ClassName', 'methodName'] behaves if ClassName does not exist.

[laruence]

hmm, then okey, I thought a similar error "call to undefined" should be triggered. but fine, let keep them consistent.. thanks

[trowski]

Oops, forgot to add the deprecated notice to the test expected output. Fixed now.

[evert]

There's a failed unittest in there as well =)

[jpauli]

Linking https://bugs.php.net/bug.php?id=68475

[trowski]

It's my own unit test that is failing, but it passes when I build it on my computer. It seems the build on travis actually does call the non-static method when I try to call it statically, rather than throw an EngineException like I was expecting.

The code that checks if the method can be called statically is copied directly from the portion that handles calls based on arrays like ['ClassName', 'methodName'], so I would imagine that would fail in a similar way.

========DIFF========
005+ 

006+ Warning: Attempt to assign property of non-object in /home/travis/build/php/php-src/Zend/tests/bug68475.php on line 8

007+ 

008+ Notice: Trying to get property of non-object in /home/travis/build/php/php-src/Zend/tests/bug68475.php on line 9

005- Using $this when not in object context
========DONE========
FAIL Bug #68475 Calling function using $callable() syntax with strings of form 'Class::method' [Zend/tests/bug68475.phpt] 

It seems the code to check if the method can be called statically is saying it can be called statically. I tried the array syntax and it fails in the same way, so maybe there's a bug in both the code I added and the code that handles array style callbacks.

[marcioAlmada]

Would be great to have tests with:

$callback($arg, $arg);
$callback(...$args);

[trowski]

I reduced the scope of the unit test I added with this pull, since it seemed to find a separate issue that needs to be addressed. It seems that the ZEND_ACC_ALLOW_STATIC flag is being set for functions that contain $this when being called dynamically. The test failed when I called the method using either string syntax supported by this pull or array syntax. This will probably need to be addressed by someone who's more familiar with internals than I am.

@marcioAlmada Sure, I can add some more tests with arguments.

[olvlvl]

Thanks @trowski for fixing this callable paradox, I hope your PR gets merged real soon and that we don't ever have to use call_user_func().

[trowski]

I added a few tests with arguments to the unit test, but oddly it didn't trigger a travis build. Can someone manually trigger the build. The tests are simple and passed on my local build.

Thoughts on getting this merged?

[marcioAlmada]

@trowski sometimes travis refuses to start builds for pull requests. Try to git commit --amend the last commit and do a forced push.

[trowski]

@marcioAlmada Thank you, that worked.

[jpauli]

No test failing for me on my env.
I'm OK to merge this.

@laruence : more things to add ?

[laruence]

@jpauli No. :)

[jpauli]

Merged

[olvlvl]

Quick question, will this fix apply to all PHP version, or only PHP7?

[jpauli]

This is merged against PHP7

[olvlvl]

Since this can be considered as a bug fix, do you think there's a chance that PHP5.4 - 5.6 will also be fixed? The type callable was introduced in PHP 5.4, and is_callable() returns true for "A::b" there.

[jpauli]

No sorry. This is controversal, we prefer not changing PHP5 VM stability and performance, but bet on PHP7.

[olvlvl]

Ok I understand. Thanks for your reply.